Conversation
| # echoes any metadata with key 'showcase-trailer', so the same metadata | ||
| # should appear as trailing metadata in the response. | ||
| interceptor = EchoMetadataClientGrpcInterceptor( | ||
| "showcase-trailer", |
There was a problem hiding this comment.
Why don't we want to echo the trailing metadata like we were doing before?
There was a problem hiding this comment.
See a572ff8 where I've updated the comments to clarify that we're still expecting an echo response with showcase-trailer in tests tests/system/test_grpc_interceptor_streams.py and tests/system/test_streams.py. The metadata with showcase-trailer is being sent as part of the request, and the showcase server will echo this back.
There was a problem hiding this comment.
But the purpose of the test in test_grpc_interceptor_streams.py, which uses this fixture, is to test the interceptor. So the test was having the interceptor insert some new metadata, and then testing that the echoed response back from the server had that new metadata. By removing the injection in the interceptor, and sending it as part of the original request, we're no longer exercising the interceptor (we're simply checking that Showcase echoes back the metadata, which was an assumption for this original test). Am I missing something?
There was a problem hiding this comment.
But the purpose of the test in test_grpc_interceptor_streams.py, which uses this fixture, is to test the interceptor.
The interceptor is still being tested. It is reading the request and response metadata.
So the test was having the interceptor insert some new metadata, and then testing that the echoed response back from the server had that new metadata.
The code in the test was incorrect. This is not the correct way to send metadata. See the grpc examples here which show how metadata should be sent:
https://github.com/grpc/grpc/blob/master/examples/python/metadata/metadata_client.py
By removing the injection in the interceptor, and sending it as part of the original request, we're no longer exercising the interceptor (we're simply checking that Showcase echoes back the metadata, which was an assumption for this original test). Am I missing something?
The interceptor is still being exercised. interceptor.response_metadata is being used in tests.
There was a problem hiding this comment.
The interceptor is still being exercised.
interceptor.response_metadatais being used in tests.
In test_grpc_interceptor_streams.py, I only see interceptor.response_metadata in the LHS of two assignments:
interceptor.response_metadata = response_metadata. Where is it being checked rather than assigned?
It seems we should make that into an assertion.
Somewhat separately, it seems to me that part of the intent of the previous test was to modify the metadata, not just read it, before sending the request. That's what I was thinking about in my previous comment. There's probably still value in doing that, but if we can't do that immediately, I think a TODO linking to a GitHub issue to come back to this would be fine.
There was a problem hiding this comment.
There is no requirement to modify metadata via interceptors as far as I'm aware. As per the grpc example I linked previously, metadata should be sent along with the request, and there is already a metadata argument which can be populated on the client side.
There was a problem hiding this comment.
Oh, the use cases for the interceptors didn't include modifing the metadata? We'd have to look back to previous docs. That's what the test was aiming to do previously, but I don't know if that was to test that functionality specifically or merely as a way of testing that the interceptor was called. If changing the metadata is allowed, it might be nice to test it/exemplify it, but we don't have to do that right now; we can defer it.
There was a problem hiding this comment.
I was able to restore the original test code that we had which updated metadata using an interceptor. PTAL
This is based off of #2441 to include asserts on the interceptor.
This is based off of #2441 to include asserts on the interceptor.
| def _read_response_metadata_stream(self): | ||
| # Access the metadata via the original stream object | ||
| if hasattr(self, "_original_stream"): | ||
| return self._original_stream.trailing_metadata() |
There was a problem hiding this comment.
It might make sense to assign to self.response_metadata before returning.
This fixes the presubmit failures at HEAD which were caused by a breaking change in gRPC 1.75.0. The breaking change was considered a bug fix because https://grpc.github.io/grpc/python/grpc_asyncio.html#grpc-metadata mentions that a map is expected rather than a list.
Prior to
grpc==1.75.0,client_call_details.metadata.append((key, value))worked for both sync and async. Withgrpc==1.75.0,client_call_details.metadata.append((self._key, self._value))only works for sync. For async, we needclient_call_details.metadata[self._key] = self._valueThis type of breaking change is challenging because we have test code that runs against both new and old versions of gRPC to check compatibility. Here is the error that we get with older gRPC code after making the necessary change to support gRPC 1.75.0:
We need to have special logic to handle this breaking change in 1.75.0.