Skip to content

Commit 5d9fbc5

Browse files
author
Nicolaus Weidner
committed
[issue-105] Fix ModelObject#equivalent not being symmetric
Signed-off-by: Nicolaus Weidner <[email protected]>
1 parent 90f7f33 commit 5d9fbc5

File tree

2 files changed

+102
-39
lines changed

2 files changed

+102
-39
lines changed

‎src/main/java/org/spdx/library/model/ModelObject.java‎

Lines changed: 48 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -766,7 +766,7 @@ public boolean equivalent(ModelObject compare, boolean ignoreRelatedElements) th
766766
List<String> propertyValueNames = getPropertyValueNames();
767767
List<String> comparePropertyValueNames = new ArrayList<String>(compare.getPropertyValueNames()); // create a copy since we're going to modify it
768768
for (String propertyName:propertyValueNames) {
769-
if (ignoreRelatedElements && SpdxConstants.PROP_RELATED_SPDX_ELEMENT.equals(propertyName)) {
769+
if (ignoreRelatedElements && isRelatedElement(propertyName)) {
770770
continue;
771771
}
772772
if (comparePropertyValueNames.contains(propertyName)) {
@@ -779,36 +779,59 @@ public boolean equivalent(ModelObject compare, boolean ignoreRelatedElements) th
779779
comparePropertyValueNames.remove(propertyName);
780780
} else {
781781
// No property value
782-
Optional<Object> propertyValue = this.getObjectPropertyValue(propertyName);
783-
if (propertyValue.isPresent()) {
784-
if (propertyValue.get() instanceof ModelCollection) {
785-
if (((ModelCollection<?>)propertyValue.get()).size() > 0) {
786-
lastNotEquivalentReason = new NotEquivalentReason(
787-
NotEquivalent.COMPARE_PROPERTY_MISSING, propertyName);
788-
return false;
789-
}
790-
} else if (!propertyNoAssertion(propertyValue)) {
791-
lastNotEquivalentReason = new NotEquivalentReason(
792-
NotEquivalent.COMPARE_PROPERTY_MISSING, propertyName);
793-
return false;
782+
Optional<Object> propertyValueOptional = this.getObjectPropertyValue(propertyName);
783+
if (propertyValueOptional.isPresent()) {
784+
Object propertyValue = propertyValueOptional.get();
785+
if (isEquivalentToNull(propertyValue)) {
786+
continue;
794787
}
788+
lastNotEquivalentReason = new NotEquivalentReason(
789+
NotEquivalent.COMPARE_PROPERTY_MISSING, propertyName);
790+
return false;
795791
}
796792
}
797793
}
798-
for (String propertyName:comparePropertyValueNames) { // check any remaining property values
799-
if (!compare.getObjectPropertyValue(propertyName).isPresent()) {
800-
lastNotEquivalentReason = new NotEquivalentReason(
801-
NotEquivalent.MISSING_PROPERTY, propertyName);
802-
return false;
794+
for (String propertyName:comparePropertyValueNames) { // check any remaining property values
795+
if (ignoreRelatedElements && isRelatedElement(propertyName)) {
796+
continue;
803797
}
798+
Optional<Object> comparePropertyValueOptional = compare.getObjectPropertyValue(propertyName);
799+
if (!comparePropertyValueOptional.isPresent()) {
800+
continue;
801+
}
802+
Object comparePropertyValue = comparePropertyValueOptional.get();
803+
if (isEquivalentToNull(comparePropertyValue)) {
804+
continue;
805+
}
806+
lastNotEquivalentReason = new NotEquivalentReason(
807+
NotEquivalent.MISSING_PROPERTY, propertyName);
808+
return false;
804809
}
805810
return true;
806811
}
807812

808-
boolean propertyNoAssertion(Optional<Object> propertyValue) {
809-
return propertyValue.isPresent() &&
810-
(propertyValue.get() instanceof SpdxNoAssertionLicense ||
811-
propertyValue.get().equals("NOASSERTION"));
813+
// Some values are treated like null in comparisons - in particular empty model collections and
814+
// "no assertion" values.
815+
private boolean isEquivalentToNull(Object propertyValue) {
816+
if (propertyValue instanceof ModelCollection) {
817+
return ((ModelCollection<?>) propertyValue).size() == 0;
818+
} else {
819+
return isNoAssertion(propertyValue);
820+
}
821+
}
822+
823+
private boolean isRelatedElement(String propertyName) {
824+
return SpdxConstants.PROP_RELATED_SPDX_ELEMENT.equals(propertyName);
825+
}
826+
827+
private boolean isEmptyModelCollection(Object value) {
828+
return (value instanceof ModelCollection)
829+
&& (((ModelCollection<?>) value).size() == 0);
830+
}
831+
832+
private boolean isNoAssertion(Object propertyValue) {
833+
return propertyValue instanceof SpdxNoAssertionLicense ||
834+
propertyValue.equals(SpdxConstants.NOASSERTION_VALUE);
812835
}
813836

814837
/**
@@ -823,22 +846,10 @@ private boolean propertyValuesEquivalent(String propertyName, Optional<Object> v
823846
Optional<Object> valueB, boolean ignoreRelatedElements) throws InvalidSPDXAnalysisException {
824847
if (!valueA.isPresent()) {
825848
if (valueB.isPresent()) {
826-
if (valueB.get() instanceof ModelCollection) {
827-
if (((ModelCollection<?>)valueB.get()).size() > 0) {
828-
return false;
829-
}
830-
} else {
831-
return false;
832-
}
849+
return isEmptyModelCollection(valueB.get());
833850
}
834851
} else if (!valueB.isPresent()) {
835-
if (valueA.get() instanceof ModelCollection) {
836-
if (((ModelCollection<?>)valueA.get()).size() > 0) {
837-
return false;
838-
}
839-
} else {
840-
return false;
841-
}
852+
return isEmptyModelCollection(valueA.get());
842853
} else if (valueA.get() instanceof ModelCollection && valueB.get() instanceof ModelCollection) {
843854
List<?> myList = ((ModelCollection<?>)valueA.get()).toImmutableList();
844855
List<?> compareList = ((ModelCollection<?>)valueB.get()).toImmutableList();
@@ -856,7 +867,7 @@ private boolean propertyValuesEquivalent(String propertyName, Optional<Object> v
856867
// Note: we must check the IndividualValue before the ModelObject types since the IndividualValue takes precedence
857868
} else if (valueA.get() instanceof ModelObject && valueB.get() instanceof ModelObject) {
858869
if (!((ModelObject)valueA.get()).equivalent(((ModelObject)valueB.get()),
859-
SpdxConstants.PROP_RELATED_SPDX_ELEMENT.equals(propertyName) ? true : ignoreRelatedElements)) {
870+
isRelatedElement(propertyName) ? true : ignoreRelatedElements)) {
860871
return false;
861872
}
862873

‎src/test/java/org/spdx/library/model/ModelObjectTest.java‎

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ public class ModelObjectTest extends TestCase {
5656
private static final String TEST_ID = "testId";
5757
private static final Object TEST_VALUE1 = "value1";
5858
private static final String TEST_PROPERTY1 = "property1";
59+
private static final String TEST_PROPERTY2 = "property2";
5960
static final String TEST_TYPE1 = SpdxConstants.CLASS_SPDX_LICENSE_EXCEPTION;
6061
static final String TEST_TYPE2 = SpdxConstants.CLASS_SPDX_EXTRACTED_LICENSING_INFO;
6162

@@ -573,6 +574,58 @@ public void testEquivalent() throws InvalidSPDXAnalysisException {
573574
assertTrue(gmo3.equivalent(gmo2));
574575
}
575576

577+
// We test symmetry in case of missing properties on either side explicitly because of issue-105
578+
public void testEquivalentIsSymmetric() throws InvalidSPDXAnalysisException {
579+
// Given
580+
GenericModelObject firstObject = new GenericModelObject(store, docUri, TEST_ID, copyManager, true);
581+
GenericModelObject secondObject = new GenericModelObject(store, docUri, "testId2", copyManager, true);
582+
String testValue2 = "value2";
583+
firstObject.setPropertyValue(TEST_PROPERTY1, TEST_VALUE1);
584+
secondObject.setPropertyValue(TEST_PROPERTY1, TEST_VALUE1);
585+
586+
// Then
587+
assertTrue(firstObject.equivalent(secondObject) && secondObject.equivalent(firstObject));
588+
589+
// Given
590+
firstObject.setPropertyValue(TEST_PROPERTY2, testValue2);
591+
592+
// Then
593+
assertFalse(firstObject.equivalent(secondObject));
594+
assertFalse(secondObject.equivalent(firstObject));
595+
596+
// Given
597+
firstObject.removeProperty(TEST_PROPERTY2);
598+
secondObject.setPropertyValue(TEST_PROPERTY2, testValue2);
599+
600+
// Then
601+
assertFalse(firstObject.equivalent(secondObject));
602+
assertFalse(secondObject.equivalent(firstObject));
603+
}
604+
605+
public void testEquivalentIgnoresEmptyModelCollections() throws InvalidSPDXAnalysisException {
606+
// Given
607+
GenericModelObject firstObject = new GenericModelObject(store, docUri, TEST_ID, copyManager, true);
608+
GenericModelObject secondObject = new GenericModelObject(store, docUri, "testId2", copyManager, true);
609+
ModelCollection<GenericModelObject> emptyModelCollection = new ModelCollection<>(store,
610+
docUri, TEST_ID, TEST_PROPERTY1, copyManager, GenericModelObject.class);
611+
firstObject.setPropertyValue(TEST_PROPERTY1, emptyModelCollection);
612+
secondObject.setPropertyValue(TEST_PROPERTY2, emptyModelCollection);
613+
614+
// Then
615+
assertTrue(firstObject.equivalent(secondObject));
616+
}
617+
618+
public void testEquivalentIgnoresNoAssertion() throws InvalidSPDXAnalysisException {
619+
// Given
620+
GenericModelObject firstObject = new GenericModelObject(store, docUri, TEST_ID, copyManager, true);
621+
GenericModelObject secondObject = new GenericModelObject(store, docUri, "testId2", copyManager, true);
622+
firstObject.setPropertyValue(TEST_PROPERTY1, new SpdxNoAssertionLicense());
623+
secondObject.setPropertyValue(TEST_PROPERTY2, SpdxConstants.NOASSERTION_VALUE);
624+
625+
// Then
626+
assertTrue(firstObject.equivalent(secondObject));
627+
}
628+
576629
public void testEquivalentModelObjectProp() throws InvalidSPDXAnalysisException {
577630
GenericModelObject gmo = new GenericModelObject(store, docUri, TEST_ID, copyManager, true);
578631
String id1 = "id1";
@@ -661,8 +714,7 @@ public void testClone() throws InvalidSPDXAnalysisException {
661714
InMemSpdxStore store2 = new InMemSpdxStore();
662715
ModelObject result = gmo.clone(store2);
663716
assertTrue(result instanceof GenericModelObject);
664-
assertTrue(result.equals(gmo));
665-
assertTrue(result.equivalent(gmo));
717+
assertEquals(result, gmo);
666718
}
667719

668720
/**

0 commit comments

Comments
 (0)