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
Merged

Conversation

@andrewsg
Copy link
Contributor

@andrewsg andrewsg commented Dec 10, 2020

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 Dec 10, 2020
@google-cla google-cla bot added the cla: yes label Dec 10, 2020
@andrewsg andrewsg merged commit 4ff6141 into master Dec 11, 2020
4 checks passed
@andrewsg andrewsg deleted the retry-fix branch Dec 11, 2020
Copy link
Contributor

@tritone tritone left a comment

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

@andrewsg andrewsg commented Dec 11, 2020

@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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants