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

Remove URL unescaping for web URL sources. #338

Merged
merged 1 commit into from Jan 12, 2018

Conversation

Projects
None yet
2 participants
@mojombo
Contributor

mojombo commented Jan 8, 2018

When providing a web URL for the sound asset, if that URL contains
percent escape sequences, an NSOSStatusErrorDomain error is thrown.
This is because stringByRemovingPercentEncoding was being called
on the URL before being sent to URLWithString. As per documentation:

This method expects URLString to contain only characters that are
allowed in a properly formed URL. All other characters must be
properly percent escaped. Any percent-escaped characters are
interpreted using UTF-8 encoding.

This commit removes the call to stringByRemovingPercentEncoding,
allowing escaped characters to pass through as required.

Remove URL unescaping for web URL sources.
When providing a web URL for the sound asset, if that URL contains
percent escape sequences, an NSOSStatusErrorDomain error is thrown.
This is because stringByRemovingPercentEncoding was being called
on the URL before being sent to URLWithString. As per documentation:

> This method expects URLString to contain only characters that are
> allowed in a properly formed URL. All other characters must be
> properly percent escaped. Any percent-escaped characters are
> interpreted using UTF-8 encoding.

This commit removes the call to stringByRemovingPercentEncoding,
allowing escaped characters to pass through as required.
@trepidity

This comment has been minimized.

Show comment
Hide comment
@trepidity

trepidity Jan 8, 2018

Collaborator

Looking at this bit of code, I don't like the whole section. Why are we removing the encoding? Really, why is an encoded URL being passed in the first place?

I like your change, but really I think it should remove the whole "if" section.

Thoughts?

Collaborator

trepidity commented Jan 8, 2018

Looking at this bit of code, I don't like the whole section. Why are we removing the encoding? Really, why is an encoded URL being passed in the first place?

I like your change, but really I think it should remove the whole "if" section.

Thoughts?

@trepidity

This comment has been minimized.

Show comment
Hide comment
@trepidity

trepidity Jan 8, 2018

Collaborator

duplicate of #328

Collaborator

trepidity commented Jan 8, 2018

duplicate of #328

@mojombo

This comment has been minimized.

Show comment
Hide comment
@mojombo

mojombo Jan 9, 2018

Contributor

@trepidity Thanks for the quick response.

Looking at this bit of code, I don't like the whole section. Why are we removing the encoding?

Don't like the whole section how? My understanding of the if block is that depending on the source, it needs to pull the data differently. The only problem is that (for web URLs) it's trying to decode the passed in URL back to UTF-8, which will cause the URLWithString call to fail if the originally provided source URL had any percent-encoded characters in it.

Really, why is an encoded URL being passed in the first place?

Well, the library doesn't say how it wants the URL, but as a user I'd certainly expect that if I send a percent-encoded URL, that it would be ok with that. Perhaps it could also accept unencoded UTF-8 URL strings and percent-encode them, to be extra friendly to the user, but the one thing it CANNOT do is call stringByRemovingPercentEncoding.

I like your change, but really I think it should remove the whole "if" section.

Again, I'm not sure what you mean by this? Remove which parts?

Contributor

mojombo commented Jan 9, 2018

@trepidity Thanks for the quick response.

Looking at this bit of code, I don't like the whole section. Why are we removing the encoding?

Don't like the whole section how? My understanding of the if block is that depending on the source, it needs to pull the data differently. The only problem is that (for web URLs) it's trying to decode the passed in URL back to UTF-8, which will cause the URLWithString call to fail if the originally provided source URL had any percent-encoded characters in it.

Really, why is an encoded URL being passed in the first place?

Well, the library doesn't say how it wants the URL, but as a user I'd certainly expect that if I send a percent-encoded URL, that it would be ok with that. Perhaps it could also accept unencoded UTF-8 URL strings and percent-encode them, to be extra friendly to the user, but the one thing it CANNOT do is call stringByRemovingPercentEncoding.

I like your change, but really I think it should remove the whole "if" section.

Again, I'm not sure what you mean by this? Remove which parts?

@trepidity trepidity reopened this Jan 10, 2018

@trepidity trepidity closed this Jan 10, 2018

@trepidity trepidity reopened this Jan 10, 2018

@trepidity

This comment has been minimized.

Show comment
Hide comment
@trepidity

trepidity Jan 10, 2018

Collaborator

Now that I look at this PR and the other, I'm not sure they are the same.
So the use case is when you (the developer) does not have control over the URL that is provided to RNS?

Like maybe a RSS feed or if you allow the user to input a URL?

Collaborator

trepidity commented Jan 10, 2018

Now that I look at this PR and the other, I'm not sure they are the same.
So the use case is when you (the developer) does not have control over the URL that is provided to RNS?

Like maybe a RSS feed or if you allow the user to input a URL?

@mojombo

This comment has been minimized.

Show comment
Hide comment
@mojombo

mojombo Jan 10, 2018

Contributor

My use case is for a language learning app. Each flash card has some audio associated with it that is downloaded over the internet. I pull the card data (including the audio URL) from our API and get back a percent-encoded URL that I feed to RNS. I ran into this problem because our URLs are named after the words or phrases, and they often have non-ASCII UTF-8 in them. When one of those URLs (with percent-encoding in them) are fed to RNS, they fail because RNS is trying to decode them back to raw UTF-8 which causes the URLWithString to fail, since it cannot deal with non-ASCII characters. Here's the relevant bit of documentation on URLWithString:

This method expects URLString to contain only characters that are
allowed in a properly formed URL. All other characters must be
properly percent escaped. Any percent-escaped characters are
interpreted using UTF-8 encoding.

Contributor

mojombo commented Jan 10, 2018

My use case is for a language learning app. Each flash card has some audio associated with it that is downloaded over the internet. I pull the card data (including the audio URL) from our API and get back a percent-encoded URL that I feed to RNS. I ran into this problem because our URLs are named after the words or phrases, and they often have non-ASCII UTF-8 in them. When one of those URLs (with percent-encoding in them) are fed to RNS, they fail because RNS is trying to decode them back to raw UTF-8 which causes the URLWithString to fail, since it cannot deal with non-ASCII characters. Here's the relevant bit of documentation on URLWithString:

This method expects URLString to contain only characters that are
allowed in a properly formed URL. All other characters must be
properly percent escaped. Any percent-escaped characters are
interpreted using UTF-8 encoding.

@trepidity

This comment has been minimized.

Show comment
Hide comment
@trepidity

trepidity Jan 11, 2018

Collaborator

Do you have examples of 1 that works, 2 that fails, 3 and the final desired result?

Collaborator

trepidity commented Jan 11, 2018

Do you have examples of 1 that works, 2 that fails, 3 and the final desired result?

@mojombo

This comment has been minimized.

Show comment
Hide comment
@mojombo

mojombo Jan 11, 2018

Contributor

Works (because it does not contain percent-encoded characters):

https://chatterbug.s3.amazonaws.com/recordings/audios/000/014/801/mp3/29068_froh.mp3?1481312271

Fails (because it DOES contain percent-encoded characters):

https://chatterbug.s3.amazonaws.com/recordings/audios/000/014/970/mp3/29180_die_N%C3%A4he.mp3?1481312786

The desired result is that both of those URLs will work. This PR allows them both to work.

Contributor

mojombo commented Jan 11, 2018

Works (because it does not contain percent-encoded characters):

https://chatterbug.s3.amazonaws.com/recordings/audios/000/014/801/mp3/29068_froh.mp3?1481312271

Fails (because it DOES contain percent-encoded characters):

https://chatterbug.s3.amazonaws.com/recordings/audios/000/014/970/mp3/29180_die_N%C3%A4he.mp3?1481312786

The desired result is that both of those URLs will work. This PR allows them both to work.

@mojombo

This comment has been minimized.

Show comment
Hide comment
@mojombo

mojombo Jan 12, 2018

Contributor

Awesome, thanks!

Contributor

mojombo commented Jan 12, 2018

Awesome, thanks!

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