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

Metadata type changed from list to tuple affecting modification via interceptor #660

Closed
ohmayr opened this issue May 24, 2024 · 4 comments
Closed
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@ohmayr
Copy link
Contributor

ohmayr commented May 24, 2024

The following change d96eb5c introduces a breaking change by changing the type of metadata from a list to a tuple resulting in users to not be able to modify the metadata via an interceptor.

The following showcase tests in the generator break, since the interceptor can no longer modify the metadata (or assign it a new value) via this function:

def _add_metadata(self, client_call_details):
        if client_call_details.metadata is not None:
            client_call_details.metadata.append((self._key, self._value))
@ohmayr ohmayr added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels May 24, 2024
@ohmayr ohmayr assigned daniel-sanche, ohmayr and parthea and unassigned parthea May 24, 2024
@daniel-sanche
Copy link
Contributor

daniel-sanche commented May 24, 2024

So it looks like there are two ways to address this:

def _add_metadata(self, client_call_details):
        if client_call_details.metadata is not None:
            client_call_details.metadata = [(self._key, self._value), *client_call_details.metadata]

The type annotations for metadata seem to only guarantee a Sequence for metadata passed in, and doesn't explicitly say anything about what format the metadata will have when passed to the callable. IMO, I'd say that the bug is in the tests, which probably shouldn't be assuming a specific sequence type, and should assume metadata output will have the same structure as the input

But if we find other places are assuming a list and it's causing issues, we can always revert back to a list and change the API contract to be more specific here

@parthea
Copy link
Collaborator

parthea commented Jun 4, 2024

I'm going to revert #527 as it's blocking our release for supporting protobuf 5.x . Please open a new PR to restore the feature with the suggested fix from #660 (comment)

@parthea parthea added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Jun 4, 2024
@parthea
Copy link
Collaborator

parthea commented Jun 4, 2024

Bumping to P2 as #527 will be reverted in main

@parthea
Copy link
Collaborator

parthea commented Oct 22, 2024

Closing as obsolete as the issue no longer exists in the main branch

@parthea parthea closed this as completed Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

3 participants