Ignore JSON keys order when stubbing a request fixes #485

[JanDintel]

No description provided.

[JanDintel]

How stable is the the build? Because the build even fails when changing nothing, see: https://travis-ci.org/JanDintel/webmock/builds/159343501

[bblimke]

@JanDintel thank you for the pull request.

Could you please describe the reasons behind this pull request?

I get the idea, but I'm not sure it's necessary or desired.

If the pattern body is declared as a hash then by all means the order of keys in json request body should not matter (that's already handled by webmock).
If one declares body pattern as a string though, perhaps it's important that the body should be exactly, what was declared in the pattern.
This gives users freedom to choose whether they want exact json string or whether the order doesn't matter.
Currently can always declare body as body: JSON.parse(....) in the request pattern if you want to ignore the order of keys.

[JanDintel]

@bblimke Thanks for your response.

The main reason behind the pull request is because I ran into this issue while using the gem and the issue was reported in #485.

I understand the the point your making. You want to offer users the flexibility of choosing how they match their request/response bodies. However I think that by offering this flexibility the library is less intuitive to use. To me it seems that the use case of exactly matching the JSON request/response bodies is very limited. Therefore I would opt for the more intuitive method.

I understand your point, please consider mine. I don't mind to closing this pull request, if you want to stick to the current behaviour. 😄

Side note
According to RFC4627 which describe the JSON format, the order shouldn't matter either:

An object is an unordered collection of zero or more name/value
pairs, where a name is a string and a value is a string, number,
boolean, null, object, or array.
An array is an ordered sequence of zero or more values.

[bblimke]

I get your point. Following your reasoning, shouldn't that be the same for other content types? I.e XML or form url encoded body?
I'm concerned about custom elsif @pattern.is_a?(String) && BODY_FORMATS[content_type] == :json condition, instead of a generic one.
Otherwise we get different behaviour for different content types.

[JanDintel]

I wanted to keep the change small and 'relevant'. Therefore I kept my change to the JSON bodies specifically, since there was only a issue reported regarding the ordering of keys in JSON and no other content types (#485).

It's a valid concern, for consistency I would expect the same behaviour with order content types. However this introduces the overhead of knowing which content type is and which content type isn't order sensitive. Supporting this in the library will also increase complexity.

My change might be more intuitive, but after your arguments I opt not to merge it since:

• A workaround is available e.g. with(body: { foo: 'bar' }, headers: { 'Content-Type' => 'application/json' })
• Additional support for other content types will most likely increase the complexity
• You keep the flexibility to choose how the request/response bodies are matched

@bblimke What do you think?

[bblimke]

@JanDintel ok, let's keep the current behaviour, but review the pull request if someone reports this issue again.

[Fivell]

@bblimke any chance to push it ?

[bblimke]

@Fivell After reviewing that PR I still not entirely convinced this should be merged,
but perhaps you can convince me :) Have you reviewed the comments above?

How did you find that PR. Can you please provide more details on how that issue has affected you?

[danielkaczmarczyk]

I've spent a while debugging my networking components just to find that the key order was the issue. Had to diff them!

I am not sure if ignoring the keys ordering is necessary. Maybe itemizing the diff and showing lines / chars that differ or something to that effect could be a worthy approach?

Since it's been so long since this PR was active, @bblimke - what's your current opinion on those topics?

I'm happy to provide a PR that aligns with your vision.

[bblimke]

@danielkaczmarczyk, thank you for bringing up this issue again, and I apologize for the hours you had to spend debugging it.

After reviewing all the previous comments in this thread (or due to more years programming ;) ), I have adjusted my perspective. I now concur that it is sensible to incorporate support for matching against JSON strings while disregarding their order. The comment by @JanDintel below resonates with me:

However, I think that by offering this flexibility, the library becomes less intuitive to use.

I acknowledge that although WebMock now offers flexibility by allowing the declaration of bodies as Hash, it might not be immediately obvious to users that this is the approach they should take. I recognize that I may have made this same oversight in the past, despite being the creator of the current behavior 🤦‍♂️.

For those (if anyone) who find the order of JSON keys significant, I propose that this can be addressed through an option or configuration within WebMock.

I would like to propose the following steps:

1. In WebMock 3.x, include a suggestion notice printed in case of a failure to match the JSON body. For instance, when the parsed JSON string matches the parsed request body, print a suggestion that the stub be declared as body: JSON.parse(...). This should help prevent the extensive debugging experienced by both you, @JanDintel, and you, @danielkaczmarczyk.
2. Implement the default behavior as proposed by @JanDintel, while also adding a configuration option to WebMock to disable it for those who wish to match the exact JSON string.
3. Release a new major version of WebMock with the behavior outlined in step 2 implemented.

What are your thoughts on that?

[danielkaczmarczyk]

@bblimke thanks for your response. I like @JanDintel's solution and I think it would get the project quickest to being more flexible.

[brevio-nicolay]

I'd just like to add that I've had some problems with this as well, and will be incorporating @JanDintel's fix—thanks for the good work on the library 👍