ericblade / mws-advanced

Javascript (node) Amazon Merchant Web Services (MWS) code with mws-simple as it's underpinnings
Apache License 2.0
41 stars 11 forks source link

getMatchingProductForId returns extremely long floats that look wrong in packageDimensions and itemDimensions #54

Open ericblade opened 6 years ago

ericblade commented 6 years ago

Sometimes, getMatchingProductForId returns itemDimensions in extremely long floats-as-strings.

It's unclear if this is coming from Amazon directly, or if it's something that is occurring as the data gets handed between amazon, mws-simple, and mws-advanced. It is clear now, as per https://github.com/ericblade/mws-advanced/issues/54#issuecomment-386970616 that the actual Amazon MWS output is bad.

        const results = await mws.getMatchingProductForId({
            MarketplaceId: 'ATVPDKIKX0DER',
            IdType: 'ASIN',
            IdList: ['B0026HM31E' ],
        });

occasionally exhibits this. Sometimes I get ...attributeSets.itemAttributes.packageDimensions as

{ height: { value: "0.69999999999999...", ... }, ... }

sometimes i get it as 0.70. So, they are badly rounded strings.

Ultimately: I don't know who's screwing these up. There's no parseFloat() usage in either mws-advanced or mws-simple. It could be occurring in xmlParser inside mws-simple, or it could be occurring before it even gets to us. Since it's not happening 100% of the time, I'm wondering if it's at the MWS server side.

Is it a valid solution, if it's occurring server side, to String(parseFloat(height.value).toFixed(2)) at a layer in between? That could break legit data, potentially. I feel like if I bring it up to MWS, i'll never really get a resolution as to if it's their end, or if it's fixed or not. :-S

pwrnrd commented 6 years ago

Tricky one indeed! I think your proposed solution is inline with the data used by Amazon based on the examples that Amazon provides in their documentation.

Example 1:

<ns2:ItemDimensions>
  <ns2:Height Units="inches">9.17</ns2:Height>
  <ns2:Length Units="inches">7.36</ns2:Length>
  <ns2:Width Units="inches">0.75</ns2:Width>
  <ns2:Weight Units="pounds">1.40</ns2:Weight>
</ns2:ItemDimensions>

Example 2:

<ns2:PackageDimensions>
  <ns2:Height Units="inches">0.80</ns2:Height>
  <ns2:Length Units="inches">9.10</ns2:Length>
  <ns2:Width Units="inches">7.30</ns2:Width>
  <ns2:Weight Units="pounds">1.35</ns2:Weight>
</ns2:PackageDimensions>

In these examples Amazon rounds to 2 decimals. Therefore I think it is save to round all dimensions to two digits. Practically, this also makes sense. I don't think that Amazon has invested in technology that would enable them to measure the dimensions of products in inches at an accuracy higher than 2 decimals.

Another solution would be to leave this choice to the client. For example, an options/config object could be passed to mws.init in which clients can indicate their pre. However, this would make the package more complex.

ericblade commented 6 years ago

Sure, and also as long as it doesn't break a boundary between say 20.0" and 20.1" then it doesn't really matter for postal usage -- 19.99999999" and 20.0 are the same thing to the post office, but 19.99999" and 20.01" are different. So I really doubt that there would be trouble doing it.. but I think it would be very good to know where the breakage is coming from.

What I've done for the time being, is at the client display end, the ultimate consumer of the data, I've done the parseFloat().toFixed(2) solution.

Maybe I could do like 100 requests straight from the Scratchpad, then do 100 requests straight from mws-simple, then 100 requests from mws-advanced and compare the results. Unfortunately, I don't know if MWS caches results for a time, so perhaps all 300 might come out with the exact same answer if it's a problem at Amazon's end.

Also, glad to see someone else out here :-) I've been working hard at this lib, though much more so recently on the consumer for it. :-)

pwrnrd commented 6 years ago

I can see that you put in a lot of work and I'm sure there are more people that recognise that! I'm not as good as a programmer in JS/Node as you are so I'm more then happy to think with you about issues like these.

Concerning the matter at hand, the question boils down to whether this is a floating point issue or not. Given the information you provided, Amazon's examples and measurement in practice, I would feel comfortable with taking this risk and assuming its a floating point issue. However, I cannot be a 100% sure.

I like your solution to see where the issue lies. Regarding caching, to determine whether MWS uses a cache, we could send in a request, measure the response time and compare this to response times of subsequent responses (we can even do this at multiple intervals). That way we can figure out how Amazon deals with caching.

Something else that might give us more assurance as to go with the temporary fix or not, is to manually put in product detail dimensions for a certain product. I read this on the MWS forum: https://sellercentral.amazon.com/forums/t/populating-product-detail-dimensions/390453 . I don't have a seller account, but if a seller could fill in dimensions for a product with three or more decimals and see whether Amazon accepts or rejects this, then we can be more certain about the temporary fix (or uncertain, depending on the outcome).

What do you think? Is this any help to you?

ericblade commented 6 years ago

well, I am positive that it's an error where someone is doing a probably unnecessary floating point conversion. It's a bit surprising to me, that a global sales API is actually using strings with decimal representations (or worse, actual float representations), when they absolutely should be using cents (or whatever the equivalent is in every other currency).

I don't know that I'd be able to tell the difference between a cached response from mws and a non-cached response, there's so many variables that could affect the data transfer, and the speed anyway might be negligibly different.

Unfortunately, this isn't really important enough to me at the moment to spend much time digging around in. I'm pretty sure that it's a problem below mws-simple, since I don't do any string-to-float conversion on the numbers, but tracking it down further than that requires that I dedicate potentially a lot of time into something that is clearly wrong, but doesn't affect me anywhere near as much as fixing major issues and implementing new code into my client. :-S i don't like to say that, but since i'm the only person working on this client software, i don't have a lot of time to dig into this right at the moment. There's plenty of more pressing things to be done both in my client, and in this library.

Of course, another problem, is that if the data is coming back differently between the actual MWS call, and into mws-simple .. does it affect anything else that could be or looks like a floating point number? ugh!

ericblade commented 6 years ago

Actually, one of the things that I want to do is implement a dummy server that presents the mock data that Amazon provides over in their PHP API implementation. If that's implemented, I could quickly write something to make thousands of requests to the dummy server to see if the data ends up different than expected once it gets to mws-simple. Otherwise, I might be stuck with putting some debug logging into mws-simple to get some warnings from my 'production' server when it sees weird looking data, and just hoping to find it.

pwrnrd commented 6 years ago

Of course, another problem, is that if the data is coming back differently between the actual MWS call, and into mws-simple .. does it affect anything else that could be or looks like a floating point number?

You are absolutely right. That's tricky!

ericblade commented 6 years ago

... I think I'm going to worry more about this problem if I see anything weird out of the Financial category. If it only affects shipping dimensions, by .01-ish of a unit, on occasional calls, then it's probably not worth digging down into. If it affects prices or other financial data, then it becomes a big problem. But I'm not currently working with the Financial category, i'm mostly working in /Products right now.

ericblade commented 6 years ago

Well, I've confirmed that this is an error on Amazon's side:

image

... I guess I can handle rounding it down somewhere, or just warn users. :-D

pwrnrd commented 6 years ago

... I guess I can handle rounding it down somewhere, or just warn users. :-D

I think so too! Nicely found!

ericblade commented 6 years ago

Oh boy. I've also just now noticed this in currency calculations, in the output of GetLowestPricedOffersForASIN for 0743229967, right now, there is one offer that is claiming it's price is 15.0400000000000001. Bad, Amazon. Bad. :-D