Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ExecutionContext dirty flag set to false on same value put or null put #4691

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix ExecutionContext dirty flag set to false on same value put or null put #4691

wants to merge 2 commits into from
+25 −6

Conversation

GGHDMS
Copy link

@GGHDMS GGHDMS commented Oct 27, 2024 •
edited
Loading

Description

Resolved an issue where the dirty flag was incorrectly set to false when a put operation used the same value as the existing one or when null was pushed. Now, the flag remains true if a prior change has occurred.

Solution

Adjusted the put method logic to correctly handle value comparisons:

if (value != null) {
	Object result = this.map.put(key, value);
	boolean newDirty = result == null || !result.equals(value);

	if (!this.dirty) {
		this.dirty = newDirty;
	}
}
else {
	Object result = this.map.remove(key);

	if (!this.dirty) {
		this.dirty = result != null;
	}
}

Fix #4685 #4692

@pivotal-cla
Copy link

@GGHDMS Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@GGHDMS Thank you for signing the Contributor License Agreement!

@GGHDMS GGHDMS mentioned this pull request Oct 27, 2024
Open
@GGHDMS GGHDMS changed the title Fix dirty flag set to false on same value put Fix ExecutionContext dirty flag set to false on same value put Oct 27, 2024
@GGHDMS GGHDMS force-pushed the fix-put-same-value-to-false branch from 05e79f3 to b4f4111 Compare October 27, 2024 06:55
@mjwiq
Copy link

mjwiq commented Oct 28, 2024

This should also be in the else branch. If the inserted value is null we don't want to remove the dirty flag either.

@mjwiq
Copy link

mjwiq commented Oct 28, 2024 •
edited
Loading

One of the old tests is wrong too:

spring-batch/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/ExecutionContextTests.java

Lines 94 to 97 in cf44ca8

context.putString("1", null); // remove an item that was present
assertTrue(context.isDirty());
context.putString("1", null); // remove a non-existent item
assertFalse(context.isDirty());

@GGHDMS GGHDMS changed the title Fix ExecutionContext dirty flag set to false on same value put Fix ExecutionContext dirty flag set to false on same value put or null push Oct 28, 2024
@GGHDMS
Copy link
Author

GGHDMS commented Oct 28, 2024 •
edited
Loading

Thank you for the feedback! @mjwiq

I have addressed the issue raised in #4692 with this PR. Specifically, I have modified the else branch to ensure that when null is pushed, it does not cause the dirty flag to be incorrectly set to false. Additionally, I have corrected the old test case that was not accurately reflecting the intended behavior. Please let me know if there are any further adjustments needed.

@GGHDMS GGHDMS changed the title Fix ExecutionContext dirty flag set to false on same value put or null push Fix ExecutionContext dirty flag set to false on same value put or null put Oct 28, 2024
mjwiq
mjwiq reviewed Oct 28, 2024
View reviewed changes
...batch-infrastructure/src/test/java/org/springframework/batch/item/ExecutionContextTests.java
context.putString("1", "test");
assertTrue(context.isDirty());
context.putString("1", null); // remove an item that was present
assertTrue(context.isDirty());
context.putString("1", null); // remove a non-existent item
assertFalse(context.isDirty());
assertTrue(context.isDirty());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By changing this line you change the meaning of the test. I think it would be better to clear the dirty flag before setting it to null for the second time

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually testing that it stays dirty when inserting null twice could then be done in its own test or testDirtyWithDuplicate

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some adjustments to the test based on the feedback. I added a step to clear the dirty flag before trying to remove an already non-existent item. This way, the test clearly checks that the dirty state doesn’t change in that situation. I think it makes the intent of the test more explicit and separates the different scenarios, making it easier to understand.

mjwiq
mjwiq reviewed Oct 28, 2024
View reviewed changes
spring-batch-infrastructure/src/main/java/org/springframework/batch/item/ExecutionContext.java
@@ -124,11 +125,18 @@ public void putDouble(String key, double value) {
public void put(String key, @Nullable Object value) {
if (value != null) {
Object result = this.map.put(key, value);
this.dirty = result == null || !result.equals(value);
boolean newDirty = result == null || !result.equals(value);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick, but I'd prefer a single line change this.dirty = this.dirty || result == null || !result.equals(value); (and similar for the else case)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think one line of code was intuitive, so I made it through newDirty

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's a personal thing then. I find the or statements easier to read. Let's see if we get a 3rd opinion from the maintainers

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your point, and I think it’s a good idea to get more opinions. I chose this approach because I wanted to explicitly handle situations where the state changes from true to false, ensuring that it doesn’t happen unintentionally.

@GGHDMS GGHDMS force-pushed the fix-put-same-value-to-false branch from 7401f7e to 0b5a670 Compare October 28, 2024 11:37
@GGHDMS GGHDMS force-pushed the fix-put-same-value-to-false branch from 0b5a670 to 2c98c7d Compare October 28, 2024 11:38
@Solodye
Copy link
Contributor

Solodye commented Nov 4, 2024

Just curious, are there any UT that proved that the dirty flag affects the partitioner behaviour?

I am curious about this because the previous issue #2020 (comment) mentioned this.

@injae-kim injae-kim mentioned this pull request Nov 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjwiq mjwiq mjwiq left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ExecutionContext dirty flag is reset by a new put
4 participants
@GGHDMS @pivotal-cla @mjwiq @Solodye