feat: port secure_context testing support to executor proxy#13522
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces secure context parameters to query actions in the Spanner executor, adding a secure_context map to the QueryAction proto and implementing helper methods in CloudClientExecutor to apply these options. The review feedback highlights a critical syntax error in the proto file (a stray 'n' character), warns against manually editing auto-generated Java files which should instead be regenerated via the protobuf compiler, and suggests correcting a typo in an exception message.
| } else if (entry.getValue().getValueTypeCase() == com.google.spanner.executor.v1.Value.ValueTypeCase.STRING_VALUE) { | ||
| valueBuilder.setStringValue(entry.getValue().getStringValue()); | ||
| } else { | ||
| throw new IllegalArgumentException("Unsupported secure parameter value type in GitHub proxy"); |
There was a problem hiding this comment.
There is a typo in the exception message: 'GitHub proxy' should be 'executor proxy' to match the context of this executor proxy tool.
| throw new IllegalArgumentException("Unsupported secure parameter value type in GitHub proxy"); | |
| throw new IllegalArgumentException("Unsupported secure parameter value type in executor proxy"); |
There was a problem hiding this comment.
This text is already used elsewhere. I think we want to keep the code in sync. If a reviewer with broader context thinks this is worthwhile, please ping here and I'll make the change in both locations.
35118d0 to
a2ae04a
Compare
afe27a7 to
02b781f
Compare
02b781f to
7819aba
Compare
d2d441e to
6f32743
Compare
f35ddce to
aa8400b
Compare
| } | ||
| sender.initForQuery(); | ||
| List<Options.QueryOption> queryOptions = new ArrayList<>(); | ||
| queryOptions.add(Options.tag("dml-transaction-tag")); |
There was a problem hiding this comment.
nit: this is also translated to a request_tag, not a transaction_tag. (But in this case, I can see that it is an existing flaw in the code, so feel free to leave as-is)
aa8400b to
3627eae
Compare
…e empty serviceKeyFile
3627eae to
4bb6869
Compare
9b3c989 to
a5b1829
Compare
2 participants
Add support for our executor proxy, so that tools can drive tests/etc using secure parameters