Introduce `JsonIndexAccessor`

[sbrannen]

Spring Framework 6.2 introduces dedicated support for indexing into custom structures via a new IndexAccessor SPI, with a corresponding CompilableIndexAccessor SPI and a ReflectiveIndexAccessor implementation.

Spring Integration should consider introducing a JsonIndexAccessor as a companion to the existing JsonPropertyAccessor, potentially removing some (if not all) of the "indexing" related functionality from JsonPropertyAccessor, thereby allowing the JsonPropertyAccessor to focus on property access while the JsonIndexAccessor focuses on index access.

Documentation on the new features in Spring Framework can be found in the reference manual.

In addition, a proof-of-concept JacksonArrayNodeIndexAccessor can be found in Spring Framework's test suite:

https://github.com/spring-projects/spring-framework/blob/a9a1798012242facac264cc468a23d01155f3e26/spring-expression/src/test/java/org/springframework/expression/spel/IndexingTests.java#L813-L850

[sbrannen]

If you decide to implement a JsonIndexAccessor, I'd be happy to assist you in case you run into any issues.

And... it would be great if you could investigate the possibility before Spring Framework 6.2 GA is released, so that we have time to make any necessary changes before the SPIs and implementations in Spring Framework are set in stone.

[artembilan]

Hi @sbrannen !

Thank you for sharing this!

Looking into the code I'm not totally sure why to replace. Looks like that JacksonArrayNodeIndexAccessor deals only with an ArrayNode which is just a part of the logic of this JsonPropertyAccessor in its inner ArrayNodeAsList.

I will try to replace it blindly with the suggested JsonIndexAccessor, but I'm not sure if that is enough.

I'll let you know later today or next week. Looks like our SpelPropertyAccessorRegistrar should be fixed for the TargetedAccessor instead. Then its name is a bit misleading. So, deprecate that one as well in favor of SpelAccessorRegistrar 😄

[artembilan]

So, this a failure in my test for the <int:transformer expression="payload.firstName"/> when I remove JsonPropertyAccessor and just add JsonIndexAccessor:

Caused by: org.springframework.expression.spel.SpelEvaluationException: EL1008E: Property or field 'firstName' cannot be found on object of type 'com.fasterxml.jackson.databind.node.ObjectNode' - maybe not public or not valid?
    at org.springframework.expression.spel.ast.PropertyOrFieldReference.readProperty(PropertyOrFieldReference.java:229)
    at org.springframework.expression.spel.ast.PropertyOrFieldReference.getValueInternal(PropertyOrFieldReference.java:112)
    at org.springframework.expression.spel.ast.PropertyOrFieldReference$AccessorValueRef.getValue(PropertyOrFieldReference.java:376)
    at org.springframework.expression.spel.ast.CompoundExpression.getValueInternal(CompoundExpression.java:97)
    at org.springframework.expression.spel.ast.SpelNodeImpl.getTypedValue(SpelNodeImpl.java:121)
    at org.springframework.expression.spel.standard.SpelExpression.getValue(SpelExpression.java:376)
    at org.springframework.integration.util.AbstractExpressionEvaluator.evaluateExpression(AbstractExpressionEvaluator.java:190)

Which is definitely expected since an IndexAccessor has nothing to do with the regular property read.

I'll play next week with both of them.

[sbrannen]

Thank you for sharing this!

Sure thing! 👍

Looking into the code I'm not totally sure why to replace.

I apologize: I should not have chosen the wording "replace" but rather to augment.

Since applications likely depend on the current syntax / feature set, you'll want to keep the JsonPropertyAccessor in place and introduce an IndexAccessor that supports natural indexing syntax (and also avoids the need to convert strings to integers, etc.).

I'll rephrase the title and description to reflect that.

Looks like that JacksonArrayNodeIndexAccessor deals only with an ArrayNode which is just a part of the logic of this JsonPropertyAccessor in its inner ArrayNodeAsList.

That JacksonArrayNodeIndexAccessor is really just a proof of concept for basic Jackson JSON support. It's not intended to be a drop-in replacement for Spring Integration's Jackson index/property accessor support.

Looks like our SpelPropertyAccessorRegistrar should be fixed for the TargetedAccessor instead. Then its name is a bit misleading. So, deprecate that one as well in favor of SpelAccessorRegistrar 😄

Sounds like a plan.

[sbrannen]

So, this a failure in my test for the <int:transformer expression="payload.firstName"/> when I remove JsonPropertyAccessor and just add JsonIndexAccessor:

Which is definitely expected since an IndexAccessor has nothing to do with the regular property read.

Yep, that's to be expected.

That's why you'll want to keep the JsonPropertyAccessor around. Though, I think you should be able to move most (if not all) of the "index" accessing support from JsonPropertyAccessor to a new JsonIndexAccessor.

I'll play next week with both of them.

OK. Keep me posted!

[artembilan]

Hi Sam!

I'll try to make it short. I see some problem with having PropertyAccessor and IndexAccessor as separate abstractions for JSON. Essentially we always need both (I doubt that someone would opt-in only for one or another), but according to the current TargetedAccessor contract we cannot implement both PropertyAccessor & IndexAccessor o a single target class like JsonPropertyAccessor. Of course it might not been intended by design to combine them into a single implementation, but that's exactly what we have with JsonPropertyAccessor: simple property accessor to JSON props and index accesses to JSON arrays.

So, is there a strict requirement that we have to extract an IndexAccessor? What would be wrong if I have it internally and delegate from my JsonPropertyAccessor for indexes, respectively?

Thanks

[sbrannen]

Hi Artem,

I see some problem with having PropertyAccessor and IndexAccessor as separate abstractions for JSON. Essentially we always need both (I doubt that someone would opt-in only for one or another),

I agree that one would likely need both. So I suppose it depends on how they are registered. If users currently have to explicitly opt in for the existing support (for example, by manually registering the JsonPropertyAccessor with the evaluation context), then splitting the functionality 100% would be a breaking change for those users, and we would likely not want to do that.

The main reason I suggested that Spring Integration introduce an IndexAccessor is that the new API allows for explicit, first-class supporting for indexing into custom structures, such as JSON array nodes.

Disclaimer: I am not 100% certain what syntax is currently supported for accessing an index in a JSON array node, but I got the impression that it is only possible by specifying an integer index as a string (e.g., ['1'] instead of [1]). Thus, my assumption was that introducing a JsonIndexAccessor would allow users to use the standard [1] syntax.

but according to the current TargetedAccessor contract we cannot implement both PropertyAccessor & IndexAccessor o a single target class like JsonPropertyAccessor. Of course it might not been intended by design to combine them into a single implementation,

Indeed, it is not expected that a single class would implement PropertyAccessor and IndexAccessor. Although, I suppose it's technically possible.

As you have noticed, there would only be one effective implementation of TargetedAccessor.getSpecificTargetClasses() in such a scenario, and that would be a bit odd.

However, the implementations of PropertyAccessor.canRead() and IndexAccessor.canRead() can effectively override what's implied by getSpecificTargetClasses().

The result of getSpecificTargetClasses() is really only used to sort accessors (either property or index accessors). SpEL does not store PropertyAccessor and IndexAccessor instances in the same set/collection. See AstUtils.getAccessorsToTry() (and usages of that method) for details.

but that's exactly what we have with JsonPropertyAccessor: simple property accessor to JSON props and index accesses to JSON arrays.

Yes, I get that: it's been a one-stop shop for your JSON support in SpEL expressions.

And if it's too much hassle, or if there is insufficient benefit in having the property and index accessing support separate, then it may be better to leave things "as is".

Though, again, my assumption was two-fold, that separating the two would:

• provide a clear separation of concerns for properties and indexes
• allow users to use the idiomatic [1] indexing syntax (instead of ['1'])

So, is there a strict requirement that we have to extract an IndexAccessor?

No, there is no such requirement.

What would be wrong if I have it internally and delegate from my JsonPropertyAccessor for indexes, respectively?

I don't think it would be "wrong" per se, but I'd leave it up to you to decide if that makes maintenance/understanding of the code easier for you.

In summary, it may be too late in the game to completely change how JsonPropertyAccessor, but there may be benefit in supporting JSON array indexing idiomatically.

Please let me know if you have any more questions.

And... we can always chat offline, or I might try to spike something against Spring Integration in a branch to experiment a bit. Speaking of which, if you have a branch where you're experimenting, I'd be happy to take a look.

Cheers,

Sam

[sbrannen]

I spiked a proof of concept (POC) here: https://github.com/spring-projects/spring-integration/commit/c892b37db768a2889ffae297c613ef26e8ad324b

As stated in the commit message:

This is a "proof of concept" which introduces a JsonIndexAccessor as a complement to the existing JsonPropertyAccessor.

ArrayNode indexing support has been removed from JsonPropertyAccessor and now resides in JsonIndexAccessor.

The benefit of this set of changes is that the JsonPropertyAccessor now focuses on property access and JSON arrays can now be indexed via integer literals (e.g., [1]) instead of only via string literals representing integers (e.g., ['1']).

However, the JsonPropertyAccessor class still has its ArrayNodeAsList type. Removing it breaks existing tests, since it's still used for "wrapping" purposes in JsonPropertyAccessor.wrap(JsonNode). Since I'm not familiar with when/why the wrapping is performed, I figured it's best to leave that untouched for now.

In any case, we can use the POC for further discussions and experimentation.

[sbrannen]

As a side note, I also modernized JsonPropertyAccessor in https://github.com/spring-projects/spring-integration/commit/05e4eb788d01679336ffd97d23866802e29e64f1.

So perhaps you'd like to merge that into main anyway. Up to you! 😉

[artembilan]

Thanks, @sbrannen , for the crack!

Well, I had similar change in my local branch, but I had it removed since then: decided that it is not the right direction to have it like you in the test config:

context.addPropertyAccessor(new JsonPropertyAccessor());
context.addIndexAccessor(new JsonIndexAccessor());

As I said before: users are going to complain why they need to register both while they always would like to have both functionality via single flag.

Saying that I definitely can accept both of your contributions (I'm sorry that I removed my PoC branch 😢 ), but that would need some evolution to satisfy end-users.

From here a couple questions:

1. Does it matter in what order we register accessors? I feel like it does since this is our logic now in the IntegrationSimpleEvaluationContextFactoryBean:

       Collection<PropertyAccessor> accessors = getPropertyAccessors().values();
       PropertyAccessor[] accessorArray = accessors.toArray(new PropertyAccessor[accessors.size() + 2]);
       accessorArray[accessors.size()] = new MapAccessor();
       accessorArray[accessors.size() + 1] = DataBindingPropertyAccessor.forReadOnlyAccess();

   Why I'm asking? Because of the SpelPropertyAccessorRegistrar. I wonder if that should be changed to the public SpelPropertyAccessorRegistrar(TargetedAccessor... targetedAccessors) { contract and we deal with their specific type internally. Or we could do just a simple change and introduce setIndexAccessors(IndexAccessor... indexAccessor) instead. Which would be also a non-breaking change.

2. If we cannot combine both IndexAccessor and PropertyAccessor on one class impl (think about that JsonPropertyAccessor), then we might leave it as is with two classes as you have just implemented and document respectively. This way the mentioned SpelPropertyAccessorRegistrar would probably have a high-level setting like enableJsonAccessors(). So, we would register both those classes with nice simple flag.

Just thinking aloud if you'd like to contribute more.

Please, open PR with whatever you have so far, and we'll go from there.

Thank you!

[sbrannen]

Thanks, @sbrannen , for the crack!

You're welcome!

Well, I had similar change in my local branch, but I had it removed since then: decided that it is not the right direction to have it like you in the test config:

context.addPropertyAccessor(new JsonPropertyAccessor());
context.addIndexAccessor(new JsonIndexAccessor());

In terms of the actual, physical registration of index and property accessors, that's unavoidable. StandardEvaluationContext and SimpleEvaluationContext both maintain separate internal lists for IndexAccessor and PropertyAccessor, and that's by design.

As I said before: users are going to complain why they need to register both while they always would like to have both functionality via single flag.

Yes, I understand that.

And, more importantly, we need to ensure that applications configured with the status quo will continue to work as-is without modifying their configuration.

1. Does it matter in what order we register accessors?

Yes, registration order is important. SpEL applies a first-one-wins strategy for selecting the accessor to use (both for properties and indexes).

• AstUtils.getAccessorsToTry(Class<?>, List<T>) sorts accessors according to their getSpecificTargetClasses().
• Indexer and PropertyOrFieldReference iterate over the sorted accessors, asking which one "can read" the index/property.

Why I'm asking? Because of the SpelPropertyAccessorRegistrar. I wonder if that should be changed to the public SpelPropertyAccessorRegistrar(TargetedAccessor... targetedAccessors) { contract and we deal with their specific type internally.

I suppose that would be one option.

Or we could do just a simple change and introduce setIndexAccessors(IndexAccessor... indexAccessor) instead. Which would be also a non-breaking change.

That might be a better option.

2. If we cannot combine both IndexAccessor and PropertyAccessor on one class impl (think about that JsonPropertyAccessor),

I think we should try avoid implementing both APIs in a single class.

then we might leave it as is with two classes as you have just implemented and document respectively. This way the mentioned SpelPropertyAccessorRegistrar would probably have a high-level setting like enableJsonAccessors(). So, we would register both those classes with nice simple flag.

enableJsonAccessors() sounds much more elegant to me and more future-proof.

Just thinking aloud if you'd like to contribute more.

Thanks for sharing your thoughts and concerns. 👍

Please, open PR with whatever you have so far, and we'll go from there.

OK.

then we might leave it as is with two classes as you have just implemented and document respectively.

Coming back to that, I'm leaning toward leaving JsonPropertyAccessor as-is, in order to avoid any possible negative side effects (unexpected breaking changes) and introducing JsonIndexAccessor with a strict focus on indexing, which complements JsonPropertyAccessor.

Having both registered should not be problematic, because SpEL's Indexer applies a custom IndexAccessor before a PropertyAccessor that's being using to index into a property of an object. Specifically, Indexer.getValueRef(ExpressionState, AccessMode) returns an IndexAccessorValueRef before it considers returning a PropertyAccessorValueRef.

[sbrannen]

Please, open PR with whatever you have so far, and we'll go from there.

I have another branch that I'm working on locally, and I'll submit that as a PR once I'm done.

[sbrannen]

I have another branch that I'm working on locally, and I'll submit that as a PR once I'm done.

You can find my current work here: https://github.com/spring-projects/spring-integration/compare/main...sbrannen:spring-integration:issues/gh-9383-spel-JsonIndexAccessor-v2

I'll likely finalize that in the coming days and submit a PR based on that, but feel free to take a look in the interim.

[artembilan]

Looks good, Sam!

Ship as PR, please!