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

Allow performing requests for URLs that do not contain a host #223

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

kmcbride
Copy link
Collaborator

Certain URL types supported by NSURLSession are valid even though they do not contain a host property. Previously, the data loader would not even attempt to request these URLs. With this change it is possible to load local files as well as data URIs.

@gitrema
Copy link
Contributor

gitrema commented Aug 23, 2022

SPTDataLoader was written with the intention to be a network library and more specific to handle HTTP requests and being implemented on top of URLSession to reduce its binary footprint. Allowing requests that are not meant to be HTTP to go through, it means to expand the surface in which the library operates so I would have expected to see an improvement in error reporting instead of allowing different types of URLs to go through.

@kmcbride
Copy link
Collaborator Author

kmcbride commented Aug 23, 2022

@gitrema fair point, though as far as I can tell nowhere in the library has it ever been enforced that requests or responses be limited to HTTP(S) — if that is the intention then I'm surprised by the lack of code doing anything to prevent otherwise. It seems to me that the use case addressed in this PR was originally supported and is now broken (probably unintentionally) because of a change to nullability handling: host resolution introductionresolved host nil checknullability refactor

@gitrema
Copy link
Contributor

gitrema commented Aug 24, 2022

Thanks for pointing out the history by sharing previous PRs. I cannot find any mention in the README about usage of SPTDataLoader for something else other than networking. It does not mean that cannot be used to load other type of URLs of course even though it might not have been the intended use case. With that said if this PR is meant as bug-fix of a previously allowed behaviour it makes sense to merge it even though I would have personally preferred to take the opportunity to restrict requests to network protocols only.

@kmcbride
Copy link
Collaborator Author

I will go ahead and merge this to restore the functionality. If it's decided that request scheme restrictions should be enforced, it can be done further up the call stack (eg: here).

@kmcbride kmcbride merged commit 44e3b86 into spotify:master Aug 24, 2022
@kmcbride kmcbride deleted the hostless-request branch August 24, 2022 18:44
@kmcbride
Copy link
Collaborator Author

@gitrema I just noticed that this portion of the readme specifically calls out support for protocols beyond HTTP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants