-
Notifications
You must be signed in to change notification settings - Fork 239
JMESPath evaluation #2878
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
base: main
Are you sure you want to change the base?
JMESPath evaluation #2878
Conversation
# Conflicts: # VERSION
|
This pull request does not contain a staged changelog entry. To create one, use the Make sure that the description is appropriate for a changelog entry and that the proper feature type is used. See |
|
This pull request does not contain a staged changelog entry. To create one, use the Make sure that the description is appropriate for a changelog entry and that the proper feature type is used. See |
smithy-model/src/main/java/software/amazon/smithy/model/node/NodeJmespathRuntime.java
Outdated
Show resolved
Hide resolved
smithy-model/src/main/java/software/amazon/smithy/model/node/NodeJmespathRuntime.java
Outdated
Show resolved
Hide resolved
smithy-model/src/main/java/software/amazon/smithy/model/node/NodeJmespathRuntime.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public Node element(Node array, Node index) { | ||
| return array.expectArrayNode().get(index.expectNumberNode().getValue().intValue()).orElseGet(this::createNull); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty rough for array indexing. I think we should add a method to ArrayNode to get a value at an index or return null if not found to remove indirection and allocations here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in a getOrNull method that returns a possibly-null Node instead of an Optional<Node> as get does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up adding an elementAt method that should always return a non-null value (since the elements of an ArrayNode should use NullNode rather than null) and throws if the index is out of range. That works better here since the evaluator checks the range already for you. Let me know if that's not what you had in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just add a method that return the value or null. It's more general purpose and covers your use case too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it means an extra range check in my case but I'm not concerned about that. Operations that will access lots of array elements will use asIterable instead anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, but do you want to return null if the array is actually storing a NullNode at the index, and not just if the index is out of range? That's less ideal here since I actually want the NullNode, but I could work with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking to return a regular Java null, basically just a pass through to Array.get, even throwing IndexOutOfBoundsException if out of range. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup that's what I had the first time. :) Agreed on just throwing for out of range. But AFAICT an ArrayNode should only ever have non-null elements and use NullNode instead. e.g. this check in withValue. So that means passing through to List.get should never return a Java null. I think that's the right call though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline and I'm going to go with returning a NullNode if the element is a NullNode or if the index is OOB. Will pull the Node changes into the next PR along with smithy-model-jmespath though.
smithy-model/src/main/java/software/amazon/smithy/model/node/NodeJmespathRuntime.java
Outdated
Show resolved
Hide resolved
smithy-jmespath/src/main/java/software/amazon/smithy/jmespath/evaluation/Evaluator.java
Outdated
Show resolved
Hide resolved
smithy-jmespath/src/main/java/software/amazon/smithy/jmespath/functions/FunctionRegistry.java
Outdated
Show resolved
Hide resolved
smithy-jmespath/src/main/java/software/amazon/smithy/jmespath/functions/FunctionRegistry.java
Outdated
Show resolved
Hide resolved
smithy-model/build.gradle.kts
Outdated
| extra["moduleName"] = "software.amazon.smithy.model" | ||
|
|
||
| dependencies { | ||
| api(project(":smithy-jmespath")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the artifact size hit to smithy-model to unconditionally pull in jmespath. Do we need Node-based evaluation? If so, can it be a bridge package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'd rather move this out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I plan to leverage the Node-based evaluation in the next PR, which will add a new trait that uses JMESPath and wants to validate that positive/negative examples are actually correct.
I've moved the NodeJmespathRuntime class and tests to a separate smithy-model-jmespath package now though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this PR can we leave out smithy-model-jmespath too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I could move that to the next PR that will actually use it, that makes sense.
smithy-jmespath/src/main/java/software/amazon/smithy/jmespath/evaluation/Evaluator.java
Show resolved
Hide resolved
Co-authored-by: Michael Dowling <michael@mtdowling.com>
…hy into smithy-jmespath-evaluator
Background
JmespathExpressionsJmespathRuntime<T>interface to provide the necessary basic operations on JSON values at runtime.Evaluatoris largely ported over fromsmithy-java'sJMESPathDocumentQuery, but also taking some inspiration fromjmespath-java, which also has a generic interface for adapting any existing JSON value representation.JmespathRuntime<T>implementations for bothLiteralExpressionandNode.smithy-jmespath-testsmodule with the compliance tests from https://github.com/jmespath/jmespath.test, and instantiates them for both runtime implementations.smithy-javaevaluator only works on its ownDocumenttype, and evaluating expressions on generated types such asSerializableStructsubtypes currently requires first converting to aDocument, which is slow. I've also written up implementations ofJmespathRuntimeforDocumentand generated types: seesmithy-javalink below.@shapeExamplestrait (Add new shapeExamples trait that communicates and enforces allowed and disallowed values #2851)Testing
smithy-jmespath-testsmodule.Links
Note for reviewers: I'm not fully satisfied with how I've arranged the code into packages. I want to minimize how much API is exposed publicly, but didn't want to add all the new types into the base package. At the same time I don't want to expose the executable version of the
Functioninterface publicly yet - I want to support custom functions in the future, but I'm not yet sure the current definition is optimal for that.evaluationpackage and made them package-private. This can be revisited in a future change to support custom functions.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.