Add API to return licenses as a map in addition to list

[dwalluck]

I see that the license list is actually using a map internally, but the API returns a list Map::values(). Is it possible to add an API which also returns a Map with key String licenseId and value SpdxListedLicense? This would be more performant rather than having to recreate a map from the list.

[goneall]

Sounds like a useful API - if you'd like to create a PR, I can review merge. Otherwise, I'll take a look at this after implementing the SPDX 3.0 support.

[goneall]

@dwalluck - Finished with the SPDX 3 upgrade - can you point me to the API that returns the list?

[dwalluck]

@goneall Thanks for following up. As an aside, I quickly tried to test 2.0.0-RC1 and I got a fatal error loading the cached SPDX licenses:

[main] WARN  org.spdx.library.ListedLicenses - Unable to access the most current listed licenses from https://spdx.org/licenses - using locally cached licenses: No implementation found for SPDX spec version 3.0.1 Note: you can set the org.spdx.useJARLicenseInfoOnly property to true to avoid this warning.
[main] ERROR org.spdx.library.ListedLicenses - Error loading cached SPDX licenses

I no longer see an API which returns a list. But, there's also currently no API which returns a map.

I guess it comes down to a design choice here since SpdxListedLicenseModelStore already has a couple maps present, Map<String, LicenseJson> listedLicenseCache and Map<String, ExceptionJson> listedExceptionCache.

However, the ListedLicense* objects themselves are not cached and can only be created on the fly via ListedLicense::getListedLicenseById* or getListedExceptionById* which returns a new object every time these methods are called. Maybe you could also add a Map<String, ListedLicense>/Map<String, ListedLicenseException> in ListedLicense since this data is static.

[goneall]

I quickly tried to test 2.0.0-RC1 and I got a fatal error loading the cached SPDX licenses...

I wasn't able to duplicate the problem.

The warning is expected if you're not connected to the internet. The hard error, on the other hand, is completely unexpected.

I'll add some additional logging to see if we can find out what is wrong.

The code just reads the license information from resource files included in the Jar file.

[goneall]

@pmonks - any thoughts on the design approach?

[pmonks]

I'm very much in favour of a "map-based" representation of the license and exception lists held by the library and accessible (perhaps only as an unmodifiableMap, so callers can't modify that core data and mess things up?) via a public API; FWIW my code constructs exactly these maps itself so this would be a (welcome!) simplification of my code.

One thought: there may be runtime performance benefits for the library to make more use of such maps itself e.g. in various methods in the org.spdx.library.model.license.ListedLicenses class (note: I'm looking at the 1.1.12 version of the code right now - I believe that class has changed packages and/or names in v2.0). For example determining whether an id is valid, or retrieving an org.spdx.library.model.license.SpdxListedLicense instance for a given id. Of course these optimisations could come later - there's no hard requirement to gate this specific issue on making use of the new maps everywhere that makes sense internally.

[dwalluck]

I quickly tried to test 2.0.0-RC1 and I got a fatal error loading the cached SPDX licenses...

I wasn't able to duplicate the problem.

The warning is expected if you're not connected to the internet. The hard error, on the other hand, is completely unexpected.

I'll add some additional logging to see if we can find out what is wrong.

The code just reads the license information from resource files included in the Jar file.

I was definitely connected to the internet. Making the error message show the exception stack trace would be helpful.

[dwalluck]

I also changed to use all 1.0.0-RC2-SNAPSHOT versions, but it didn't make a difference. Maybe the project can benefit from a BOM since the two models, the core, and the library all seem to be on different versions of the same dependencies.

java.lang.RuntimeException: Unexpected error loading SPDX Listed Licenses
        at org.spdx.library.ListedLicenses.initializeLicenseModelStore(ListedLicenses.java:130)
        at org.spdx.library.ListedLicenses.<init>(ListedLicenses.java:77)
        at org.spdx.library.ListedLicenses.getListedLicenses(ListedLicenses.java:155)
Caused by: org.spdx.core.ModelRegistryException: No implementation found for SPDX spec version 3.0.1
        at org.spdx.core.ModelRegistry.typeToClass(ModelRegistry.java:194)
        at org.spdx.core.TypedValue.<init>(TypedValue.java:31)
        at org.spdx.storage.listedlicense.LicenseCreatorAgent.<init>(LicenseCreatorAgent.java:55)
        at org.spdx.storage.listedlicense.SpdxListedLicenseModelStore.<init>(SpdxListedLicenseModelStore.java:118)
        at org.spdx.storage.listedlicense.SpdxListedLicenseLocalStore.<init>(SpdxListedLicenseLocalStore.java:37)
        at org.spdx.library.ListedLicenses.initializeLicenseModelStore(ListedLicenses.java:127)
        ... 6 more

[dwalluck]

@goneall I tracked it down to missing calls to SpdxModelFactory.init() and DefaultModelStore.initialize() in my application (it apparently needs both). I am not sure about the proper arguments to DefaultModelStore.initialize() as neither of these methods used to be necessary. I believe that a call to ListedLicenses.getListedLicenses() took care of the initialization before so it looks like some sort of regression.

[goneall]

@goneall I tracked it down to missing calls to SpdxModelFactory.init() and DefaultModelStore.initialize() in my application (it apparently needs both). I am not sure about the proper arguments to DefaultModelStore.initialize() as neither of these methods used to be necessary. I believe that a call to ListedLicenses.getListedLicenses() took care of the initialization before so it looks like some sort of regression.

Yes - these are now required with the redesign to support SPDX 3.0.

I don't know if there is a better way to initialize the model registry so we don't have to call SpdxModelFactory.init(). It is a static method, so it only needs to be called once. The new design has a registry of supported model implementations which are implemented in external Jar files. We need to register those before any models are accessed. Let me know if you have any ideas on a better approach rather than calling SpdxModelFactory.init().

We probably could put back the default default model store initializations. At one point, we restricted the model store to only store one major version - in other words, you had to choose whether the default would store SPDX spec version 2.X exclusive or SPDX spec version 3.X. This is no longer true - you can now store both versions in the same model store - so the defaults can be added back.

[goneall]

I added #284 to track adding back a default store.

[goneall]

@dwalluck - We might need the SpdxModelFactory.init() to initialize the default model store. See #284 (comment)

[dwalluck]

@dwalluck - We might need the SpdxModelFactory.init() to initialize the default model store. See #284 (comment)

Actually, I thought it had something to do with the split between the core and the library, since it used to be set to new InMemSpdxStore() by default, which is now in the core. But, can't the library set this instead of the core now?

[goneall]

@dwalluck - We might need the SpdxModelFactory.init() to initialize the default model store. See #284 (comment)

Actually, I thought it had something to do with the split between the core and the library, since it used to be set to new InMemSpdxStore() by default, which is now in the core. But, can't the library set this instead of the core now?

It could, but the question is where would it be set. If we keep the SpdxModelFactory.init() - we could set it there.