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: fix conditional retry handling of camelCase query params #340

Merged
merged 1 commit into from
Dec 11, 2020

Conversation

andrewsg
Copy link
Contributor

A customer reported difficulty with if_metageneration_match conditional retries. It turned out to be confusion in the retry code over snake case vs. camel case query params; this will resolve that issue.

@andrewsg andrewsg requested a review from a team December 10, 2020 22:12
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 10, 2020
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label Dec 10, 2020
@andrewsg andrewsg merged commit 4ff6141 into master Dec 11, 2020
@andrewsg andrewsg deleted the retry-fix branch December 11, 2020 00:18
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Belated comment on this-- does this seem to indicate that we're missing an integration test case?

Also @frankyn are these parameter keys case-sensitive?

@andrewsg
Copy link
Contributor Author

@tritone Yes, I thought the same thing. We discussed integration coverage for this area in the initial commit, but found that there was really no good way to test the retry logic without mocking server responses in our systems tests. Rather than do that, which would be a change in policy on mocks in system tests, we decided to wait for automated conformance testing using the test bench which is supposedly coming in the future.

I'm open to revisiting that decision; this bug was nasty to track down and would have been easy with good integration tests here. What do you think?

If I recall correctly (I haven't tested this on GCS, this is my experience from other libraries) the API is flexible on how it accepts parameters, and both camelCase and snake_case are accepted, hence my confusion here. I don't know whether case is enforced on camel case; I'd assume it is not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/python-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants