This repository has been archived by the owner on Sep 26, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: REST Gapic (REGAPIC) Support #1177
feat: REST Gapic (REGAPIC) Support #1177
Changes from 7 commits
85bb35d
d6c2829
55b9f21
3c4c19d
aee533a
e02e368
7c08c24
d264bc6
3a982ff
a71e5d8
07b20ba
7f72f2b
0c0328b
5f257e6
71474be
b231f0f
d3b13f9
79c41a1
0dc2d89
169fcf2
2619f63
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
triple nested generics strongly suggests that there's a more detailed object type should be created for this. I.e. avoid FieldsExtractor of Map of List.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that three nested generics is too much, but unfortunately I'm forced to do it here. This field is used solely to implement the
Map<String, List<String>> getQueryParamNames(MessageFormatT apiMessage);
method of this class, which, in its turn, is declared in the interface implemented by this class (HttpRequestFormatter<MessageFormatT>
). I.e. this triple generic thing was predetermined by the existing architecture which I really don't want to mess with unless it is absolutely necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a brief comment to this effect, so that future well-meaning readers won't try to "fix" this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am increasingly less and less happy with GAX and GSON. Their mistakes are cascading down the dependency tree. :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why a method called getQueryParamNames is returning a map? Names would seem to be a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is implementing an existing interface. As I understand it, key is a name of a parameter, List is its values (to support array query parameters):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, and maybe I'll find the answer below: when does a client need to serialize a response message into a (JSON) string? Isn't it always paring JSON into messages? Or are we making this available purely for use by servers?
(it might be nice to note the answer in a comment in the interface)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same thing. I don't have a good answer for you =).
Note, this class implements an interface (
HttpResponseParser
), so I'm basically forced here to implement it. At the same time I can't find any prod usages of this method. It is used only once in a test, I'm not sure why. In other words, the safest and simplest thing was just to implement this method properly, especially taking into account that it takes only one line of code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Maybe a brief comment inside the function to the effect that we're not using this in prod but this is needed by the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is serializing to and also parsing from JSON, right? Maybe you could rename it so it doesn't sound like it's just serializing. How about
RequestConverterRestProto
? Similarly forHttpResponseParser
, which both parses and serializes. Maybe that should be calledResponseConverterRestProto
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class translates to/from text format and the message. It is not json only (also queryParameters for example). I would not call it conversion, because it does it for data to be transmitted over the network and reconstructed on the other machine (which is technically the definition of serialization, which is a more specific term than conversion).
So it technically does both serialization and deserialization. I called it just
*Serializer
similarly tojava.io.Serializable
interface, which marks classes that are expected to be both serialized and deserialized (otherwise serialization would not make sense). I could call itSerializerDeserializer
, but that looks horrible. I could also split it on two classes, probably (one serializer and deserializer), but it looks as an overkill to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think the comment helps a lot. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this error message and the one below, will printing
e
help the user isolate where the error comes from (specific message/field)? If not, maybe we should add here some helpful info...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by printing. It will throw an exception, preserving the original error as "cause" argument (
e
). How that exception is handled and if/how it will be shown in the log depends on many other factors (it is technically beyond the scope here, we can't trace all possible try/catch constructs this method may be executed under, if any). It is a typical and idiomatic way of doing it (throw a reasonable exception, letting the outer code to deal with it).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so all the info is contained in
e
and callers have the option of dealing with that. Sounds good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RuntimeException is almost certainly wrong here. better to bubble the InvalidProtocolBufferException or just decare it in throws as an IOException if you don't want to expose the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing it out! When I was prototyping it I needed the checked exception to be silenced out and did the laziest thing possible - just wrapped it in a raw
RuntimeException
and forgot to clean that out.Note,
IOException
is a checked exception andInvalidProtocolBufferException
extends it and thus is checked too.Instead of declaring IOExceptions in throws I declared a proper unchecked
ProtoRestSerializationException
. Note, these methods are explicitly called from the generated code and are part of the long chain of interactions for every single rpc call. Propagating the checked exception to the generated code and then to the long chain of various calls would be problematic and in general we do not use checked exceptions on gax surface.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requires character set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same story. Please, never do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with a newly declared
ProtoRestSerializationException extends RuntimeExcdeption
(same as above) to stay consistent with the rest of the exceptions in gax-java and to avoid the the hassle of propagating/catching checked exceptions in the generated code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking this
if
is harmless but unnecessary. If the condition is true, the path segment will wind up being filled with the default value even if theif
isn't here. It might be less confusing to remove it. And maybe add a comment to the effect of "while we prioritize field presence in what we send over the wire, parameters that appear in the path are ALWAYS sent over the wire."There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(but I realize I'm not clear on where these
put*
methods are invoked)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want this
if
here, since as per §A2 in the default values doc, we need to prioritize field presence. So even if the query param the user set matches the default value, it should be included. You do this for body below, and that's correct. You don't do this for path params above, but IIUC, it won't matter because it'll get filled in either way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(but I realize I'm not clear on where these
put*
methods are invoked)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I added this because it was literally breaking the execution of the generated clients. I guess it is especially relevant for strings, which have empty strings as default values, and this leads to query params being something like
?path_param=&path_param2=
, which made either GCE server or the other parts of gax unhappy (or something along those lines, I don't remember specifics).We will get back to default parameters either way. To match default values doc, plust we will have to revisit it if/when we move to using field presence feature of proto3. Please lets keep this as is for now, because it was added to fix specific failures and I have the whole toolchain tested with this present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great context, could you please add a comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable, but let's flag this more prominently. Could you add below your existing comment something like
TODO: Revisit this approach to ensure proper default-value handling as per design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: Curious whether gax-java is still on Java 7, otherwise we could use a forEach here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GAX is still on Java 7, as are all GCP libraries. See go/nojava7