Inconsistent behavior when reading filesAnalyzed of a package that does not define it explicitly

[nicoweidner]

I ran into a super dubious lazy-loading issue (supposedly), related to reading filesAnalyzed from a document deserialized from a json file that did not set this property.

I tried to verify that conversion from tag/value to json using the Python tools does not change any properties. To that end, I wrote a simple test in spdx-testbed:

  @Test
  public void detectDifferences()
      throws IOException, InvalidFileNameException, InvalidSPDXAnalysisException {
    var convertedFile = SpdxToolsHelper.deserializeDocument(new File("converted.json"));
    var originalFile = SpdxToolsHelper.deserializeDocument(new File("SPDXTagExample-v2.2.spdx"));
    var differences = Comparisons.findDifferencesInSerializedJson(convertedFile, originalFile);
    assertThat(differences).isEmpty();
  }

This test failed, because filesAnalyzed was set to true for the document deserialized from tag/value (named originalFile here), while it was unspecified for the document deserialized from json (named convertedFile).

I first suspected that the json reader does not set the default of true while the tag/value reader does.
This may be one issue, but I ran into something weirder: When setting a breakpoint on the var differences = ... line and evaluating the package in question via

convertedFile.getModelStore().getTypedValue(convertedFile.getDocumentUri(), "SPDXRef-Package")

, the filesAnalyzed value actually came back as true, and the test subsequently passed. This feels like some strange lazy-loading issue, but I can't debug anymore because it stopped behaving like this; I can't reproduce this behavior anymore and the test now consistently fails on my machine.

So...

• filesAnalyzed should probably be set to true as default, like the spec says
• I am utterly flabberghasted regarding the lazy loading issue

For completeness sake, this is the json file that I used. It was created by converting the 2.2 tag/value example for spdx-spec to json using the Python tools. I think most of it is irrelevant though, the important part is that it contains a package glibc which does not have a value for filesAnalyzed.
converted.zip

[goneall]

Very strange lazy loading issue. Assuming you're using the default InMemSpdxStore as the baseStore, then the values are just stored and fetched from a HashMap. There could be a concurrency problem if more than one thread is accessing the store at the same time - these need to be controlled external from the store by calling the enterCriticalSection and leaveCriticalSection methods.

[goneall]

It looks like the fileAnalyzed default is set in model class SpdxPackage:

Spdx-Java-Library/src/main/java/org/spdx/library/model/SpdxPackage.java

Line 57 in cd1b91d

public SpdxPackage() throws InvalidSPDXAnalysisException {

The isFilesAnalyzed also returns a default if not present:

Spdx-Java-Library/src/main/java/org/spdx/library/model/SpdxPackage.java

Line 148 in cd1b91d

public boolean isFilesAnalyzed() throws InvalidSPDXAnalysisException {

... very curious ...

[nicoweidner]

Very strange lazy loading issue. Assuming you're using the default InMemSpdxStore as the baseStore, then the values are just stored and fetched from a HashMap. There could be a concurrency problem if more than one thread is accessing the store at the same time - these need to be controlled external from the store by calling the enterCriticalSection and leaveCriticalSection methods.

Yeah, the SpdxToolsHelper in the code above is from tools-java, and for json it instantiates the store like this, nothing fancy going on:

new MultiFormatStore(new InMemSpdxStore(), Format.JSON_PRETTY, Verbose.COMPACT);

I was already wondering whether concurrency plays a role here because of the inconsistent behavior (saw some locks...), but there is nothing going on except for the few basic lines above.
I started seriously doubting myself since I cannot even reproduce the observed lazy-loading behavior anymore (the property suddenly showing up when querying in the debugger). But I did see it repeatedly before (some cases in pairing with @armintaenzertng - I have a witness 😅 ), and in those cases the test turned out green.

[nicoweidner]

It looks like the fileAnalyzed default is set in model class SpdxPackage:

This gave me an idea: Do I understand correctly that the value is initialized when SpdxPackage is instantiated? I suspect in the usage above, it may not ever be instantiated - instead the values are just queried from the model store, leading to filesAnalyzed still being null.

In fact, with this idea in mind, I went ahead and did the following:

• As before, set a breakpoint in line var differences = ..., after the two documents are deserialized, but before anything else happens
• Execute the test. At the breakpoint, first query the package directly from the model store:

convertedFile.getModelStore().getTypedValue(convertedFile.getDocumentUri(), "SPDXRef-Package")

• As expected, it does not have a filesAnalyzed property. Now instantiate a SpdxPackage from the stored values:

SpdxModelFactory.createModelObject(convertedFile.getModelStore(), convertedFile.getDocumentUri(), "SPDXRef-Package", "Package", convertedFile.getCopyManager())

• Then query the package from the model store again using the first command. This time, filesAnalyzed is set to true

This works reliably for me. And it makes the test green, when it would be red without evaluating anything in the debugger.

This is certainly unexpected behavior imho, and it appears to happen for MultiFormatStore, but not TagValueStore.

[goneall]

I suspect in the usage above, it may not ever be instantiated - instead the values are just queried from the model store, leading to filesAnalyzed still being null.

I'm sure that's it - mystery solved!

This is certainly unexpected behavior imho, and it appears to happen for MultiFormatStore, but not TagValueStore.

The store operates independent of any semantic interpretation (such as default values), so I would not expect the default value to be taken care of by the model.

I also don't think the serializers / deserializers should do any semantic interpretation - it should just store the values as is. So the TagValueStore is not behaving as expected imo.

There is one behavior which I think is wrong, however. I don't think the model should actually update the filesAnalyzed in the store when "setting the default". Rather, it should just return the default value if the filesAnalyzed were not set at all. Storing the default value modifies a value when there was no explicit setting of the value. I think I can just remove the setting of the values from the SpdxPackageManager since the isFilesAnalyzed already handles the default if there is no value specified.

[nicoweidner]

The store operates independent of any semantic interpretation (such as default values), so I would not expect the default value to be taken care of by the model.

I also don't think the serializers / deserializers should do any semantic interpretation - it should just store the values as is. So the TagValueStore is not behaving as expected imo.

I get where you are coming from here, I think that's a valid point. I was about to complain about troubles with the testbed, but actually, as long as the different stores behave the same, there would be no issue at all.

There is one behavior which I think is wrong, however. I don't think the model should actually update the filesAnalyzed in the store when "setting the default". Rather, it should just return the default value if the filesAnalyzed were not set at all. Storing the default value modifies a value when there was no explicit setting of the value. I think I can just remove the setting of the values from the SpdxPackageManager since the isFilesAnalyzed already handles the default if there is no value specified.

Yeah, it's definitely unexpected that instantiating the package in question as SpdxPackage modifies values in the model store as a side effect.

[goneall]

Transferring this issue to the SPDX Java Library. I'll create a PR to stop updating the isFilesAnalyzed flag - this should fix the inconsistent behavior between the Tag/Value and Jackson stores.