-
Notifications
You must be signed in to change notification settings - Fork 553
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
Ignore JSON keys order when stubbing a request fixes #485 #649
base: master
Are you sure you want to change the base?
Conversation
How stable is the the build? Because the build even fails when changing nothing, see: https://travis-ci.org/JanDintel/webmock/builds/159343501 |
@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). |
@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
|
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 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:
@bblimke What do you think? |
@JanDintel ok, let's keep the current behaviour, but review the pull request if someone reports this issue again. |
@bblimke any chance to push it ? |
@Fivell After reviewing that PR I still not entirely convinced this should be merged, How did you find that PR. Can you please provide more details on how that issue has affected you? |
I've spent a while debugging my networking components just to find that the key order was the issue. Had to 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. |
@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:
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:
What are your thoughts on that? |
@bblimke thanks for your response. I like @JanDintel's solution and I think it would get the project quickest to being more flexible. |
No description provided.