-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Override equals for IndividualUriValues #112
Conversation
This will cause any individual URI values to be equal if their URI's are equal. Fixes #108 Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@nicoweidner - Let me know if this change looks reasonable - I'll squash the commits before merging. Also - I'm thinking about spinning a release in the next couple of days with the fixes, let me know if there are any issues you think need to be fixed before the release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to understand how the classes relate. Assuming that any IndividualUriValue
is wholly defined by its individual URI (and all other properties are irrelevant/unused), I think these changes make sense.
There are some technical cases that make me feel uneasy, though (I am a mathematician, I am always looking for pathological cases):
- There are some cross-combinations where e.g. a
ExternalExtractedLicenseInfo
could be equal to aExternalSpdxElement
if their URIs agree. I think this cannot actually happen because of format requirements on their ids. I didn't check the patterns or combinations exhaustively. - A
IndividualUriValue
can never be equal to aModelObject
that is not an instance ofIndividualUriValue
- but aModelObject
can be equal to aIndividualUriValue
if their ids agree (in cases where a class extends both).
The first one might actually be considered fine. If the URI is the defining property, then even if the situation is possible or becomes possible by future changes, I can see it being fine to consider such elements equal.
The second one seems problematic though. It is breaking symmetricity of equals
(I am all about symmetricity these days 😀 ), and this scenario is actually possible if one picks somewhat artificial ids. I added the following lines to your first test case, and it passed, accepting the annotation as equal to the license info:
ExternalExtractedLicenseInfo eel = new ExternalExtractedLicenseInfo(modelStore, namespace, externalDocId + ":" + id, copyManager, true);
Annotation annotation = new Annotation(modelStore, namespace, "DocumentRef-externalDoc:LicenseRef-ID", copyManager, true);
assertEquals(annotation, eel);
I wonder if this could be solved by adding a more precise class check in ModelObject#equals
, i.e.: Instead of
if (!(o instanceof ModelObject)) {
return false;
}
use
if (o == null || o.getClass() == this.getClass()) {
return false;
}
Do you see any cases (except IndividualUriValue
) where instances of ModelObject
should be considered equal even though they are not instances of the exact same class?
src/main/java/org/spdx/library/model/license/ExternalExtractedLicenseInfo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/spdx/library/model/license/SpdxNoAssertionLicense.java
Outdated
Show resolved
Hide resolved
src/main/java/org/spdx/library/model/license/SpdxNoneLicense.java
Outdated
Show resolved
Hide resolved
@Override | ||
public boolean equals(Object compare) { | ||
if (!(compare instanceof IndividualUriValue)) { | ||
return false; | ||
} | ||
return ((IndividualUriValue)compare).getIndividualURI().equals(documentNamespace); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return documentNamespace.hashCode(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExternalDocumentRef
does not implement IndividualUriValue
, so requiring the comparison object to be an instance of IndividualUriValue
feels wrong. Was this added by accident, or is there some implicit logic relating the two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - I believe this is was an incorrect addition. I'll update the PR removing this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In looking back at the code, the override is for an IndividualUriValue
created inline for a property value, so I believe the code is correct:
setPropertyValue(SpdxConstants.PROP_EXTERNAL_SPDX_DOCUMENT,
new IndividualUriValue() {
@Override ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you are totally right, I didn't look close enough 😳
But then I wonder why a SimpleUriValue
with documentNamespace
as uri isn't used here instead of the anonymous class? I assumed that SimpleUriValue
was meant as a generic implementing class of IndividualUriValue
, and I believe it would behave the same (apart from a technical difference in hashCode
that shouldn't matter)
@nicoweidner Thanks for the extensive review. Below are a few responses:
This cannot happen if the spec is followed since the ID's must have different prefixes. The code that re-inflates the objects from the individuals uses the unique prefixes to build the correct object, so it can not happen in practice either.
I can't think of any specific cases, but I would be nervous about making this assumption. Similar to There is a fundamental assumption in the spec (and carried forward in the code) that the ID's are unique for each element/ModelObject. Rather than changing equals in
If you assume that ID's are unique, would the equals be considered symmetric? |
Yeah, I think this case is fine.
Do you mean you are hesitant to change the
Do you consider one model store as basically the same as one document? Because technically, it would check for
Haha... I started writing a similar comment in response to your "id's are expected to be unique" statement, then realized you had already written basically the same thing. Offtopic remark: Thanks for being patient in your explanations of the backgrounds! As I'm still missing a lot of context knowledge, being pedantic helps me understand the precise relationships between objects. |
Yes - I'm a bit hesitant since the namespace + ID's must be unique and if the namespace + ID is the same, then the should be equal - even if they are instantiated in different classes. BTW - if everything is working properly, than the classes shouldn't be different. I would just want that namespace + ID to take precedence on equality.
Typically, there is one document in one model store, but the design allows for multiple document URI's and multiple documents in the same store. Most
This would be my preferred approach. There is a field
No worries - I'm very appreciative of the review and comments. It really helps build a better library. |
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
This reverts commit 7b78b28. Commit was added to the incorrect branch
|
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@nicoweidner I did some refactoring of the common code for Unless you see any major issues, I'll merge this after it passes the CI. I'll submit another PR that should help with the symmetric equals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only left some small comments, looks good to me now!
This will cause any individual URI values to be equal if their URI's are equal.
Fixes #108
Signed-off-by: Gary O'Neall gary@sourceauditor.com