Missing protection against duplicate id's

[nicoweidner]

Yet another technical issue incoming...

It is possible to create an instance of ModelObject using an id that is already in use. In that case, data in the model store will be untouched during instance creation, but will be changed when using the corresponding methods on the instance.
As an example, let's assume that id is not present in the model store before the following lines:

	Annotation annotation = new Annotation(modelStore, namespace, id, copyManager, true);
	annotation.setAnnotator("Person:annotator");
	SpdxFile file = new SpdxFile(modelStore, namespace, id, copyManager, true);
	file.setCopyrightText("copyrightText");

What happens is that the first line creates the corresponding StoredTypedItem in the model store, and the second line sets the annotator property. The third line creates a SpdxFile instance, but due to this part of the ModelObject constructor, nothing is changed in the model store. But the fourth line adds the copyrightText property, and we end up with a stored item having an inconsistent set of properties (some from Annotation, some from SpdxFile).

The only fairly quick fix I can think of is to change the semantics of the create boolean flag that is passed:

• If set to true, fail if an object with the given id already exists
• If set to false, fail if no object with the given id exists

Of course, this could break a bunch of existing code, depending on how it is used.

Remark: As @armintaenzertng pointed out to me, this is also the reason that some weird examples described in #112 (review) are possible, where objects of different classes are erroneously considered equal.

[goneall]

Yikes. A bit of a design flaw. Thanks @nicoweidner for pointing this out. This is probably a root cause of a few issues.

The change you proposed is very logical, but as you point out it is potentially breaking since the semantics.

I wonder if there may be another interface option that would preserve the existing semantics.

I was thinking of using the strict flag to force errors is the ID already exists, however, currently it can only be set after the ModelObject is created.

[goneall]

One thought is to add a new constructor with different semantics and deprecate the old one. We could add a strict parameter, for example. It would require, however, updating a lot of code.

The idea behind the strict parameter is, if it is set to true then the call will fail if the ID already exists. Otherwise, it would behave the way it does today. This is similar to how the library treats other checks on parameter validity.

I have mixed feeling about whether the strict approach is a good one. @nicoweidner - let me know what you think.

[goneall]

@nicoweidner I came up with another (partial) solution we could implement with low risk.

If create is true and there is another ModelObject in the store with the same ID, check to see if the type of the ModelObject in the store is the same as type type of the newly created ModelObject. We could fail if they are different types.

This would preserve the semantics of re-using an ModelObject even if create is true. It would prevent the non-symentric equals behavior mentioned in PR 112 comments.

I'm pretty comfortable no one would intentionally create a ModelObject with a different type expecting it to reuse the properties of a previous type.

The only case this doesn't cover is if someone accidentally creates 2 ModelObjects of the same type with the same ID.

[nicoweidner]

@goneall Given the current situation, I prefer your latest suggestion of failing when someone tries to create an object with the same id, but of different type. That should prevent all the bad scenarios, and is little effort.

[goneall]

Fixed (mostly) with PR #121