Skip to content

Fix JsonFileItemWriter for multiple write's #636

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.

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

Closed

Conversation

gd-estrepetov
Copy link

Resolves BATCH-2749

@fmbenhassine
Copy link
Contributor

Thank you for reporting this issue and for opening a PR! 👍

The problem is that the snippet that adds a separator if a previous chunk has been written is inside the loop. So I would not add a boolean (alreadyAddedLineSeparator) but instead we need to move the snippet outside the loop:

@Override
public String doWrite(List<? extends T> items) {
	StringBuilder lines = new StringBuilder();
+	if (!items.isEmpty() && state.getLinesWritten() > 0) {
+		lines.append(JSON_OBJECT_SEPARATOR).append(this.lineSeparator);
+	}
	Iterator<? extends T> iterator = items.iterator();
	while (iterator.hasNext()) {
-		if (iterator.hasNext() && state.getLinesWritten() > 0) {
-			lines.append(JSON_OBJECT_SEPARATOR).append(this.lineSeparator);
-		}
		T item = iterator.next();
		lines.append(' ').append(this.jsonObjectMarshaller.marshal(item));
		if (iterator.hasNext()) {
			lines.append(JSON_OBJECT_SEPARATOR).append(this.lineSeparator);
		}
	}
	return lines.toString();
}

There is another test that needs to be updated (in addition to the ones you added in this PR): JsonSupportIntegrationTests. For this one, just adding a line in the trades.json file is enough. For example:

[
 {"isin":"123","quantity":5,"price":10.5,"customer":"foo","id":1,"version":0},
 {"isin":"456","quantity":10,"price":20.5,"customer":"bar","id":2,"version":0},
- {"isin":"789","quantity":15,"price":30.5,"customer":"baz","id":3,"version":0}
+ {"isin":"789","quantity":15,"price":30.5,"customer":"baz","id":3,"version":0},
+ {"isin":"100","quantity":20,"price":35.5,"customer":"bat","id":4,"version":0}
]

Could you please update the PR with these changes? Thank you upfront!

@gd-estrepetov
Copy link
Author

Fixed your comments, by the way it should be at least 4 objects to reproduce the issue, if size of chunk is 2.

@fmbenhassine
Copy link
Contributor

Thank you for updating the PR! LGTM.

In my previous comment, I meant the spring-batch-samples/src/test/resources/org/springframework/batch/item/json/trades.json file needs also to be updated with 4 items. I'll do it when rebasing this PR.

@mminella
Copy link
Member

LGTM. Rebase, squashed, and merged as 7a3248c. Thanks for the catch and the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants