Skip to content

Conversation

@Hronom
Copy link
Contributor

@Hronom Hronom commented Oct 23, 2015

Greetings everyone!

spring-test for comparing json use external library called jsonassert.

This lib has several mods of comparing json:

  • STRICT - Strict checking. Not extensible, and strict array ordering.
  • LENIENT - Lenient checking. Extensible, and non-strict array ordering.
  • NON_EXTENSIBLE - Non-extensible checking. Not extensible, and non-strict array ordering.
  • STRICT_ORDER - Strict order checking. Extensible, and strict array ordering.

By default spring-test in method org.springframework.test.web.servlet.result.ContentResultMatchers#json use this library in mod LENIENT.

This mod is not strict enough. In situation when in your json new field has added, your tests not failed, so app that use API with this json can crash when try to deserialize this in some POJO!

Also this is not acceptable, when you want compare json in other useful mods, for example NON_EXTENSIBLE.

I have added changes and for now, developer can select mod that he want.

Issue: SPR-13607

I have signed and agree to the terms of the Spring Individual Contributor
License Agreement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are breaking changes.

If you want to introduce something like this, you will have to introduce overloaded methods that accept the new method argument, keeping the existing signatures in tact.

@sbrannen
Copy link
Member

Please create a JIRA issue requesting this improvement.

@sbrannen
Copy link
Member

Also, have you signed the Spring Individual Contributor License Agreement (ICLA)?

@sbrannen sbrannen self-assigned this Oct 24, 2015
@Hronom
Copy link
Contributor Author

Hronom commented Oct 25, 2015

Thanks for your comments.

I add sign of Spring Individual Contributor License Agreement (ICLA) to my initial comment.

Also I create issue in JIRA

I created changes that breaks API, because I got a problem with tests, which does not breaks, when my API accidentally has changed. So the default comparison mode is very error prone. And I think that by removing "default method" we remove oportunity for developers to go in a wrong way.

Also this problem can be solved by setting STRICT mode for default method, but, I think this breaks some test cases in more bad way, because code compiled but tests fails, after upgrading version of Spring Framework.

So on my opinion, the better way is to break API, so when compilation fails, developers can find what goes wrong faster.

Sorry for my bad english I hope you understand what I try to explain=)

@sdeleuze
Copy link
Contributor

@Hronom: as said by Sam, you should introduce overloaded methods instead of making those breaking API changes.

Maybe you could update JsonExpectationsHelper Javadoc to be more specific about the comparaison behavior, in order to avoid the issues you encountered.

Would introducing a (String expected, String actual, boolean strict) variant be enough for your use case? I am asking that because I am wondering if we could avoid exposing JSONAssert specific JSONCompareMode type in JsonExpectationsHelper API.

@sbrannen Any thoughts about exposing JSONCompareMode in the API?

@sdeleuze
Copy link
Contributor

As discussed with @jhoeller during our weekly meeting, it would be nice to only add the (String expected, String actual, boolean strict) variant and avoid to expose JSONCompareMode in Spring API.

@Hronom
Copy link
Contributor Author

Hronom commented Oct 27, 2015

Looks like (String expected, String actual, boolean strict) is acceptable for my case.
I commit changes with overloaded method.

@sdeleuze sdeleuze assigned sdeleuze and unassigned sbrannen Oct 28, 2015
@sdeleuze
Copy link
Contributor

I merged a polished version of your commit that also avoid to break current API.
Thanks for your contribution.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants