Comparing IndividualUriValues in ModelObject#listsEquivalent may be broken? about spdx-java-library HOT 4 CLOSED

@nicoweidner Excellent catch - thanks for the review!

For most of the cases, the previous check for contains will return true even if it is a IndividualUriValue. The extra check is primarily for external references (the enum implementations will likely be caught by the previous check).

I'm thinking we could fix this specific issue by changing the class check from:

if (!(comp instanceof SimpleUriValue)) {

to:

if (!(comp instanceof IndividualUriValue)) {

Looking further, some of the other implementations of IndividualUriValue should also override equals and hashcode to use the URI value so that it will be equal to a SimpleUriValue with the same URI.

Probably needs a bit more review and thought before committing the changes - let me know what you think.

As far as making the check symmetric - I agree this is a possible issue.

from spdx-java-library.

After thinking about this a bit more, I'm pretty convinced anything that implements IndividualUriValue should override equals/hashcode to use the URI value for equality. The one exception is is the class implementing IndividualUriValue inherits from ModelObject which already overrides equals using the ID for equality.

@nicoweidner I'll create a PR with the changes for this. If you have some time, feel free to tackle the symmetric issue.

from spdx-java-library.

@nicoweidner I created a PR to update equals for IndividualUriValues.

In thinking about this some more, SimpleUriValue is typically only used when storing a property - when it is re-inflated back to a model object, it is typically cast to the appropriate type. So this it will not likely cause a problem in practice. That being said, it is a bug since the SimpleUriValue may be used for other purposes and the semantics of IndividaulUriValue should be equal if the URI's are equal.

I did go ahead and update the ModelObjects that implement IndividualUriValue to override equals.

The only implemenations of IndividualUriValue I did not update was the enumerations. Since the equals method in enums are final, I could not override the method.

Please check out the PR and let me know if my logic and updates make sense.

from spdx-java-library.

For most of the cases, the previous check for contains will return true even if it is a IndividualUriValue. The extra check is primarily for external references (the enum implementations will likely be caught by the previous check).

Ah, you are right that the previous check should catch various IndividualUriValue implementations. I completely missed that when looking at the two lines about IndividualUriValue in isolation 😅 . In the end, it boils down to how the equals method works in each case. All enum implementations should be fine.

I will think about how the other cases behave. It probably makes more sense to comment directly on the PR since you already made changes there, so I will do that.

from spdx-java-library.