FBA Inbound Shipment Operation Fail about peddler HOT 11 CLOSED

Wow. I have no idea how this didn't blow up before. It perhaps is a very recent (undocumented) change.

In any case, we now include theversion parameter in all requests, whether Amazon really expects it or not. I re-recorded all fixtures to make sure nothing else has broken.

Thanks, @shijunwu.

from peddler.

lol, yeah, i think its a recent, and its not all operations have that
issue.

And thankyou for your attention.

On Sat, Mar 28, 2015 at 3:06 PM, Hakan Ensari [email protected]
wrote:

Wow. I have no idea how this didn't blow up before. It perhaps is a very
recent (undocumented) change.

In any case, we now include theversion parameter in all requests, whether
Amazon really expects it or not. I re-recorded all fixtures to make sure
nothing else has broken.

Thanks, @shijunwu https://github.com/ShijunWu.

—
Reply to this email directly or view it on GitHub
#36 (comment).

from peddler.

hakanensari, I just remember there's another minor things that are not compatible in the current version which is not relate to this issue.
The amazon's api (create inbound shipment (plan) or update inbound shipment), the max uniq sku is 200, but if u try to do those operation for more than about 50 or sth like that, you will get an error about uri too long from excon. Because in jeff, it use the query to create the signature and append it back to the url, which make it too long. Of course, you can put the skus in the body, but it will not use them to create the signature which will fail. So I did need to tweak it and instead of append the signed query back to url, i put it to the body if the request is post.

from peddler.

@shijunwu,

Could I take a look at your patch?

from peddler.

Yeah, but what i did is a quick way around, its not best practice. Since i use rails, i just have an file in config/initializers that override the jeff's build_options method. Since the jeff i am using is kinda old, i copy the newest one and patch what i change to it below:

def build_options(options)

 # Add Content-MD5 header if uploading a file.
 if options.has_key?(:body)
   md5 = Content.new(options[:body]).md5
   (options[:headers] ||= {}).store("Content-MD5", md5)
 end

# Build query string.
query_values = default_query_values.merge(options.fetch(:query, {}))
query_string = Query.new(query_values).to_s

# Generate signature.
signature = Signer
  .new(options[:method], connection.data[:host], options[:path] || connection.data[:path], query_string)
  .sign_with(aws_secret_access_key)

#Everything above are the same

# Return options after appending an escaped signature to query.
if(options[:method].upcase == 'POST' && !options.has_key?(:body))
  options.update(query: "")
 (options[:headers] ||= {}).store('Content-Type', 'application/x-www-form-urlencoded')
  options.update(body: "#{query_string}&Signature=#{Utils.escape(signature)}")
else
   options.merge(query: "#{query_string}&Signature=#{Utils.escape(signature)}")
end

end

Basically, what i did is check if the method is post and dont have body, then i append the signed query to body and clear the query, otherwise does what it does before.
This is not a clean way to do it, but in this case, it is pretty specific, since the body only present when upload the file, otherwise, the body will be just blank.
I think a better way might set the content-type in peddler to application/x-www-form-urlencoded if its not try to upload the file and check the content-type in jeff?
Or alawys include the other params beside the basic params like auth and id, then in jeff, check if the content-type is file, if not, also include the body to singed the query and append the signed query to url

from peddler.

@shijunwu,

I like your original solution (and I'm not sure I fully got the explanation of the other two alternatives). What you did is another step in massaging the input, like when Jeff adds a checksum if there's a body.

One side note: It may make sense to do away with all the unused HTTP verbs in Jeff and just stick to POST.

Let me think about this for a while. I'm on holiday so may not get back as quickly. Feel free to submit a PR to Jeff in the meanwhile.

from peddler.

@hakanensari ,
Its not urgent, so no worry about it and hope you have great vacation.

Yeah, the original solution is like adding the checksum which massage the input, and i think this is a easiest way to fix it. But i just concern that this way is not very safe, let say in the future, something change which make request that not upload the file also have the body, then it will fail(not really fail, just back to the current way). And I just feel this kind of checking is not very good and safe.

The other 2 I mentioned are basically pretty much the same thing, sorry for the confusion on them. So I was thinking is like the path and version, maybe add another one to specific whether or not should append the signed query to body or not. This can be in class lvl which is a little too generic, or in method lvl, this will be more safe.Then we can remove this param in jeff or i think even keep it wont give u error. And dont need to add it to every method/class, just add it to the method/class that need upload file and the rest use default value. In this way, i think its more safe and clear.

If you come up a better solution, please let me know, by going through your code, I do learn many new things.

from peddler.

@shijunwu,

I'm wondering if Amazon documents what maximum query it accepts in a GET query. Would you have any leads?

from peddler.

@hakanensari ,
Ya, amazon's api have the max number that can query at a time for each operation, in this case, i think its diff, for normal http request the max url is around 2000 characters, for the shipment example, it can query up to 200 skus, but the query key is very long, i think around 50 sku, the url already over 2000 characters

from peddler.

@shijunwu

I'm not sure this is the best solution, but I handle now 414 in Jeff. If Amazon returns this, Jeff retries after moving the query to the body. There's an extra request involved, but I felt this is cleaner because Jeff doesn't need to make any assumptions about when the error would be triggered (e.g. size of URI) and behaviour is otherwise unchanged.

Thanks again for surfacing this issue.

from peddler.

@hakanensari
I think its a good way to handle it also, even there's one extra request, but its cleaner and no need to change the behavior.

I am glad can help.

from peddler.