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

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
+25 −6
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4f4111
Fix dirty flag set to false on same value put
GGHDMS Oct 27, 2024
2c98c7d
Fix prevent dirty flag from being set to false when pushing null
GGHDMS Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions ...g-batch-infrastructure/src/main/java/org/springframework/batch/item/ExecutionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2006-2023 the original author or authors.
* Copyright 2006-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -37,6 +37,7 @@
* @author Lucas Ward
* @author Douglas Kaminsky
* @author Mahmoud Ben Hassine
* @author Seokmun Heo
*/
public class ExecutionContext implements Serializable {

Expand Down Expand Up @@ -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.


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

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

Expand Down
17 changes: 14 additions & 3 deletions ...ch-infrastructure/src/test/java/org/springframework/batch/item/ExecutionContextTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2006-2023 the original author or authors.
* Copyright 2006-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -30,7 +30,7 @@
/**
* @author Lucas Ward
* @author Mahmoud Ben Hassine
*
* @author Seokmun Heo
*/
class ExecutionContextTests {

Expand Down Expand Up @@ -88,11 +88,13 @@ void testNotDirtyWithDuplicate() {
}

@Test
void testNotDirtyWithRemoveMissing() {
void testDirtyWithRemoveMissing() {
context.putString("1", "test");
assertTrue(context.isDirty());
context.putString("1", null); // remove an item that was present
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.


context.clearDirtyFlag();
context.putString("1", null); // remove a non-existent item
assertFalse(context.isDirty());
}
Expand Down Expand Up @@ -161,6 +163,15 @@ void testCopyConstructorNullInput() {
assertTrue(context.isEmpty());
}

@Test
void testDirtyWithDuplicate() {
ExecutionContext context = new ExecutionContext();
context.put("1", "testString1");
assertTrue(context.isDirty());
context.put("1", "testString1"); // put the same value
assertTrue(context.isDirty());
}

/**
* Value object for testing serialization
*/
Expand Down