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

Bugfix/13 delete zone record illegal argument exception #30

Merged
merged 8 commits into from
May 26, 2020

Conversation

ggalmazor
Copy link
Contributor

@ggalmazor ggalmazor commented May 13, 2020

FIxes #13

This PR handles a case where the client fails to delete a zone record due to being unable to parse the API's response.

After studying this unexpected behavior, we discovered that other operations leading to HTTP 204 No Content responses will also fail with the same error.

We have discarded a problem in the API responses and have established that the way we mock HTTP responses in our tests is making it difficult to reproduce the issue reported in #13.

We were able to reproduce the issue in a test by artificially modifying a response and were able to confirm that this PR is a fix for the issue.

The fix comes from completely skipping the parsing step if we get an HTTP 204, which we know it's going to be (it should be) empty.

@ggalmazor ggalmazor self-assigned this May 13, 2020
@ggalmazor ggalmazor added the bug label May 13, 2020
@ggalmazor ggalmazor force-pushed the bugfix/13_deleteZoneRecord_IllegalArgumentException branch from c71a2d5 to 3e78c6f Compare May 13, 2020 11:44
@ggalmazor
Copy link
Contributor Author

ggalmazor commented May 13, 2020

After discussing this issue in deep with @weppos, we've decided to either find a way to add a regression test that doesn't involve tampering with the fixtures or releasing the fix without tests.

I've updated the PR description to document what we've learned so far.

@ggalmazor ggalmazor force-pushed the bugfix/13_deleteZoneRecord_IllegalArgumentException branch 2 times, most recently from b17febc to 80a354a Compare May 13, 2020 14:07
@ggalmazor
Copy link
Contributor Author

In 80a354a I have prepared an integration test that puts in motion the full HTTP stack without requiring a live request to the API, and leverages the fixtures without tampering with them.

This won't produce false positive as the one we fixed in #32 while maintaining the run times under control.

@ggalmazor
Copy link
Contributor Author

@weppos, I've separated the migration to end-to-end tests using intact fixtures to another PR #35

We can review and merge this one, which only contains the fix for the bug in how we deal with 204s.

@ggalmazor ggalmazor requested review from duduribeiro and san983 and removed request for duduribeiro May 22, 2020 18:36
@weppos
Copy link
Member

weppos commented May 25, 2020

It may be good to add an entry in the CHANGELOG
https://github.com/dnsimple/dnsimple-java/blob/master/CHANGELOG.md

@ggalmazor ggalmazor force-pushed the bugfix/13_deleteZoneRecord_IllegalArgumentException branch from 05d5dc8 to ba3e98b Compare May 25, 2020 13:55
@ggalmazor
Copy link
Contributor Author

I will hold merging this one for today to give @san983 some time to review it.

Copy link
Member

@san983 san983 left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@ggalmazor ggalmazor merged commit 8eff5a1 into master May 26, 2020
@ggalmazor ggalmazor deleted the bugfix/13_deleteZoneRecord_IllegalArgumentException branch May 26, 2020 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deleteZoneRecord throws exception even when call to API is successful
3 participants