Rewrite / check our equals/hashcode methods

[sbernard31]

Investigating on NPE in LwM2mPath.hashcode() find at #1502.

I decide to add unit tests to check the bug before to fix it.

I found that library to do that job : EqualsVerifier.

Good news : it does a very good job.
Bad news: this is more complicate than expected to implement good custom equals/hashcode method.

We should :

1. Have a look at all custom equals/hashcode methods and add a corresponding test based on EqualsVerifier.
2. Decide some convention about written equals/hashcode in Leshan.
3. Reimplement equals/hashcode if needed.
4. Add documentation about that in : https://github.com/eclipse-leshan/leshan/wiki/Code-&-design-guidelines#code-rules

Let's use this issue to talk about "Decide some convention about written equals/hashcode in Leshan".
If you interested about that you should first read : How to Write an Equality Method in Java

My current opinion :

• we should probably use Objects.equals and Objects.hash

• Use of this 3 a valid behavior :
  1. Use classic getClass() != obj.getClass() + making the class final : this is the simple way but I do not like too much the idea I prefer to let user being able to extend class.
  2. Use an instanceof + make hashcode/equals final : this is simple enough and this allow to extend the class but not to customize equals behavior...
  3. Use the canEqual method, which is a bit unusual but sounds to solve all problems.

  Should we encourage to use 3. in all code base ?

[nkrzykala]

Hello, I'm a new internship student at Orange under the lead of @JaroslawLegierski.

I was reading about this issue and I added test to almost every class which overrides equals and hashCode using EqualsVerifier (locally for now, not sure if this is needed).

Then I tried to implement canEqual method in the LwM2mPath class as @sbernard31 suggested, with the help of this article.

This is what I have now:

@Override
public final boolean equals(Object obj) {
        boolean result = false;
        if (obj instanceof LwM2mPath) {
            LwM2mPath that = (LwM2mPath) obj;
            result = (that.canEqual(this) && Objects.equals(objectId, that.objectId) && Objects.equals(objectInstanceId, that.objectInstanceId)
                    && Objects.equals(resourceId, that.resourceId)
                    && Objects.equals(resourceInstanceId, that.resourceInstanceId));
        }
        return result;
    }

public boolean canEqual(Object obj) {
       return (obj instanceof LwM2mPath);
    }

Is this the implementation you thought of in the comment?

The EqualsVerifier test passes without .simple(), but now there is another problem. I get a warning saying that equals and hashCode methods should be final (see the warning here). Is this a big challange for us?

Other tests showed some different warnings. I believe I could go into more detail once you find time to look again at this issue. Thanks

[sbernard31]

Is this the implementation you thought of in #1504 (comment)?

Yep that's pretty much the idea. if we decide to go with 3.
I still don't know what is the best way to follow. I really don't like 1.
But for most of the case maybe 2. is enough ?

At Orange do you have any opinion about that ?
Maybe we go with 2. and when we find use case where inheritance is needed with implement 3. ?

I was reading about this issue and I added test to almost every class which overrides equals and hashCode using EqualsVerifier

@nkrzykala how many class did you find ? the number could help to answer to question above ☝️

The EqualsVerifier test passes without .simple(), but now there is another problem. I get a warning saying that equals and hashCode methods should be final (see the warning here). Is this a big challange for us?

If we go with 2. no need to solve this (as we will have equals and hashcode final)
If we go with 3. we probably need to write something like :

    private class ExtendedLwM2mPath extends LwM2mPath {
        public ExtendedLwM2mPath(int objectId) throws InvalidLwM2mPathException {
            super(objectId);
        }
        @Override
        public boolean canEqual(Object obj) {
            return (obj instanceof ExtendedLwM2mPath);
        }
    }

    @Test
    public void assertEqualsHashcode() {
        EqualsVerifier.forClass(LwM2mPath.class).withRedefinedSubclass(ExtendedLwM2mPath.class).verify();
    }

[nkrzykala]

I found 67 classes which override equals and hashCode.

We agree that your approach:

Maybe we go with 2. and when we find use case where inheritance is needed with implement 3.

sounds good.

[sbernard31]

I found 67 classes which override equals and hashCode.

Are there classes which extend or are extended ?

Maybe we go with 2. and when we find use case where inheritance is needed with implement 3.
sounds good.

OK so let's go for 2. (instanceof + final equals/hashcode) for most classes and if there is some classes which are extended lets using 3. (canEqual).

Just to let you know, with eclipse you can generate equals using Source / Generate hascode() and equals()..., then you can check :

This gives this kind of code :

    @Override
    public int hashCode() {
        return Objects.hash(objectId, objectInstanceId, resourceId, resourceInstanceId);
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (!(obj instanceof LwM2mPath))
            return false;
        LwM2mPath other = (LwM2mPath) obj;
        return Objects.equals(objectId, other.objectId) && Objects.equals(objectInstanceId, other.objectInstanceId)
                && Objects.equals(resourceId, other.resourceId)
                && Objects.equals(resourceInstanceId, other.resourceInstanceId);
    }

Then you just need to add final to both methods.

If you are not using eclipse, please try to produce code which looks like this 🙏

[nkrzykala]

@sbernard31 I found 68 classes which override equals and hashCode, 20-25 of them are extended or extend (or both).

I have other questions:

1. In some cases the EqualsVerifier tests I wrote (in clasess which extend other class) return an error saying that not every field is used in equals/hashCode.
For example I have generated equals & hashCode and added canEqual for WriteRequest class (it extends AbstractSimpleDownlinkRequest):

@Override
    public boolean equals(Object o) {
        if (!(o instanceof WriteRequest)) return false;
        if (!super.equals(o)) return false;
        WriteRequest that = (WriteRequest) o;
        return that.canEqual(this) && Objects.equals(node, that.node) && Objects.equals(contentFormat, that.contentFormat) && mode == that.mode;
    }

    public boolean canEqual(Object o) {
        return (o instanceof WriteRequest);
    }

    @Override
    public int hashCode() {
        return Objects.hash(super.hashCode(), node, contentFormat, mode);
    }

The test looks like this:

    private class ExtendedWriteRequest extends WriteRequest {
        public ExtendedWriteRequest(Mode mode, ContentFormat contentFormat, int objectId, int objectInstanceId,
                            Collection<LwM2mResource> resources) throws InvalidRequestException {
            super(mode, contentFormat, newPath(objectId, objectInstanceId), new LwM2mObjectInstance(objectId, resources),
                    null);
        }
        @Override
        public boolean canEqual(Object obj) {
            return (obj instanceof ExtendedWriteRequest);
        }
    }

    @Test
    public void assertEqualsHashcode() {
        EqualsVerifier.forClass(WriteRequest.class).withRedefinedSuperclass().withRedefinedSubclass(ExtendedWriteRequest.class).verify();
    }

I get an error that "coapRequest" field is not included in equals or it is stateless (see this).
coapRequest field is in the AbstractLwM2mRequest class which AbstractSimpleDownlinkRequest class extends (same problem for example in BootstrapRequest class which extends AbstractLwM2mRequest class directly).
Should I ommit this field with .withIgnoredFields() or is there an error in my implementation?

2. In some other tests I get an error saying that fields of type BigDecimal should be compared using compareTo() in instead of Objects.equals(). Can we do that or should we suppress this warning? see this
3. In class RouterService there is a transient field (isPrefixed) used in equals and hashCode. Should we delete this field from equals&hashCode or suppress this warning? see this
4. What if the fields are not final? see this

Thanks for advice

[sbernard31]

1. coapRequest field should not be included in equals. So yes should should omit this field.

2. Yep we should use compareTo as we probably want to compare by value.

3. This is more or less a copy/paste from java-coap, It is not really necessary to test this one. I should be removed to use the java-coap one at mid term. If you really want to test it just remove the transient keyword. I don't understand why transient it used, so I asked to java-coap : open-coap/java-coap#88

4. this smell not so good, I guess we should study each case 1 by 1.

[nkrzykala]

2. I tried to add compareTo as it was showed here:

    @Override
    public final boolean equals(Object o) {
        if (this == o) return true;
        if (!(o instanceof JsonArrayEntry)) return false;
        JsonArrayEntry that = (JsonArrayEntry) o;
        boolean comparablyEqual = (time == null && that.time == null)
                || (time != null && that.time != null && time.compareTo(that.time) == 0);
        return Objects.equals(name, that.name) && Objects.equals(floatValue, that.floatValue)
                && Objects.equals(booleanValue, that.booleanValue) && Objects.equals(objectLinkValue, that.objectLinkValue)
                && Objects.equals(stringValue, that.stringValue) && comparablyEqual;
    }

Also modified the hashCode to use .stripTrailingZeros() so that the hashcode is consistent. Then, I got the NPE error in hashCode, on the field "time" (the one that is BigDecimal), see this. I guess that I should check in hashCode if the field is NULL. Is this possible?:

    @Override
    public final int hashCode() {
        int result = Objects.hash(name, floatValue, booleanValue, objectLinkValue, stringValue);
        result = 31 * result + (time != null ? time.stripTrailingZeros().hashCode() : 0);
        return result;
    }

4. The cases of the not final fields error are in classes:

• 4.1. SenMLRecord - all fields are not final (9)
• 4.2. JsonArrayEntry - all fields are not final (6)
• 4.3. JsonRootObject - all fields are not final (3)
• 4.4. MixedLwM2mLink - all fields are not final (2)
• 4.5. BaseAttribute - 2 out of 3 fields are not final
• 4.6. ValuelessAttribute - all fields are not final (1)
• 4.7. DeregisterRequest - all fields are not final (1)
• 4.8. SenMLPack - all fields are not final (1)

[sbernard31]

see this.

The this link is not good or ?

I tried to add compareTo as it was showed here:

Maybe we should consider ""Normalise the BigDecimal fields during your class’s construction" ?
I will look at this next week.

4. The cases of the not final fields error are in classes:

For :

• 4.4. MixedLwM2mLink - all fields are not final (2)
• 4.5. BaseAttribute - 2 out of 3 fields are not final
• 4.6. ValuelessAttribute - all fields are not final (1)
• 4.7. DeregisterRequest - all fields are not final (1)

I think make the field final should do the trick.

For :

• 4.8. SenMLPack - all fields are not final (1)
• 4.1. SenMLRecord - all fields are not final (9)
• 4.2. JsonArrayEntry - all fields are not final (6)
• 4.3. JsonRootObject - all fields are not final (3)

Use .suppress(Warning.NONFINAL_FIELDS) then we will fix that in another PR later.

[nkrzykala]

The this link is not good or ?

Sorry, I meant the site about NPE error.

Maybe we should consider ""Normalise the BigDecimal fields during your class’s construction" ?

OK, I will also look at this.

I think make the field final should do the trick

I'm afraid it won't work for DeregisterRequest class because there's a field registrationId which cannot be final (one of the constructors assigns a value to this field). Should I add the .suppress(Warning.NONFINAL_FIELDS) here as well?

Also I found another class with not final fields - Tlv

Thanks for the answer

[sbernard31]

I'm afraid it won't work for DeregisterRequest class because there's a field registrationId which cannot be final (one of the constructors assigns a value to this field).

I think we can just remove the null initialization in attribute definition.

---    private String registrationId = null;
+++    private final String registrationId;

[sbernard31]

Also I found another class with not final fields - Tlv

Check if setters are used in code (just by removing it and see if code still compiles)
If yes, I think the better solution is to remove setter and make the fields final.

[sbernard31]

About "Normalise the BigDecimal fields during your class’s construction", that comes with usage of Warning.BIGDECIMAL_EQUALITY but this is surprising to me so I ask about it : jqno/equalsverifier#987

[nkrzykala]

As I can see, open-coap/java-coap#52 (comment) says that I should delete the transient field from equals&hashCode.

Also in jqno/equalsverifier#987 what I understand:
it is either compareTo() or you have to use Warning.BIGDECIMAL_EQUALITY because you can't really test the normalisation of BigDecimal fields...

If you find time I would like to ask you to look into classes:

1. StringUtils - there is an equals() method but not the overriden one
2. SenMLTestUtil - same, there is an equals() method but not the overriden one

I don't know how to treat them. They don't look like they should be tested by the EqualsVerifier because they have a different purpose?

3. ULong - I added canEqual method (because ULong extends Number), but it doesn't change anything because ULong is final... should I leave ULong without canEqual?

4. LwM2mResourceInstance - there are comments in hashCode() and equals(): "custom hashcode/equals to handle arrays". Should I leave the custom as it is or the generated hashCode&equals methods are OK for you too?

[sbernard31]

As I can see, open-coap/java-coap#52 (comment) says that I should delete the transient field from equals&hashCode.

Nope just delete the transient key word, we duplicate that class especially because prefix in not in equals, see : open-coap/java-coap#52 (comment)

Also in jqno/equalsverifier#987 what I understand:
it is either compareTo() or you have to use Warning.BIGDECIMAL_EQUALITY

For now I understand that too.

If we go with "normalisation" (and so we use Warning.BIGDECIMAL_EQUALITY) maybe we can complete test of BigDecimal with a manual test like this : jqno/equalsverifier#987 (comment)

because you can't really test the normalisation of BigDecimal fields...

Yep not possible for now in EqualsVerifier, maybe in the future : jqno/equalsverifier#987 (comment)

So we must decide compareTo() or normalization at constructor ?
First, come at cost of custom equals/hashcode() and make equals and hashcode slower when used.
Second, come at cost of suppress warning + manual test, and make constructor slower.

If you find time I would like to ask you to look into classes:

StringUtils - there is an equals() method but not the overriden one
SenMLTestUtil - same, there is an equals() method but not the overriden one

this is static method this is out of scope of that PR, let this aside for now. (unless you think you maybe find an issue with it ? )

ULong - I added canEqual method (because ULong extends Number), but it doesn't change anything because ULong is final... should I leave ULong without canEqual?

The class is final so no need to add canEqual, as I understand that canEqual is to support inheritance.

LwM2mResourceInstance - there are comments in hashCode() and equals(): "custom hashcode/equals to handle arrays". Should I leave the custom as it is or the generated hashCode&equals methods are OK for you too?

Oh I looked at this class and I'm not sure EqualsVerifier will really like it ... (jqno/equalsverifier#987 (comment))
This is typically case where there is a kind of logic between attribute value...
But we need to have custom hashcode/equals for Array because of https://stackoverflow.com/questions/8777257/equals-vs-arrays-equals-in-java/8777266#8777266

[nkrzykala]

Second, come at cost of suppress warning + manual test, and make constructor slower.

Plus we need to add .stripTrailingZeros() to setters, which may also make them slower?

I have no strong opinion on whether compareTo() or normalisation is better/more efficient... I don't know what is more important for Leshan.

But we need to have custom hashcode/equals for Array because of https://stackoverflow.com/questions/8777257/equals-vs-arrays-equals-in-java/8777266#8777266

OK, I understand. I tried to test with EqualsVerifier on the original, custom equals&hashCode and I got this error:
https://jqno.nl/equalsverifier/errormessages/subclass-object-is-not-equal-to-an-instance-of-a-trivial-subclass-with-equal-fields/
One of the proposed solutions there is using instanceof so maybe we could do in equals:

-        if (obj == null)
-            return false;
-        if (getClass() != obj.getClass())
-            return false;

+       if (!(obj instanceof LwM2mResourceInstance)) return false;

and leave the custom rest. After that there is only symmetry problem which can be resolved by making the methods final (the class isn't extended and doesn't extend other class).

[sbernard31]

I have no strong opinion on whether compareTo() or normalisation is better/more efficient... I don't know what is more important for Leshan.

🤔 my guess most of the class will not be too much used in a Map but constructor is always called.
So maybe better to take performance penalty only on equals/hashcode in a first time, and so go for compateTo

So I suppose we go back to your initial question : #1504 (comment)

and you could do something like this :

public static class ClassWithBigDecimalField {
    private final BigDecimal bigdecimalField;
    private final String stringfield;

    public ClassWithBigDecimalField(BigDecimal b, String s) {
        this.bigdecimalField = b;
        this.stringfield = s;
    }

    @Override
    public final boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (!(obj instanceof ClassWithBigDecimalField))
            return false;
        ClassWithBigDecimalField other = (ClassWithBigDecimalField) obj;

        boolean comparablyEqual = (bigdecimalField == null && other.bigdecimalField == null)
                || (bigdecimalField != null && other.bigdecimalField != null
                        && bigdecimalField.compareTo(other.bigdecimalField) == 0);

        return comparablyEqual && Objects.equals(stringfield, other.stringfield);
    }

    @Override
    public final int hashCode() {
        return Objects.hash(bigdecimalField != null ? bigdecimalField.stripTrailingZeros() : null, stringfield);
    }

    @Override
    public String toString() {
        return String.format("ClassWithBigDecimalField [bigdecimalField=%s, stringfield=%s]", bigdecimalField,
                stringfield);
    }
}

One of the proposed solutions there is using instanceof so maybe we could do in equals:

Yep using instanceof + final equals/hashcode is what we decide above : #1504 (comment)

[nkrzykala]

and you could do something like this

OK, sure

Yep using instanceof + final equals/hashcode is what we decide above: #1504 (comment)

Yes, I wanted to make sure if this could be done here as well, without changing the custom parts 🙂

[nkrzykala]

Hello, I have pushed all changes to my branch: https://github.com/JaroslawLegierski/leshan/tree/opl/natalia_test

I'm waiting for your review😄

[sbernard31]

Could you create a PR this will be easier to review it 🙂

[nkrzykala]

Hi, PR #1644 is now created.

I'll be gone from the office next week, but ready on the 2nd of September to do fixups!

[sbernard31]

So #1644 is now integrated in master.

Remaining works are :

• 1. dealing with SenMLPack, SenMLRecord, JsonArrayEntry, JsonRootObject (comment#2278242269)
• 2. Add documentation about that in : https://github.com/eclipse-leshan/leshan/wiki/Code-&-design-guidelines#code-rules

@nkrzykala Let me know if you plan to work on that ? (1. or 2. or both)
(Just to know for organization 🙂)

[nkrzykala]

Yes, I can work on that, preferably on the 1.

[sbernard31]

Yes, I can work on that, preferably on the 1.

Not a surprise . Programmer rarely like writing documentation 😁

So do you want hint for that 4 classes ? or you want to try to propose your own solution based on your knowledge on this topic ?

• SenMLPack works with SenMLRecord,
• And JsonArrayEntry works with JsonRootObject.

So better to separate works/discussion in 2 tasks. (no preferred order for me)

[sbernard31]

Sure, should I create issues or would you like to do that?

We can start the discussion here and continue it in 2 separated PRs but you can also create dedicate issue if you prefer.

(any preferable way to name them for recognition?)

If you create an issue, feel free to choose a name.

So do you want hint for that 4 classes ?

OK so following #1504 (comment), we decide to
" go for 2. (instanceof + final equals/hashcode) for most classes and if there is some classes which are extended lets using 3. (canEqual). "

Here no need to extend, so we go with (instanceof + final equals/hashcode).
But we still have the Non Final Field Warning which is not so good because :

"This means that if the value of this field changes, the same object may not be equal to itself at two different points in time. This can be a problem if you use this object as a key in a map, or if you put it in a HashSet or other hash-based collection: the collection might not be able to find your object again."

So we should probably change that class to make it immutable by :

• simple solution remove setter and just keep constructor.
• if needed, eventually add a Builder.

Does it sounds good to you ?

[sbernard31]

I forget to mention that to make a class immutable. Attributes should generally be final but they should be immutable too.
If an attribute is a collection, that collection should be immutable too (or at least unmodifiable).

Since Java 9 there is immutable collection. But we target java 8 as minimal java version so we can not use it and so we need to fall back with unmodifiable collection. (see Collections.unmodifiableList)

[nkrzykala]

We can start the discussion here and continue it in 2 separated PRs but you can also create dedicate issue if you prefer.

Ok, let's stay here and go for 2 PRs later on.

simple solution remove setter

I thought about that at first when I got the error of non final fields (I made them final and realised that setters are problematic then). But then I asked for your opinion right away and didn't check if the setters are needed, now I now they are used.

and just keep constructor

Keep or add? I see only no or default constructors.

[nkrzykala]

Since Java 9 there is immutable collection. But we target java 8 as minimal java version so we can not use it and so we need to fall back with unmodifiable collection. (see Collections.unmodifiableList)

OK, I didn't know that, I'll look at this

[sbernard31]

Keep or add? I see only no or default constructors.

I didn't check the code but "keep if already exist" OR "add if needed"

now I know they are used.

Yep so we need to rewrite code which use them.

[sbernard31]

(@nkrzykala I integrated lot of PR recently do not forget to checkout last version of master before to work)