Add a streaming Json item reader #607

fmbenhassine · 2018-05-22T11:44:18Z

This PR adds a new Json item reader with two implementations based on Jackson and gson.

This commit adds a new Json item reader with two implementations based on Jackson and Gson. Resolves BATCH-2691

mminella · 2018-05-30T02:51:57Z

...sts/src/test/java/org/springframework/batch/item/json/GsonJsonItemReaderFunctionalTests.java

+
+	@Override
+	protected Class<? extends Exception> getJsonParsingException() {
+		return JsonSyntaxException.class;


These exceptions being exposed outside the impl is a leaky abstraction. We should be catching the implementation's exception and wrapping it in a standard exception. Using the Marshaller as an example, it throws an XmlMappingException if there is an issue and that would wrap the underlying implementation's exception.

I agree! Good catch. The best exception I see as a wrapper is org.springframework.batch.item.ParseException.

Fixed in 53262e6. Now the implementation detail exception is wrapped in a ParseException.

mminella · 2018-05-30T02:54:18Z

...g-batch-infrastructure/src/main/java/org/springframework/batch/item/json/JsonItemReader.java

+	public void afterPropertiesSet() throws Exception {
+		Assert.notNull(this.jsonObjectReader, "The json object reader must not be null.");
+		Assert.notNull(this.resource, "The resource must not be null.");
+	}


This really is an old school way of doing things. We should require these in the constructor if possible.

For the StaxEventItemReader, both the resource and unmarshaller are mandatory resources but they are not required at construction time (they are checked in afterPropertiesSet). I've seen this in many places in the code base like for example the ValidatingItemProcessor where I can create an instance with the default constructor while the validator is mandatory. Another example is the JmsItemReader: the jmsTemplate is mandatory but it is not required in the constructor.

When XML was the preferred way to configure things, setter injection and using InitializingBean made sense (since java doesn't have named parameters). However, with java config the norm now, we should take advantage of that and utilize constructor injection where we can. Also, given the builders will be used in most cases going forward, there's even less reason to take this approach.

I think it's more about class design than configuration style: I should not be able to create a class without specifying its required dependencies (fail fast, at construction time) rather than letting me create the class and then throw me an exception when the container calls afterPropertiesSet. Here, I used the InitializingBean way for consistency with the current code base, but for me, we should always use constructor injection for required dependencies and setter injection for optional ones regardless of the configuration style. I couldn't agree more with you, so I fixed that in 493196c.

mminella · 2018-05-30T02:57:00Z

...g-batch-infrastructure/src/main/java/org/springframework/batch/item/json/JsonItemReader.java

+ * @since 4.1
+ */
+public class JsonItemReader<T> extends AbstractItemCountingItemStreamItemReader<T> implements
+		ResourceAwareItemReaderItemStream<T>, InitializingBean {


Since you're already bringing in the ItemStream and the ItemReader interfaces via the AbstractItemCountingItemStreamItemReader, you can probably just implement ResourceAware (no need to use the composite).

I took the same as StaxEventItemWriter.

mminella · 2018-05-30T17:39:15Z

...batch-infrastructure/src/main/java/org/springframework/batch/item/json/JsonObjectReader.java

+	 * @param resource the input resource
+	 * @throws Exception if unable to open the resource
+	 */
+	default void open(Resource resource) throws Exception {


We try not to throw Exception unless there is a real reason to do so in an API. We'd rather throw a documented RuntimeException if possible which this feels like it should be able to.

Here I think there is a real reason to do so. If the implementation cannot open the resource, we gave it a chance to throw a checked exception and signal the problem to the framework. We do the same in org.springframework.batch.item.support.AbstractItemCountingItemStreamItemReader#doOpen. The javadoc makes sense to me: @throws Exception Allows subclasses to throw checked exceptions for interpretation by the framework.

mminella · 2018-05-30T17:39:33Z

...batch-infrastructure/src/main/java/org/springframework/batch/item/json/JsonObjectReader.java

+	 * @return the next object or null if the resource is exhausted
+	 * @throws Exception if unable to read the next object
+	 */
+	T read() throws Exception;


See above about throwing Exception

Same as previous comment on doOpen. We throw an exception in org.springframework.batch.item.support.AbstractItemCountingItemStreamItemReader#doRead and org.springframework.batch.item.ItemReader#read.

mminella · 2018-05-30T17:39:45Z

...batch-infrastructure/src/main/java/org/springframework/batch/item/json/JsonObjectReader.java

+	 * Close the input resource.
+	 * @throws Exception if unable to close the input resource
+	 */
+	default void close() throws Exception {


See above about throwing Exception

Same as org.springframework.batch.item.support.AbstractItemCountingItemStreamItemReader#doClose.

mminella · 2018-05-30T18:20:49Z

...ure/src/test/java/org/springframework/batch/item/json/builder/JsonItemReaderBuilderTest.java

+			new JsonItemReaderBuilder<String>()
+					.jsonObjectReader(this.jsonObjectReader)
+					.resource(this.resource)
+					.saveState(true)


This line is redundant isn't it (save state defaults to true)?

I thought being explicit is better for this specific test case. But I will remove it, I agree it is not really necessary.

fixed in 5b6e7b9

wrapping them in a ParseException

mminella · 2018-05-31T13:15:44Z

LGTM. Rebased, squashed, and merged as 5281d6d

fmbenhassine added the ready-to-review label May 22, 2018

fmbenhassine force-pushed the BATCH-2691 branch 2 times, most recently from 9f4c4ec to 284e0b5 Compare May 22, 2018 16:15

fmbenhassine added waiting-for-feedback and removed ready-to-review labels May 22, 2018

fmbenhassine force-pushed the BATCH-2691 branch from 284e0b5 to ba27fb9 Compare May 22, 2018 18:34

fmbenhassine added ready-to-review and removed do-not-merge-yet labels May 22, 2018

fmbenhassine force-pushed the BATCH-2691 branch 2 times, most recently from c04d17f to 56e9720 Compare May 23, 2018 08:45

Add a streaming Json item reader

d1b9998

This commit adds a new Json item reader with two implementations based on Jackson and Gson. Resolves BATCH-2691

fmbenhassine force-pushed the BATCH-2691 branch from 56e9720 to d1b9998 Compare May 28, 2018 18:29

mminella reviewed May 30, 2018

View reviewed changes

fmbenhassine added waiting-for-feedback and removed ready-to-review labels May 30, 2018

fmbenhassine added 2 commits May 31, 2018 00:48

BATCH-2691: remove saveState(true) since it is the default

5b6e7b9

BATCH-2691: remove leaky abstraction of json parsing exceptions by

53262e6

wrapping them in a ParseException

fmbenhassine added ready-to-review and removed waiting-for-feedback labels May 30, 2018

BATCH-2691: use constructor injection for required dependencies

493196c

mminella closed this May 31, 2018

fmbenhassine deleted the BATCH-2691 branch May 31, 2018 13:36

spring-projects-issues mentioned this pull request Dec 16, 2019

Streaming JSON reader [BATCH-2691] #917

Closed

Add a streaming Json item reader #607

Add a streaming Json item reader #607

Uh oh!

Conversation

fmbenhassine commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmbenhassine May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmbenhassine May 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mminella May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mminella commented May 31, 2018

Uh oh!

Uh oh!

fmbenhassine commented May 22, 2018 •

edited

Loading

fmbenhassine May 30, 2018 •

edited

Loading

fmbenhassine May 30, 2018 •

edited

Loading

mminella May 31, 2018 •

edited

Loading