joelittlejohn / jsonschema2pojo

Generate Java types from JSON or JSON Schema and annotate those types for data-binding with Jackson, Gson, etc
http://www.jsonschema2pojo.org
Apache License 2.0
6.22k stars 1.66k forks source link

Errorprone BoxedPrimitiveEquality error on generated code by jsonschema2pojo #1625

Open juherr opened 1 month ago

juherr commented 1 month ago

When using ErrorProne on my project using jsonschema2pojo, it complains about the following errors:

[BoxedPrimitiveEquality] Comparison using reference equality instead of value equality. Reference equality of boxed primitive types is usually not useful, as they are value objects, and it is bug-prone, as instances are cached for some values but not others.

((this.myInteger == rhs.myInteger)||((this.myInteger!= null)&&this.myInteger.equals(rhs.myInteger)))

According to ErrorProne, the generated code should be:

(Objects.equals(this.myInteger, rhs.myInteger))
unkish commented 4 weeks ago

Hi

I'd consider it as a false-positive and suggested change as incorrect.

Given following oversimplified schema:

{
  "type": "object",
  "javaType":"Example",
  "properties": {
    "myInteger": {
      "type": "integer"
    }
  }
}

And following code

System.out.println(new Example().equals(new Example()));

Code accoring to ErrorProne would yield false whereas one would expect it to be true.

juherr commented 4 weeks ago

@unkish Example is not a boxed primitive. The issue is about Integer, Double, Float, ...

Object equality must not change.

unkish commented 4 weeks ago

"type": "integer" can be replaced with "existingJavaType": "Integer" yielding same outcome whilst having same expectations

juherr commented 4 weeks ago

Yes and

System.out.println(new Integer(10).equals(new Integer(10)));

will return true as expected.

I still don't understand your point.

unkish commented 4 weeks ago

Schema above will generate code containing equals method

    @Override
    public boolean equals(Object other) {
        if (other == this) {
            return true;
        }
        if ((other instanceof Example) == false) {
            return false;
        }
        Example rhs = ((Example) other);
        return ((this.myInteger == rhs.myInteger)||((this.myInteger!= null)&&this.myInteger.equals(rhs.myInteger)));
    }

that contains "erroneous code" as reported by ErrorProne. My point is that leaving out this.myInteger == rhs.myInteger produces result that would differ from expected one.

juherr commented 4 weeks ago

@unkish Sorry for overlooking the double null case. Thank you for pointing it out. By the way, I have made the necessary updates to the request, and it is now functioning as expected thanks to Objects.equals.