fix: Sanitize Java keywords in generated client #911
Conversation
|
See the failure on pulpogato/pulpogato#841 for example |
rahulsom
requested review from
asibross,
iparadiso,
iuliiasobolevska,
jjacobs44,
kilink,
kzwang,
paulbakker and
srinivasankavitha
as code owners
| """ | ||
| |InputArgument ${input.name}Arg = new InputArgument("${input.name}", ${input.name}, false, null); | ||
| |getInputArguments().get("${it.name}").add(${input.name}Arg); | ||
| |InputArgument ${javaReservedKeywordSanitizer.sanitize(input.name)}Arg = new InputArgument( |
There was a problem hiding this comment.
nit: consider extracting into a local var and reusing in all 3 places in the modified block
| |InputArgument ${input.name}Arg = new InputArgument("${input.name}", ${input.name}Reference, true, ${getVariableDefinitionType( | ||
| |InputArgument ${javaReservedKeywordSanitizer.sanitize( | ||
| input.name, | ||
| )}Arg = new InputArgument("${input.name}", ${input.name}Reference, true, ${getVariableDefinitionType( |
There was a problem hiding this comment.
shall it be sanitized it here as well (${input.name}Reference)?
| | ${javaReservedKeywordSanitizer.sanitize(input.name)}, | ||
| | false, | ||
| | null); | ||
| |getInputArguments().get("${fieldDefinition.name}").add(${input.name}Arg); |
There was a problem hiding this comment.
same question: why is it sanitized on line 603 but not here?
| |InputArgument ${input.name}Arg = new InputArgument("${input.name}", ${input.name}Reference, true, ${getVariableDefinitionType( | ||
| |InputArgument ${javaReservedKeywordSanitizer.sanitize( | ||
| input.name, | ||
| )}Arg = new InputArgument("${input.name}", ${input.name}Reference, true, ${getVariableDefinitionType( |
There was a problem hiding this comment.
I think *Reference param name should be sanitized as well? I'm not sure if this would also be a problem and perhaps we're missing a test case, or this param doesn't need to be sanitized?
Edit: Looks like @iuliiasobolevska beat me to it here. 😄
rahulsom
force-pushed
the
handle-java-keywords
branch
2 times, most recently
from
7745ac2 to
f19b2ec
Compare
|
Thanks @iuliiasobolevska & @sjohnr for the review! Changes are in. |
| .addCode( | ||
| """ | ||
| |this.variableReferences.put("${javaReservedKeywordSanitizer.sanitize(inputValue.name)}", variableRef); | ||
| |this.variableReferences.put("$sanitizedInputName", variableRef); |
There was a problem hiding this comment.
My understanding might be off but would we want to instead keep the unsanitized input name here to match the GraphQL input so the query is serialized correctly?
There was a problem hiding this comment.
Yep. That makes sense. Updated that. Thanks!
Prior to this change, the generated code would have the right name for the parameter but wrong name in the creation of InputArgument.
```java
InputArgument packageArg = new InputArgument("package", package, false, null);
```
Now we have a test for generated code being compilable, and we handle this case correctly.
rahulsom
force-pushed
the
handle-java-keywords
branch
from
f19b2ec to
a1c7ba8
Compare
4 participants
Prior to this change, the generated code would have the right name for the parameter but wrong name in the creation of InputArgument.
Now we have a test for generated code being compilable, and we handle this case correctly.