Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: NPE in parsing error status during HttpJson server streaming #3640

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

baeminbo
Copy link

@baeminbo baeminbo commented Feb 13, 2025

Fix NPE in googleapis/java-spanner#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:

  • 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)

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Feb 13, 2025
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: s Pull request size is small. labels Feb 14, 2025
@baeminbo
Copy link
Author

@blakeli0, could you please review this? Thanks.

@blakeli0
Copy link
Collaborator

@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 googleapis/java-spanner#3640 (comment).
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.).

@baeminbo
Copy link
Author

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.

  1. HttpJsonCallableFactory, which is responsible to create callables, doesn't provide methods for client and bidi streaming types.
  2. It appears that auto-generated stub codes explicitly marks client and bidi streaming as unsupported. For example, bidi-streaming call StreamingPull throws UnsupportedOperationException.
  3. 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.

@blakeli0
Copy link
Collaborator

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 googleapis/java-spanner#3640 (comment), I think the best way to move forward is to create a dedicated issue for it, we can discuss potential solutions and priority there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants