[Issue 105] Fix ModelObject#equivalent not being symmetric

[nicoweidner]

Fixes #105
Note: After making my changes, ModelObjectTest#testClone failed. I removed the line testing that result.equivalent(gmo) because my current understanding is that clone only creates a copy with the same identifiers in a new store, but without the properties populated; copyFrom seems to be the method to use for copying properties. Hence the check that the objects are equivalent is misplaced in that test (and wasn't noticed because of the bug fixed in this PR)

Signed-off-by: Nicolaus Weidner nicolaus.weidner@tngtech.com

[goneall]

@nicoweidner Thanks for the PR!

For the clone function, I would still expect the 2 objects to be equivalent. The clone function does copy the properties. The difference between clone and copyFrom is that copyFrom does not create a new ModelObject, it copies properties from a different ModelObject to itself. Do we know why the call to equivalent is failing (or working prior to the change)?

[nicoweidner]

@nicoweidner Thanks for the PR!

For the clone function, I would still expect the 2 objects to be equivalent. The clone function does copy the properties. The difference between clone and copyFrom is that copyFrom does not create a new ModelObject, it copies properties from a different ModelObject to itself. Do we know why the call to equivalent is failing (or working prior to the change)?

@goneall Now I am confused again about clone 😅 . My reasoning was this: In the clone method, a new ModelObject is created using a different store. Following the code, this just calls the corresponding constructor with the store that was passed in. So all properties of the new cloned object will be taken from the model store that was passed in, not from the model store of the original object. Am I wrong here?

The test testClone was failing becaues the two objects were not equivalent at all. The input object gmo has 23 properties which are populated in this line. The cloned object result has no properties at all. This strengthened my belief that the conclusion above was correct...

The reason the test passed so far is exactly the bug fixed in this PR: The assertion was written in the form result.equivalent(gmo). In this case, properties only present on gmo were simply ignored during the comparison.

On a different note, I noticed one bug I introduced (erroneous early return) and several test failures (I only executed ModelObjectTest locally 😳 ). I will fix those. I assume it is okay to squash and force-push on feature branches? Then I would squash it into 4ca128f because it should have been included there in the first place.

[nicoweidner]

I pushed some changes that should fix CI. In my effort to simplify the logic, I accidentally broke some edge cases with my previous commit 😬 . Luckily, some test caught it; I also added specific unit tests for those cases (empty model collections and noassertion values, which are treated like null in comparisons).

As mentioned before, the additional 4 commits should all be part of the initial commit. @goneall I will wait for a quick opinion before squashing them and force-pushing.

[nicoweidner]

@goneall One further question: I noticed that in the propertyValuesEquivalent method (which is called when comparing two ModelObjects for equivalence), NOASSERTION is not considered. Is this by mistake?

This would only be relevant in specific situations I believe (and I can't tell at this point whether they can actually occur, but still...). Let's say we are comparing property1 between object1 and object2:

• first value is NOASSERTION, second value is missing (this shouldn't occur since model store reported a nonempty value, but we still check for it)
• first value is NOASSERTION, second value is an empty ModelCollection (not sure if NOASSERTION is valid when the property contains a list)

[goneall]

My reasoning was this: In the clone method, a new ModelObject is created using a different store. Following the code, this just calls the corresponding constructor with the store that was passed in. So all properties of the new cloned object will be taken from the model store that was passed in, not from the model store of the original object. Am I wrong here?

@nicoweidner You're correct - I just looked at the code and indeed it doesn't copy the properties. I think this is yet another bug :( It can be fixed by copying the properties in the cloned object. In thinking about this, the copyManager must be provided for this to work, so maybe something like:

		if (Objects.isNull(this.copyManager)) {
			throw new RuntimeException("A copy manager must be provided to clone");
		}
		try {
			ModelObject retval = SpdxModelFactory.createModelObject(modelStore, this.documentUri, this.id, this.getType(), this.copyManager);
			retval.copyFrom(this);
			return retval;
		} catch (InvalidSPDXAnalysisException e) {
			throw new RuntimeException(e);
		}

It's been a while since I've worked on this method, but I'm pretty sure I intended the properties to be copied. I did a search in all my local projects and couldn't find any instance of clone being used, so perhaps we should just deprecate this method?

I assume it is okay to squash and force-push on feature branches?

Yes - that would be preferred.

Thanks!

[nicoweidner]

I created #107 to discuss the clone issue. Then I would leave this PR as is, with the equivalent check removed from the test for clone. The linked issue can add it back in if we decide to change the clone method.

[nicoweidner]

Ok, commits are squashed, should be good to go now unless someone finds another bug :-)

(the small refactor commit was squashed into the others since it would have been some effort to separate it, and it only changed a few lines anyway)

[goneall]

Thanks @nicoweidner