test: Add test proxy implementation for ExecuteQuery api #2360
Conversation
product-auto-label
bot
added
size: l
jackdingilian
force-pushed
the
test-proxy-rebase
branch
from
7c9ea5c to
ec94ea6
Compare
Change-Id: I39f81dcf098b93eff79973648c57515e8908fde3
Change-Id: I53122fdf0301fb41d4f24881dc8de65d174dee35
jackdingilian
force-pushed
the
test-proxy-rebase
branch
from
ec94ea6 to
52832b2
Compare
jackdingilian
changed the title
| // CBT backend with the right options, however, the proxy itself is insecure | ||
| // so it is not recommended to use it with real credentials or outside testing | ||
| // contexts. | ||
| SecurityOptions security_options = 8; |
There was a problem hiding this comment.
Can these changes be in a separate PR? able to connect to production also sounds scary. Is there a way to avoid that?
There was a problem hiding this comment.
Talked to Patrick and removed this for now. We do want to use this for some internal tests, so we will put it in a separate PR
| import java.util.concurrent.ExecutionException; | ||
| import org.threeten.bp.Instant; | ||
|
|
||
| public class ResultSetSerializer { |
There was a problem hiding this comment.
Are there existing client code for serialization and deserialization? This is a lot of code to maintain and test proxy should just be a thin layer that calls into the client
There was a problem hiding this comment.
There is client code (within ResultSet) that handles deserializing protobuf Value messages to the corresponding java types. I agree this feels too 'heavy' for the testproxy but I think with the introduction of types it is somewhat unavoidable.
We want to validate that each client is converting to the relevant types correctly, to do so we need to do some form of roundtripping back to a shared format we can validate against. We reuse Value from the api here. I think no matter how we do it we need to do some conversion from java type back to some protobuf format.
There was a problem hiding this comment.
For existing java type -> Value serialization theres some existing code that does a subset of what we're doing here but it's not sufficient to cover what we need and Im not sure either is a great fit for replacing this.
-
https://github.com/googleapis/java-bigtable/blob/main/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/Value.java#L112 - This serializes some Values to proto but is only used for the types supported by aggregates right now. We would have no use for the additional SQL types there and we would need to have the aggregates apis reject those value types (https://github.com/googleapis/java-bigtable/blob/main/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/RowMutation.java#L231)
Statement does this for the types of query params we support but it doesn't support all the types we need (no map or struct) and sets the Value.type field which we don't expect here (though I guess it would be ok)
Change-Id: Ibe2fb899f83744d4e38debc58379e9942350a83f
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.