[READY FOR MERGE] - implement full jmespath support in codegen for downstream AWS SDK v2 #523
Conversation
| } | ||
|
|
||
| private Result visit(JmespathExpression expr, Shape current) { | ||
| private Variable visit(JmespathExpression expr, Variable current) { | ||
| if (expr instanceof FunctionExpression tExpr) { |
There was a problem hiding this comment.
[nit] now that we have like 10 if-else it would really be nice to enable pattern matching https://www.baeldung.com/java-switch-pattern-matching
However I don't think we need to prioritize it, would just be cool
| private Variable visitFilterProjection(FilterProjectionExpression expr, Variable current) { | ||
| var unfiltered = visitProjection(new ProjectionExpression(expr.getLeft(), expr.getRight()), current); | ||
| if (!(unfiltered.shape instanceof CollectionShape unfilteredCol)) { | ||
| throw new CodegenException("projection did not create a list"); |
There was a problem hiding this comment.
Can we add expr to the Codegen exception so we know which projection failed to create a list?
| private Variable visitLiteral(LiteralExpression expr) { | ||
| var ident = nextIdent(); | ||
| if (expr.isNumberValue()) { | ||
| // FUTURE: recognize floating-point, for now we just use int |
There was a problem hiding this comment.
is it safe to leave this as future?
There was a problem hiding this comment.
Yes -- would fail at compile time in the result.
|
|
||
| // inner HAS to be a list by spec, otherwise something is wrong | ||
| if (!(inner.shape instanceof CollectionShape innerList)) { | ||
| throw new CodegenException("left side of projection did not create a list"); |
There was a problem hiding this comment.
Can we add the expression to the exception message that failed to create a list?
| ++idIndex; | ||
| var ident = nextIdent(); | ||
| // projections implicitly filter out nil evaluations of RHS | ||
| var needsDeref = isPointable(lookahead.type); |
There was a problem hiding this comment.
[nit] can we move this closer to where it's used?
...o-codegen/src/main/java/software/amazon/smithy/go/codegen/GoJmespathExpressionGenerator.java
Show resolved
Hide resolved
| for _, v := range v1 { | ||
| v3 := v.Key | ||
| if v3 != nil { | ||
| v2 = append(v2, *v3) |
There was a problem hiding this comment.
are these string guaranteed to be string pointers all the time?
There was a problem hiding this comment.
No -- the projection routine checks for that and generates the check accordingly (it's the needsDeref thing).
| } | ||
|
|
||
| @Test | ||
| public void testComparatorStringLHSNil() { |
There was a problem hiding this comment.
I don't follow how LHS is nil here here :(
There was a problem hiding this comment.
Shapes are nilable by default for cross-language compatibility - so since output.nested.nestedField is just a plain String shape, we generate it as nilable.
| v2 := input.Nested | ||
| v3 := v2.NestedField | ||
| var v4 bool | ||
| if v3 != nil { |
There was a problem hiding this comment.
[nit] is the extra space expected
There was a problem hiding this comment.
Yes -- side effect of the template being $L $L $L where some of the format args can be empty (in this case, only both operands being nilable will result in all three being set). gofmt gets rid of it.
|
|
||
| @Test | ||
| public void testNot() { | ||
| var expr = "objectList[?!(length(innerObjectList) > `0`)]"; |
There was a problem hiding this comment.
[nit] I'd prefer to use a value that is not 0, since this could hide any bug we may introduce later on by just using int defaults
There was a problem hiding this comment.
I changed one of them to 99. Note we do have a test in here somewhere that basically checks 0 < x <= 10, so it was covering the nondefault case.
I don't think there are any restrictions on the jmespath syntax used in waiters. Docs do not seem to convey any.
No -- there are still unhandled expressions. Those would be caught at codegen time. |
lucix-aws
force-pushed
the
feat-jmespath2
branch
from
4fe70f3
to
7031a52
Compare
lucix-aws
changed the title
| @@ -0,0 +1,763 @@ | |||
| /* | |||
| * Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
There was a problem hiding this comment.
[nit] The best practice is to just not include a year
| String waiterName | ||
| ) { | ||
| waiterName = StringUtils.capitalize(waiterName); | ||
| return String.format("%sWaiterOptions", waiterName); |
There was a problem hiding this comment.
[tiny nit] Java can be simpler and just do return waiterName + "WaiterOptions"
Variableis now the input and output of a traversed expression, as well as the result of each intermediate phase.This implementation was verified by re-generating the public SDKs and ensuring that they compiled.
This PR should not be merged as-is. It was submitted in this state such that the diff for the waiters integration was most easily read. We intend to do a trial rollout of the new waiter implementation downstream for the EC2 service. Before merging this PR we must do the following:Waiters2etc. implementationCloses #238