gapic-generator-java
gapic-generator-java copied to clipboard
fix: NPE in parsing error status during HttpJson server streaming
Fix NPE in https://github.com/googleapis/java-spanner/issues/3640.
However, a separate type resolution error will still prevent successful error status parsing. This requires a more substantial change, as discussed in #2237 (comment)..
Thank you for opening a Pull Request! Before submitting your PR, please read our contributing guidelines.
There are a few things you can do to make sure it goes smoothly:
- [x] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
- [ ] Ensure the tests and linter pass
- [ ] Code coverage does not decrease (if any source code was changed)
- [ ] Appropriate docs were updated (if necessary)
@blakeli0, could you please review this? Thanks.
@baeminbo Thanks for reporting this issue and creating this PR! However, I would like to discuss the priority of this issue before we jumping to the solution. Please see my latest comment https://github.com/googleapis/java-spanner/issues/3640#issuecomment-2670018252. In terms of the current solution, I don't think it scales. Because it only fixes the issue for server streaming calls, while it could happen for any types of calls (unary, client streaming etc.).
Unary calls do not throw the NullPointerException because a similar fix to this PR has already been applied. HttpJsonDirectCallable sets TypeRegistry to the CallOption to avoid NPE.
It appears that httpjson doesn't support client-streaming and bidi-streaming.
- HttpJsonCallableFactory, which is responsible to create callables, doesn't provide methods for client and bidi streaming types.
- It appears that auto-generated stub codes explicitly marks client and bidi streaming as unsupported. For example, bidi-streaming call StreamingPull throws
UnsupportedOperationException. - The lack of supporting those streaming types is confirmed in gapic generator code, specifically within Model. This restriction is applied in HttpJsonServiceStubClassComposer that generates the REST stub codes.
Yes, you are right that unary callables already applied a similar fix, client streaming and bidi streaming are not supported in httpjson, they were poor examples.
What I'm trying to get to is to come up with a generic problem, that is the current implementation can not serialize a response that contains an Any message (I think even unary calls would have similar problems), because a TypeRegistry with all the possible Message types is required. Similar problems have been encountered by others. In the Spanner case, BatchWriteResponse contains a google.rpc.Status, which contains a list of Any messages that surfaced this problem. Even if we provide a default empty TypeRegistry, it still wouldn't work because we don't know an exhaustive list of possible types in this Any field.
As I mentioned in https://github.com/googleapis/java-spanner/issues/3640#issuecomment-2672410302, I think the best way to move forward is to create a dedicated issue for it, we can discuss potential solutions and priority there.