feat: port secure_context testing support to executor proxy#13522
feat: port secure_context testing support to executor proxy#13522aseering wants to merge 1 commit into
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. |
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.
|
|
||
| // Parameters for the SQL string. | ||
| repeated Parameter params = 2; | ||
| n // Secure context parameters. |
There was a problem hiding this comment.
There is a syntax error in the proto file. A stray 'n' character was introduced at the beginning of the line, which will cause proto compilation to fail.
| n // Secure context parameters. | |
| // Secure context parameters. |
| */ | ||
| @java.lang.Override | ||
| public java.util.List<com.google.spanner.executor.v1.QueryAction.Parameter> getParamsList() { | ||
| public java.util.Map<java.lang.String, com.google.spanner.executor.v1.Value> getSecureContextMap() { return java.util.Collections.emptyMap(); } |
There was a problem hiding this comment.
Please avoid manually editing auto-generated files. These files should be regenerated using the protobuf compiler. Additionally, manually stubbing getSecureContextMap() to return Collections.emptyMap() breaks the functionality because the actual secure context map sent in the request will be ignored and never passed to the Spanner client. Regenerating the files from the proto definition will automatically produce the correct getter and builder methods for the secure_context map field.
References
- Avoid manually editing auto-generated files (such as pom.xml in generated client libraries) because changes will be overwritten. Modify the generator or the source metadata instead.
| * | ||
| * <code>repeated .google.spanner.executor.v1.QueryAction.Parameter params = 2;</code> | ||
| */ | ||
| java.util.Map<java.lang.String, com.google.spanner.executor.v1.Value> getSecureContextMap(); |
There was a problem hiding this comment.
Please avoid manually editing auto-generated files. This interface should be regenerated automatically by the protobuf compiler after fixing the proto definition.
References
- Avoid manually editing auto-generated files (such as pom.xml in generated client libraries) because changes will be overwritten. Modify the generator or the source metadata instead.
| } 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"); |
35118d0 to
a2ae04a
Compare
a2ae04a to
afe27a7
Compare
afe27a7 to
02b781f
Compare
Add support for our executor proxy, so that tools can drive tests/etc using secure parameters