Skip to content

Conversation

ioanbsu
Copy link
Contributor

@ioanbsu ioanbsu commented Feb 3, 2023

Updating ServerInterceptors to support different marshallers for requests and responses.
See #9870 for more details.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 3, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ioanbsu / name: Ivan Bahdanau (56ba24f)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 3, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ioanbsu / name: Ivan Bahdanau (33bbb56)

…s.java to support different marshallers for Request and Response messages.
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect in the future we'd want to have null marshaller mean "don't replace that marshaller." But that can be added in the future because this NPE's now in that case.

That style of test isn't my favorite, but it is mirroring the existing test for this, so is fair.

@ejona86 ejona86 requested a review from YifeiZhuang February 3, 2023 16:33
@ejona86
Copy link
Member

ejona86 commented Feb 3, 2023

Note for posterity: the encryption case here is a bit awkward because reflection and other tooling won't know about the shenanigans. There's ways to work around some of that, but they'd introduce some of their own interesting behavior. So I think the encryption case here is "a successful hack that may work well in limited situations." But it isn't something I'd expect to be supported in other languages or to encourage widely. So I'm not condoning this use-case, really, but would rather this than abusing Compressor.

I'm okay with this API though because it does make sense to be able to change the marshaller for request separately from response. Especially in the case where you only want to replace one (which falls into the null case I mentioned in my review, for future enhancement).

But I also think this API should be extremely rarely used. useMarshalledMessages() was added as a convenience during implementation of useInputStreamMessages(), so useMarshalledMessages() may have been unused by users up to this point, and I think useInputStreamMessages() makes a lot more sense to use for things like caching responses, logging, and the like.

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 3, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 3, 2023
@ioanbsu ioanbsu changed the title Enable server e2e encryption by updating ServerInterceptors.java to support different marshallers for Request and Response messages. Updating ServerInterceptors.java to support different marshallers for Request and Response messages. Feb 3, 2023
@ioanbsu
Copy link
Contributor Author

ioanbsu commented Feb 10, 2023

@YifeiZhuang, friendly ping.

@ejona86 ejona86 merged commit 5beae3a into grpc:master Feb 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants