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

Fixing search where Title contains spaces #2

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jasondew
Copy link
Contributor

jasondew commented Feb 19, 2016

Hi @zachgarwood! I'm very new to Elixir but I started a project where I needed to search the Amazon API and noticed that the signature wasn't valid for Titles containing a space. I wanted to write a failing test but I was unable to get the existing test to run due to some dependency issues. Anyway, this is what I came up with to make things work for me. I did remove the process_url callback and just computed the params up front. Feel free to merge or ignore. Either way, thanks for writing this!

@zachgarwood

This comment has been minimized.

Copy link
Collaborator

zachgarwood commented Feb 19, 2016

That's odd, the URI.encode_query call should be url-encoding each query parameter. But you're saying that it was not url-encoding spaces properly?

@jasondew

This comment has been minimized.

Copy link
Contributor Author

jasondew commented Feb 20, 2016

Right, the crux of the matter is that encode_query converts spaces to + instead of %20 as they should be:

iex> URI.encode_query(%{"Title" => "big hero 6"})
"Title=big+hero+6"
iex> AmazonProductAdvertisingClient.parameterize(%{"Title" => "big hero 6"})
"Title=big%20hero%206"

Oddly, the docs say that encode_query uses encode but it doesn't seem to? Based on my tracing through https://github.com/elixir-lang/elixir/blob/v1.1.1/lib/elixir/lib/uri.ex#L61 it goes encode_query -> pair -> encode_www_form -> percent which specifically changes %20s into +s. /shrug

@zachgarwood zachgarwood self-assigned this Feb 24, 2016

@zachgarwood

This comment has been minimized.

Copy link
Collaborator

zachgarwood commented Feb 24, 2016

I'm going to hold off on merging this until 1) I do some research into the difference between + and %20 space encoding (You know, I've been a web dev for years and never bothered to look it up. ¯\_(ツ)_/¯) and 2) I investigate the testing issue. I'll try to take a look at this in the next couple days or early next week.

@jasondew

This comment has been minimized.

Copy link
Contributor Author

jasondew commented Feb 24, 2016

Sounds great. Here are some details on the dependency issues I ran up against while trying to get the tests to run. The first error was:

warning: the dependency :timex requires Elixir "~> 1.0.0" but you are running on v1.2.2

A mix deps.update timex seemed to fix that, which lead to:

** (Mix) Could not compile dependency :hackney, "/Users/jasondew/.mix/rebar compile skip_deps=true deps_dir="/Users/jasondew/Projects/elixir-amazon-product-advertising-client/_build/test/lib"" command failed. You can recompile this dependency with "mix deps.compile hackney", update it with "mix deps.update hackney" or clean it with "mix deps.clean hackney"

Unfortunately, mix deps.update hackney doesn't fix that for me :/

@zachgarwood

This comment has been minimized.

Copy link
Collaborator

zachgarwood commented Feb 29, 2016

  • 1) In my + vs %20 usage research I found that, because of historical reasons, it's generally accepted to percent encode spaces everywhere in a URL except for the query string; however, the Amazon docs specifically state: "Percent encode the space character as %20 (and not +, as common encoding schemes do)." 😕, but whatever. I added a percent_encode_query/1 to handle this.
  • 2) I updated the dependencies in mix.exs, cleaned with mix deps.clean, then updated with mix deps.update --all and the tests worked for me.

@jasondew, sorry, I didn't end up using your code changes. But could you pull down my space-encoding branch (28ebdd1), run the tests and make sure everything works on your end?

Also, could you confirm something for me: Does the Signature param need to be the last parameter in the query, or can it appear anywhere in the query string? (I've not used the API in a while and my developer account has lapsed, or else I'd check it myself.) This could allow me to simplify the sign_url/1 function even further. Thanks in advance!

@zachgarwood

This comment has been minimized.

Copy link
Collaborator

zachgarwood commented Feb 29, 2016

@jasondew Sorry, since I didn't use your code, I'm closing this pull request and moving discussion over to #3. Please still test off that branch, and let me know about the Signature param ordering if you wouldn't mind.

@jasondew

This comment has been minimized.

Copy link
Contributor Author

jasondew commented Mar 1, 2016

@zachgarwood No worries, thanks for fixing the +/%20 issue! I can confirm that everything works against the live API and the tests pass. I can also confirm that the Signature params position seems to not matter. I put it in the beginning of the query string with no problems. I do still need my second commit from the PR in order to use the main line repo. Would you be willing to accept that in a PR?

@zachgarwood

This comment has been minimized.

Copy link
Collaborator

zachgarwood commented Mar 2, 2016

@jasondew Oh, sorry, I didn't even notice that change. Yeah, submit that in a new PR please. Are there any other params I missed for the ItemSearch struct?

@zachgarwood

This comment has been minimized.

Copy link
Collaborator

zachgarwood commented Mar 5, 2016

@jasondew I also put in a PR to correct the language used in URI's doc block: elixir-lang/elixir#4361.

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