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 14 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.
HTTP response
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.
Corrected
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.
If the proto comes from external sources, as they usually do, this should be a checked exception.
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.
Unfortunately I can't make it checked:
There is a long-standing controversy around checked vs unchecked exceptions (for example: https://www.ibm.com/developerworks/library/j-jtp05254/index.html). This does not prove that checked exceptions are wrong, but their application is not something that community agrees upon unanimously.
gax-java architecture does not foresee usage of checked exceptions on its surface. If any exception happens during a call on any of the stages, sooner or later it will be wrapped to an unchecked exception, most likely one of the defined in gax itself. I can't change it without rewriting in a backward-incompatible way the whole gax's exception handling logic. This means that if I don't wrap a checked
IOException
into an unchecked one here, it will be wrapped somewhere up the stack before a user can see it, plus going through the trouble of propagating the checked aspect of it explicitly to that level.For example, check ApiExceptionFactory - all of the created exceptions are unchecked.
Many of those exceptions mean that the particular call in which they were thrown must be terminated and cannot be completed successfully (i.e. there is no reasonable way to recover from the exception except starting the whole call over again). This is in alignment with Oracle's recommendation (https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html): "If a client can reasonably be expected to recover from an exception, make it a checked exception. If a client cannot do anything to recover from the exception, make it an unchecked exception". If the
ProtoRestSerializationException
is thrown the only thing a user can do is to fix something in their call and try it over again from the beginning.On practice, for this particular case, an attempt to keep the exceptions checked hits the wall of troubles almost immediately. Specifically
ProtoRestSerializer#toJson()
is called fromProtoMessageResponseParser#parse()
, which, in its turn implements HttpResponseParser#parse() interface method. To propagate it further, we will have to addthrows IOException
to the interface method, which is a breaking change, or wrap it in an unchecked on theHttpResponseParser#parse()
level. This is where it begins, if we propagate further if will get more and more complicated until it gets wrapped anyways or gax-java exception handling model is revisited as a whole.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.
HttpResponseParser#parse() does not document that it throws any exceptions. That strongly suggests it doesn't want any to be thrown, not that it thinks throwing a runtime exception is OK. You might need to return a default value here or null instead of throwing an exception.
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.
Yes, the interface does not document the thrown exceptions, but they are definitely possible by the existing implementaiton of this interface ApiMessageHttpResponseParser#l94, that calls
Gson#fromJson
, which throws two exceptions:JsonIOException, JsonSyntaxException
, both unchecked (JsonIOException
is also unchecked, even though it is named like it extendsIOException
, while it does not). Gson (google's libraryh) implementation seems following the no-checked exceptions philosophy, same as gax.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.
Swallowing the error information by returning null in case of an exception seems worse than throwing an unchecked exception, plus the existing implementation already throws them. WDYT about just updating the interface method documentation?
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 renamed
ProtoRestSerializationException
toRestSerializationException
(to make it less proto-specific), added it (still as an unchecked exception) to the documentation of theApiMessageHttpResponseParse
and in other relevant classes.Note,
ApiMessageHttpResponseParse
had broken documentation block, so stricktly speaking it never had documentation, but rather had a multi-line comment (/* */
was used instead of/** */
).