Several years ago, I have decided that it would be nice to add fluent API to JsonUnit library. If you are not familiar with fluent assertion APIs it looks like this.

assertThatJson("{\"test\":1}").node("test").isEqualTo(1);

assertThatJson("[1 ,2]").when(IGNORING_ARRAY_ORDER).isEqualTo("[2, 1]");

I really like fluent assertions. First of all, they are really easy to use, you get help from your IDE unlike when using Hamcrest static methods. Moreover, they are quite easy to implement. This is how the original implementation looked like.

public class JsonFluentAssert {

    protected JsonFluentAssert(...) {
    }

    public static JsonFluentAssert assertThatJson(Object json) {
        return new JsonFluentAssert(...);
    }

    public JsonFluentAssert isEqualTo(Object expected) {
        ... 
    }


    public JsonFluentAssert node(String path) {
        ...
    }


    public JsonFluentAssert when(Option firstOption, Option... otherOptions) {
    ...
    }

    ...
}

We have static factory method assertThatJson() that creates JsonFluentAssert. And all other methods return the same class so you can chain them together. Nice and simple. Unfortunately, there are three mistakes in this API. Do you see them? Congratulations if you do. If not, do not be sad, it took me several year to see them

Unnecessarily complicated

The biggest mistake is that the API supports chaining even after the assertion method isEqualTo() is called. It seems I designed this way on purpose since there is a test like this

assertThatJson("{\"test1\":2, \"test2\":1}")
    .node("test1").isEqualTo(2)
    .node("test2").isEqualTo(1);

The problem is that to support such rare use-case, now the API is more error prone. I got several error reports complaining that this does not work

assertThatJson("[1 ,2]").isEqualTo("[2, 1]").when(IGNORING_ARRAY_ORDER);

It looks reasonable, but it can not work. The comparison has to be done in isEqualTo and if it fails, it throws an assertion error so when() method is not called at all. In current design you can not postpone the comparison since we do not know which method is the last one in the invocation chain. OK, how to fix it? Ideally, isEqualTo() should have returned void or maybe some simple type. But I can not change the contract, it would break the API.

If I can not change the API, I can do the second best thing – mark it as deprecated. This way the user would at least get a compile time warning. It seems simple, I just need to return a new type from isEqualTo() with when() method marked as deprecated.

Methods return class not an interface

Here comes second mistake – isEqualTo() and other methods return a class not an interface. An interface would have given me more space to maneuver. And again, I can not introduce an interface without potentially breaking backwards compatibility. Some clients might have stored the expression result in an variable like this

JsonFluentAssert node1Assert = 
assertThatJson("{\"test1\":2, \"test2\":1}").node("test1");

If I want to mark method when() as deprecated if called after assertion, isEqualTo() has to return a subclass of JsonFluentAssert like this

public class JsonFluentAssert {

    private JsonFluentAssert(...) {
    }

    public static JsonFluentAssert assertThatJson(Object json) {
        return new JsonFluentAssert(...);
    }

    public JsonFluentAssertAfterAssertion isEqualTo(Object expected) {
        ... 
    }


    public JsonFluentAssert node(String path) {
        ...
    }


    public JsonFluentAssert when(Option firstOption, Option... otherOptions) {
        ...
    }
    ...
    public static class JsonFluentAssertAfterAssertion extends JsonFluentAssert {

        @Override
        @Deprecated
        public JsonFluentAssert when(Option firstOption, Option... otherOptions) {
            return super.when(firstOption, otherOptions);
        }
   }
}

Extensibility

Third mistake was to make the class extensible by making the constructor protected and not private. I doubt anyone actually extends the class but I can not know for sure. And I can not change signature of isEqualTo() without breaking subclasses.

Solution

It’s pretty tricky situation. I can either keep broken API or break backward compatibility, tough choice. At the end I have decided to bet on the fact that no one is extending JsonFluentAssert and I did the change depicted above. I may be wrong but there is nothing else I could do, I do not want to live with bad API design forever. But if you have a better solution, please let me know. Full source code is available here.