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 URLRequest not always being available publicly #1792

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@rastersize
Contributor

rastersize commented Nov 18, 2016

This PR aims to resolve #1785 by first trying to return the URLRequest from the task. If it’s nil we’ll attempt to return URLRequest associated with the originalTask, if there is one. As per the #1785 (comment).

I’ve also made Error.underlyingAdaptError public so that request re-triers can check any underlying adapt errors. I opted to not make the error type public as it might add confusion of what type of error RequestAdapters should throw.

Hopefully this is in line with Alamofire’s style and with what you where thinking @cnoon. Let me know if you want me to make any changes.

🧀🍷

rastersize added some commits Nov 18, 2016

Change underlyingAdaptError to be public
- Makes it possible to inspect errors sent to the request re-trier. To
  for example check if they’re from the request re-trier.
- The actual error type is still internal. This is in hopes to reduce
  confusion as to what type of error should be thrown by adapters. I.e.
  not an AdaptError, but rather a custom error the re-trier can handle.
Fix URLRequest not always returned when available
- In certain cases the `request` property would return `nil` even
  though the `Request` had a `URLRequest` object. This change makes
  sure we return any `URLRequest` object we might have.
- Adds some tests to verify that catches the original problem (#1785).
- Resolves #1785.
@rastersize

This comment has been minimized.

Show comment
Hide comment
@rastersize

rastersize Nov 19, 2016

Contributor

The tests seems to have failed due to some test flakiness and Xcode/simulator flakiness. Unfortunately I can’t re-trigger them. Ran the tests locally for all platforms (except watchOS) and they all passed ¯_(ツ)_/¯

Contributor

rastersize commented Nov 19, 2016

The tests seems to have failed due to some test flakiness and Xcode/simulator flakiness. Unfortunately I can’t re-trigger them. Ran the tests locally for all platforms (except watchOS) and they all passed ¯_(ツ)_/¯

@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Nov 20, 2016

Member

Okay @rastersize, thanks for making the time to put this together!

I squashed your changes, made a few minor tweaks and pushed them into master in commit 19e5012. I also removed your change to make the AdaptError public. That error should definitely not need to be public which led me to investigate some more as to why you thought you needed to do that.

Once I dug in more, I realized that throwing an Error in the adapter wasn't working quite right. The retrier was being called correctly, but was being sent the AdaptError instead of the underlying error like it was supposed to. The idea is that if you throw an AuthenticationError.expiredAccessToken in your adapter, then the retrier error will also be an AuthenticationError.expiredAccessToken. You should NEVER see an AdaptError in any of the public Alamofire APIs. If you do, it is a bug. I've fixed the underlying issue and added tests to ensure everything is working properly in d2826a1. This should resolve the need for you to ever need the AdaptError to be public.

If there's still something I'm missing here, please don't hesitate to reach out. This logic is brand new and it's possible there's still a kink or two. I "think" at this point the logic should be pretty solid. Regardless, if you see anything that seems a bit odd, just assume it is and please file an issue. On a different note, these changes should be released within the next few days.

Thanks again! 🍻

Member

cnoon commented Nov 20, 2016

Okay @rastersize, thanks for making the time to put this together!

I squashed your changes, made a few minor tweaks and pushed them into master in commit 19e5012. I also removed your change to make the AdaptError public. That error should definitely not need to be public which led me to investigate some more as to why you thought you needed to do that.

Once I dug in more, I realized that throwing an Error in the adapter wasn't working quite right. The retrier was being called correctly, but was being sent the AdaptError instead of the underlying error like it was supposed to. The idea is that if you throw an AuthenticationError.expiredAccessToken in your adapter, then the retrier error will also be an AuthenticationError.expiredAccessToken. You should NEVER see an AdaptError in any of the public Alamofire APIs. If you do, it is a bug. I've fixed the underlying issue and added tests to ensure everything is working properly in d2826a1. This should resolve the need for you to ever need the AdaptError to be public.

If there's still something I'm missing here, please don't hesitate to reach out. This logic is brand new and it's possible there's still a kink or two. I "think" at this point the logic should be pretty solid. Regardless, if you see anything that seems a bit odd, just assume it is and please file an issue. On a different note, these changes should be released within the next few days.

Thanks again! 🍻

@cnoon cnoon closed this Nov 20, 2016

@cnoon cnoon added this to the 4.2.0 milestone Nov 20, 2016

@cnoon cnoon self-assigned this Nov 20, 2016

@rastersize

This comment has been minimized.

Show comment
Hide comment
@rastersize

rastersize Nov 20, 2016

Contributor

@cnoon Sounds great and what I expected from the API to begin with. Good change 👍

Contributor

rastersize commented Nov 20, 2016

@cnoon Sounds great and what I expected from the API to begin with. Good change 👍

@rastersize rastersize deleted the rastersize:fix-missing-urlrequest-when-re-trying branch Nov 21, 2016

@rastersize rastersize restored the rastersize:fix-missing-urlrequest-when-re-trying branch Nov 21, 2016

@rastersize rastersize deleted the rastersize:fix-missing-urlrequest-when-re-trying branch Nov 21, 2016

@rastersize rastersize restored the rastersize:fix-missing-urlrequest-when-re-trying branch Nov 21, 2016

@rastersize rastersize deleted the rastersize:fix-missing-urlrequest-when-re-trying branch Nov 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment