Skip to content

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
Contributor

@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
Contributor

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