Make JsonPropertyAccessor returned type directly a JsonNode or value #3270

pilak · 2020-05-04T07:00:48Z

According to that thread on stackoverflow, I'm submitting a PR in order to improve the selector readability on JsonPropertyAccessor.

I made this changes because I noticed that using the same expression passing from different accessors such as MapAccessor, ReflectivePropertyAccessor is working but not with JsonNodePropertyAccessor.
Here's an example that shows the breaking or the success of using or not the new JsonNodePropertyAccessor along with the same SpEL expression:
https://github.com/pilak/examples/blob/master/TestJsonAccessing.java

artembilan · 2020-05-04T15:51:28Z

Thank you for contribution @pilak !

We will take a look into your PR soon.

Meanwhile, please, consider to run gradlew clean check task against your changes: looks like there are a lot of Checkstyle violations.

First of all we use tabs for indents. Secondly our imports order is like:

<module name="ImportOrder">
	<property name="groups" value="java,/^javax?\./,org,org.springframework,*"/>
	<property name="ordered" value="true"/>
	<property name="separated" value="true"/>
	<property name="option" value="top"/>
	<property name="sortStaticImportsAlphabetically" value="true"/>
</module>

pilak · 2020-05-05T10:49:44Z

Hi Artem,

I made changes related to checkstyle violations and I think it's ok now.
I don't know if everything is well integrated, especially for the bean context integration part.

Hope my solution will fit.
Regards

artembilan

Sorry, I don't understand what is going on.

Json property accessor selector friendly

This is not enough to justify such a breaking change.

Can you give in the commit message as much explanation what you are doing and why as possible?

See recommendations here: https://chris.beams.io/posts/git-commit/

artembilan · 2020-05-05T16:39:58Z

...ntegration-core/src/main/java/org/springframework/integration/json/JsonPropertyAccessor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2013-2019 the original author or authors.
+ * Copyright 2014-2020 the original author or authors.


Why have you moved the first year and why haven't you changed all others to 2020 in the last year?
Anyway it looks like you didn't run gradlew clean check because we have a Gradle task to update Copyright for all the changed classes...

unfortunately I have an error while running the task check

Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Unable to find: src/checkstyle/checkstyle-suppressions.xml

Can you show, please, how do you call it?
Is it from command line like ./gradlew clean check ?
Or do you use an IDE integration?

from the wrapper yes in command line

The compilation was not working on JDK 11, so I used JDK 8 instead

pilak · 2020-05-06T17:05:55Z

Sorry, I don't understand what is going on.

Json property accessor selector friendly

This is not enough to justify such a breaking change.

Can you give in the commit message as much explanation what you are doing and why as possible?

See recommendations here: https://chris.beams.io/posts/git-commit/

Sorry, I don't know exactly what to set for the commit title...
Is the title below good for you ?

JsonPropertyAccessor now renders a JsonNode or the value if at leaf

artembilan · 2020-05-06T17:12:24Z

Is the title below good for you ?

Well, the header of the commit message should not exceed 50 symbols.
So, with any effort it is hard to explain everything in such a short sentence.
Your current title is OK.
What I worry about that there is no explanation what is going on in the change.

And here are some questions from me since I review this:

Why would one rename a bean and class making so big breaking change?
Why do you change a logic in side that JsonPropertyAccessor so radically?
Why do you use a proxy over there?

All those are hard changes for me and I can't merge whatever I don't understand.
With some explanation in the commit message that would be great improvement in our collaboration over this PR.

(Did you really read that Chris Beam's article?)

Thanks

artembilan · 2020-05-06T19:56:14Z

@pilak ,

please, take a look here: f047f05

I think this one has to fix your problem with Checkstyle + Gradle.

artembilan · 2020-05-06T19:57:10Z

In general you need to pull upstream/master and rebase your json-property-accessor-selector-friendly branch to get those changes immediately.

pilak · 2020-05-09T11:31:17Z

@artembilan why do we return "null" stringified in the link below? Does a null not enough?
dea81e3#diff-4c3528130fa3a7cdcb0a2c05f990cd57L209
On the other hand, when the JsonPropertyAccessor reads the json object, and then creates a ToStringFriendlyJsonNode, it does check if it is null before and thus returns a TypedValue.NULL if it is. So is that really necessary to override the toString method in ToStringFriendlyJsonNode? I don't get it.

I also found that actually, the Jackson ArrayNode should implement the List interface or else Collection (don't know why jackson didn't do it, but there must be a reason). But because of this not, we must wrap() the ArrayNode before evaluation otherwise accessing with index cannot be possible. To me this should not be the responsability of the caller.
As a workaround we could add the method wrap(@Nullable Object target) which defaultly returns the target, to the PropertyAccessor interface and then call this in the Ast Indexer with if SUPPORTED_CLASSES then wrap() then getValueRef()

Reading the code again I now understand that this JsonPropertyAccessor was primarily designed for the MessageConverter purpose. But does it mean we could forget other usages? On different projects on which I work, we use the SpEL expression evaluation as a tool appart, exactly the way I use it in the stackoverflow thread, by declaring all thing manually, and the fact is that everything works and is commutable, until we use the JsonNode object, so that we had to convert it in other Objects.

Anyway, I will push an update of my changes soon, according to your different reviews, and eventually what could be decided from this post.

Thanks for your patience
Regards

pilak · 2020-05-11T21:15:21Z

@artembilan I pushed some changes and modified the commit message to explain better what I intent to solve with my changes in this PR.
I also changed the first message on that thread
#3270 (comment)

Hope this is not too much time consuming for you, and will worth it.
Regards

artembilan · 2020-05-12T18:49:03Z

Thank you, @pilak , for such an effort!
I see now your point, but I'll review it a bit later this or next week.
We have GA release tomorrow and looks like your change is slightly a breaking (or behavior) change. We can't apply it in GA phase.
The next version should be OK.

Stay tuned!

pilak · 2020-05-13T07:36:29Z

Ok @artembilan thanks for the feedback.
No problem for the wait, I have all my time. And if the changes are breaking, it is better to have a long-term test phase.
Wish you good luck!

artembilan · 2020-05-19T19:15:26Z

Please, rebase your branch to the latest master. It is now 5.4.x.
We will review your effort shortly after that.
Thanks

pilak · 2020-05-26T09:15:33Z

Hi @artembilan,
I rebased from Master and while building on local I have many errros such as:

org.springframework.amqp.AmqpConnectException: java.net.ConnectException: Connexion refusée (Connection refused)

I also have this error from Travis CI:

FluxAggregatorMessageHandlerTests > testWindowTimespan() FAILED java.lang.IllegalStateException at FluxAggregatorMessageHandlerTests.java:205

artembilan · 2020-05-26T15:28:38Z

Thank you for the update!

Pulling locally for thorough review...

artembilan · 2020-05-26T15:29:49Z

org.springframework.amqp.AmqpConnectException: java.net.ConnectException:

I'll turn off my RabbitMQ to see what is wrong in the tests.
Those AMQP tests really have to be covered by the @RabbitAvailable condition...

artembilan · 2020-05-26T18:13:13Z

It looks like we indeed need a Comparable for JsonNode and an ArrayNodeAsList, but not proxy.
It seems for me a JsonNode has all the methods we need need for SpEL collection selection.
Although we need to revise that anyway because it looks like has() and get() are over there in the JsonNode. Probably we don't need to worry about a List wrapping anyway. Plus JsonNode is an Iterable by itself.

I worry that proxying is a heavy operation and we should avoid it as much as possible.

Would you mind to play with much simpler model when we deal only with a JsonNode?
It feels for me that Jackson has made some effort into its core to simplify our life...

Thanks for understanding!

artembilan · 2020-05-26T17:48:35Z

...ntegration-core/src/main/java/org/springframework/integration/json/JsonPropertyAccessor.java

-	private ContainerNode<?> assertContainerNode(JsonNode json) throws AccessException {
-		if (json instanceof ContainerNode) {
-			return (ContainerNode<?>) json;
+	private JsonNode assertContainerNode(JsonNode json) throws AccessException {


Doesn't look like we need such an assertion at all.

if (index != null && container.has(index)) { return typedValue(container.get(index)); }

So, we are good to deal with whatever JsonNode we get.
Looks like the previous Jackson version was different, but now it is a bit easier to avoid extra checks.

...ntegration-core/src/main/java/org/springframework/integration/json/JsonPropertyAccessor.java

artembilan · 2020-05-26T17:58:31Z

...ntegration-core/src/main/java/org/springframework/integration/json/JsonPropertyAccessor.java

-	 */
-	public static class ArrayNodeAsList extends AbstractList<WrappedJsonNode<?>>
-			implements WrappedJsonNode<ArrayNode> {
+	public static class ArrayNodeAsList extends AbstractList<Object> implements JsonNodeWrapper {


I wouldn't make all these support classes as public to avoid confuse on the end-user side when they get access to them.

artembilan · 2020-05-26T20:34:08Z

org.springframework.amqp.AmqpConnectException: java.net.ConnectException:

Please, rebase your branch to the latest master. The fix should be there for this.

Although consider to test only spring-integration-core module: you don't do any changes which may effect other modules.

pilak · 2020-05-29T09:26:37Z

It seems for me a JsonNode has all the methods we need need for SpEL collection selection.
Although we need to revise that anyway because it looks like has() and get() are over there in the JsonNode. Probably we don't need to worry about a List wrapping anyway. Plus JsonNode is an Iterable by itself.

Unfortunately the method below called in AST doesn't check for Iterable but instead Collection... That's why I guess we used to wrap with ArrayNodeAsList at start. Or maybe the change could be done in that method?
org.springframework.expression.spel.ast.Indexer.getValueRef(ExpressionState)

pilak · 2020-05-29T09:51:34Z

It looks like we indeed need a Comparable for JsonNode and an ArrayNodeAsList, but not proxy.

As JsonNode is an asbtract meta class (in a Jackson way), this is not possible to extend it without having to implement every abstract methods in order to add Comparable contract. So to me, proxy is the only way :/
Or we would have to check every implemention types and create an extended class for each...

artembilan

Thank you for the update, @pilak !

I don't think this can make it into the current 5.4 GA phase.
So, probably already to the next one 6.0.

I'll review it a bit later, next week after release, but meanwhile consider these issue Travis shows to us:

/home/travis/build/spring-projects/spring-integration/spring-integration-core/src/main/java/org/springframework/integration/json/JsonPropertyAccessor.java:76: error: cannot find symbol

				return Set.of(new ConvertiblePair(JsonNodeWrapper.class, JsonNode.class));

				          ^

  symbol:   method of(ConvertiblePair)

  location: interface Set

/home/travis/build/spring-projects/spring-integration/spring-integration-core/src/main/java/org/springframework/integration/json/JsonPropertyAccessor.java:123: warning: [cast] redundant cast to JsonNode

			return assertContainerNode((JsonNode) wrapper.getRealNode());

			                           ^

1 error

1 warning

Plus see if you rebased to the latest master.

pilak

this is not possible to extend it without having to implement every abstract methods in order to add Comparable contract.

OK. Can we deal with the TreeNode instead for that Comparable contract? I'm not sure that we indeed should let to have all the JsonNode functionality access.
Plus, we probably just could convert an ArrayNode into a List:
 StreamSupport.stream(arrayNode.spliterator(), false)
    .collect(Collectors.toList());

Using this we won't be able to call expression.getValue(..., ArrayNode.class)

pilak · 2020-10-22T10:12:57Z

...ntegration-core/src/main/java/org/springframework/integration/json/JsonPropertyAccessor.java

-	private ContainerNode<?> assertContainerNode(JsonNode json) throws AccessException {
-		if (json instanceof ContainerNode) {
-			return (ContainerNode<?>) json;
+	private JsonNode assertContainerNode(JsonNode json) throws AccessException {


artembilan

I think it is very close now to the finish.

Would you mind to answer to those my couple questions?
Or make a respective change...

Thanks

artembilan · 2020-10-26T19:23:23Z

...ntegration-core/src/main/java/org/springframework/integration/json/JsonPropertyAccessor.java

+		converterRegistry.addConverter(new GenericConverter() {
+			@Override
+			public Set<ConvertiblePair> getConvertibleTypes() {
+				return Collections.singleton(new ConvertiblePair(JsonNodeWrapper.class, JsonNode.class));


Why do we need this converter?

If that, shouldn't we introduce it alongside with that ToStringFriendlyJsonNodeToStringConverter?
On the other hand: do we still need that ToStringFriendlyJsonNodeToStringConverter with this whole your change?

Good point, as the Wrapper already integrate the toString method, it is no more necessary to have such a converter...

I removed the ToString converter, and add some more test with this in mind.

I take it the application context will load it automatically in case of a Spring application is starting...? If so, now the wrapper converter is not an anonymous class anymore.

artembilan · 2020-10-29T17:20:23Z

Please, stop squashing commits.
I'm not sure now what change have you done since the last review.

pilak

Please, stop squashing commits.
I'm not sure now what change have you done since the last review.

Ok sorry I will try to restore the history, and make it build better

...ation-core/src/test/java/org/springframework/integration/json/JsonPropertyAccessorTests.java

pilak · 2020-10-29T16:31:31Z

...ntegration-core/src/main/java/org/springframework/integration/json/JsonPropertyAccessor.java

+		converterRegistry.addConverter(new GenericConverter() {
+			@Override
+			public Set<ConvertiblePair> getConvertibleTypes() {
+				return Collections.singleton(new ConvertiblePair(JsonNodeWrapper.class, JsonNode.class));


Good point, as the Wrapper already integrate the toString method, it is no more necessary to have such a converter...

I removed the ToString converter, and add some more test with this in mind.

I take it the application context will load it automatically in case of a Spring application is starting...? If so, now the wrapper converter is not an anonymous class anymore.

artembilan

I think we are good to go with your PR here so far, but now it only can make it into the next 6.0 version since we have just done with 5.4 and such a change cannot make it into the point release since it is slightly breaking change.

Thanks for understanding!

artembilan · 2020-10-30T14:01:08Z

.travis.yml

    - ./gradlew checkAsciidocLinks check --refresh-dependencies --no-daemon
+    - kill %1


Are those logs in the end of build on Travis is what we would expect from this change:

Done. Your build exited with 0. /home/travis/.travis/functions: line 536: 5866 Terminated while sleep 540; do echo "=====[ ${SECONDS} seconds still running ]====="; done

?

Especially I worry about that non-resolved ${SECONDS} placeholder...

${SECONDS} is a built-in shell variable that start counting as you start/open a command in a terminal

I made this change because of the result of this build
https://travis-ci.org/github/spring-projects/spring-integration/builds/740138931

I guess this log is only the output of the kill %1

in this line you have an example of the working routine:
https://travis-ci.org/github/spring-projects/spring-integration/builds/740153889#L826

...ation-core/src/test/java/org/springframework/integration/json/JsonPropertyAccessorTests.java

* add `Comparable` contract to the initial wrapper so that expressions containing selection/projection with JsonNode filtering are now possible * render directly the value of a ValueNode. SpEL parser can now evaluate correctly the object value when the expression deals with filtering on JsonNode values (<, >, ==...) * the 2 above changes make this possible: `property.^[name.getTarget().asText() == 'value1'].name` can now be written as simply as `property.^[name == 'value1'].name`. The expression is now fully compatible with other PropertyAccessor types * rename `ToStringFriendlyJsonNode` to `JsonNodeWrapper` as it reflects more the capability of the new wrapper * add a GenericConverter to be able to convert the `JsonNodeWrapper` class into a `JsonNode` (or its derivative classes) directly while calling `expression.getValue(..., JsonNode.class)` * add the possibility to access list items in a json-path like index => a negative number will begin from the end of the list <= * return null value when JsonNode cannot handle the property name (or index)

artembilan · 2021-01-08T16:00:59Z

Since we have decided to go with 5.5 version yet for the upcoming Spring Boot 2.5, then pulling this change locally for the final review and clean up... I think we can apply such a change into the minor release with an appropriate Migration Guide note (if any).

pilak

Since we have decided to go with 5.5 version yet for the upcoming Spring Boot 2.5, then pulling this change locally for the final review and clean up... I think we can apply such a change into the minor release with an appropriate Migration Guide note (if any).

Hello @artembilan
Am I supposed to do something for now?

pilak · 2020-10-30T18:26:16Z

.travis.yml

    - ./gradlew checkAsciidocLinks check --refresh-dependencies --no-daemon
+    - kill %1


${SECONDS} is a built-in shell variable that start counting as you start/open a command in a terminal

...ation-core/src/test/java/org/springframework/integration/json/JsonPropertyAccessorTests.java

pilak · 2020-10-30T18:32:27Z

.travis.yml

    - ./gradlew checkAsciidocLinks check --refresh-dependencies --no-daemon
+    - kill %1


I made this change because of the result of this build
https://travis-ci.org/github/spring-projects/spring-integration/builds/740138931

pilak · 2020-10-30T19:10:55Z

.travis.yml

    - ./gradlew checkAsciidocLinks check --refresh-dependencies --no-daemon
+    - kill %1


I guess this log is only the output of the kill %1

in this line you have an example of the working routine:
https://travis-ci.org/github/spring-projects/spring-integration/builds/740153889#L826

artembilan · 2021-01-14T19:34:47Z

Hi @pilak !
Thank you for reminder!

We've busy with releases this week.
Now we are good to wrap up issues for ucomping 5.5 M1 next week.
So, I'm pulling your PR locally for the final review ASAP.

Probably you don't need to do anything more, unless you'd like to contribute some other stuff 😄

Anyway I'll update this later.

artembilan · 2021-01-14T21:09:16Z

Merged as b58b0e5.

@pilak ,

thank you very much for the contribution; looking forward for more!

pilak · 2021-01-15T09:02:21Z

Nice, thank you @artembilan for your advices
I will probably make some little changes then if this PR spring-projects/spring-framework#26323 fits

artembilan · 2021-01-15T16:52:46Z

Sure! Looking forward for decision on that PR from Spring Framework commiters!

pilak force-pushed the json-property-accessor-selector-friendly branch 2 times, most recently from 7060332 to dea81e3 Compare May 4, 2020 21:25

pilak marked this pull request as ready for review May 5, 2020 10:55

pilak changed the title ~~WIP: Json property accessor selector friendly~~ Json property accessor selector friendly May 5, 2020

artembilan requested changes May 5, 2020

View reviewed changes

pilak force-pushed the json-property-accessor-selector-friendly branch from dea81e3 to a6ad719 Compare May 11, 2020 20:23

pilak changed the title ~~Json property accessor selector friendly~~ Make JsonPropertyAccessor returned type directly a JsonNode or value May 11, 2020

pilak requested a review from artembilan May 12, 2020 07:58

pilak force-pushed the json-property-accessor-selector-friendly branch from a6ad719 to 4b9b9bd Compare May 12, 2020 17:23

artembilan added this to the 5.4.x milestone May 12, 2020

pilak force-pushed the json-property-accessor-selector-friendly branch from 4b9b9bd to e059536 Compare May 20, 2020 19:30

artembilan requested changes May 26, 2020

View reviewed changes

artembilan requested changes Oct 22, 2020

View reviewed changes

pilak force-pushed the json-property-accessor-selector-friendly branch 2 times, most recently from 1309e53 to 4e7bfda Compare October 23, 2020 22:58

pilak commented Oct 24, 2020

View reviewed changes

pilak marked this pull request as ready for review October 24, 2020 14:00

pilak requested a review from artembilan October 24, 2020 14:40

pilak force-pushed the json-property-accessor-selector-friendly branch from 4e7bfda to 0c6efab Compare October 24, 2020 14:51

artembilan requested changes Oct 26, 2020

View reviewed changes

pilak force-pushed the json-property-accessor-selector-friendly branch from 0c6efab to ac1ff8d Compare October 29, 2020 16:33

pilak requested a review from artembilan October 29, 2020 16:43

pilak commented Oct 30, 2020

View reviewed changes

pilak force-pushed the json-property-accessor-selector-friendly branch 2 times, most recently from c93c3f8 to e3633b5 Compare October 30, 2020 10:33

artembilan requested changes Oct 30, 2020

View reviewed changes

pilak force-pushed the json-property-accessor-selector-friendly branch from e3633b5 to 2a7621f Compare December 10, 2020 22:54

Pierre Lakreb added 2 commits December 28, 2020 15:04

Change Json Converter

5efd452

pilak force-pushed the json-property-accessor-selector-friendly branch from 2a7621f to 5efd452 Compare December 28, 2020 14:05

artembilan modified the milestones: Backlog, 5.5 M1 Jan 8, 2021

artembilan added in: core type: enhancement labels Jan 8, 2021

pilak commented Jan 14, 2021

View reviewed changes

artembilan closed this Jan 14, 2021

		- ./gradlew checkAsciidocLinks check --refresh-dependencies --no-daemon
		- kill %1

Make JsonPropertyAccessor returned type directly a JsonNode or value #3270

Make JsonPropertyAccessor returned type directly a JsonNode or value #3270

Conversation

pilak commented May 4, 2020 • edited Loading

artembilan commented May 4, 2020

pilak commented May 5, 2020

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pilak May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pilak commented May 6, 2020

artembilan commented May 6, 2020

artembilan commented May 6, 2020

artembilan commented May 6, 2020

pilak commented May 9, 2020 • edited Loading

pilak commented May 11, 2020 • edited Loading

artembilan commented May 12, 2020

pilak commented May 13, 2020

artembilan commented May 19, 2020

pilak commented May 26, 2020 • edited Loading

artembilan commented May 26, 2020

artembilan commented May 26, 2020

artembilan commented May 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan commented May 26, 2020

pilak commented May 29, 2020 • edited Loading

pilak commented May 29, 2020 • edited Loading

artembilan left a comment

Choose a reason for hiding this comment

pilak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan commented Oct 29, 2020

pilak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan commented Jan 8, 2021

pilak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan commented Jan 14, 2021

artembilan commented Jan 14, 2021

pilak commented Jan 15, 2021 • edited Loading

artembilan commented Jan 15, 2021

pilak commented May 4, 2020 •

edited

Loading

pilak May 6, 2020 •

edited

Loading

pilak commented May 9, 2020 •

edited

Loading

pilak commented May 11, 2020 •

edited

Loading

pilak commented May 26, 2020 •

edited

Loading

pilak commented May 29, 2020 •

edited

Loading

pilak commented May 29, 2020 •

edited

Loading

pilak left a comment •

edited

Loading

pilak commented Jan 15, 2021 •

edited

Loading