Added a 'strict' property to ExecutionContextPromotionListener to ensure the key is present [BATCH-1309]

[spring-projects-issues]

Dan Garrette opened BATCH-1309 and commented

Added a 'strict' property to ExecutionContextPromotionListener. If set to TRUE, the listener will ensure that all of the keys actually exist to ensure the key is present. If a key is missing from the step's ExecutionContext, then an exception will be thrown.

---

Affects: 2.0.1

Attachments:

• ExecutionContextPromotionListenerFix.patch (2.28 kB)
• ExecutionContextPromotionListenerFix2.patch (2.50 kB)

Referenced from: commits fe26ccb, 822f8bb

[spring-projects-issues]

Randy Hall commented

I noticed that the change introduced when resolving this issue introduces its own bug due to a logic problem.

From ExecutionContextPromotionListener.afterStep, lines 55-63:

for (String key : keys) {
if (stepContext.containsKey(key)) {
if (strict) {
throw new IllegalArgumentException("The key [" + key

• "] was not found in the Step's ExecutionContext.");
  }
  jobContext.put(key, stepContext.get(key));
  }
  }

Basically, what this code says that if the key is present (the if block) and then it subsequently checks the strict boolean, it throws an exception.

The truth table goes like this:

1: !stepContext.containsKey(key) && !strict ==> no action
2: !stepContext.containsKey(key) && strict ==> no action [NOTE: this is the case that BATCH-1309 is trying to catch!]
3: stepContext.containsKey(key) && !strict ==> jobContext.put()
4: stepContext.containsKey(key) && strict ==> throws IllegalArgumentException [Behavior should be that this succeeds and #2 above fails]

Code to demonstrate this:

public static void main(String[] args) {
    // Load the charge
    final String REQUIRED_KEY = "required.key";
    JobInstance jobInstance = new JobInstance(0L, new JobParameters(), "jobInstance");
    JobExecution jobExecution = new JobExecution(jobInstance);
    StepExecution stepExecution = new StepExecution("step", jobExecution);
    ExecutionContext executionContext = new ExecutionContext();
    executionContext.put(REQUIRED_KEY, "required.value");
    stepExecution.setExecutionContext(executionContext);

    // Light the fuse
    ExecutionContextPromotionListener listener = new ExecutionContextPromotionListener();
    listener.setKeys(new String[] {REQUIRED_KEY});
    listener.setStrict(true);

    // Watch fireworks
    listener.afterStep(stepExecution);
}

[spring-projects-issues]

Randy Hall commented

I checked the test case that tests BATCH-1309 (i.e. spring-batch-core/src/test/java/org/springframework/batch/core/listener/ExecutionContextPromotionListenerTests.java), and the reason it passes is because the first key that was present in the list of keys to promote is what threw the IllegalArgumentException that allowed the test to pass, rather than the lack of key2. I'll attach a patch for the fix to both the class and the test case.

[spring-projects-issues]

Randy Hall commented

Here's a patch file for the changes I'm proposing to make 'strict' work correctly.

[spring-projects-issues]

Dave Syer commented

I'm not sure I agree necessarily. The log message is mistyped, but the summary of BATCH-1309 says: "ensure the key is present" when strict=true, and that's what the code does right?

[spring-projects-issues]

Randy Hall commented

Disregard last patch, I hadn't run the unit tests and completely hosed my off-the-cuff recollection of JUnit Assert. This one actually passes.

[spring-projects-issues]

Randy Hall commented

Dave,

I think if you look carefully, you'll see my logic is sound. The only reason I say that is that I attempted to actually use the strict setting to ensure a key got promoted, and ended up running the code through a debugger to figure out why I was getting the IllegalArgumentException.

[spring-projects-issues]

Dave Syer commented

I'm still confused then because strict=true will not ensure that a key gets promoted (it will intentionally barf if you try to overwrite an existing key). If you want to unconditionally promote a key you need strict=false, right?

[spring-projects-issues]

Dave Syer commented

OK, I get it. We need a new issue to track the bug though, sinc ethis one was marked resolved in 2.0.3. I guess nobody uses strict=true (see BATCH-1547).