Skip to content

Make ListedLicenses more user configurable #211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
goneall merged 2 commits into from
Oct 18, 2023
Merged

Make ListedLicenses more user configurable #211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bca63f
Make ListedLicenses more user configurable
loosebazooka Oct 13, 2023
67dc8c6
Add src/test/resources resource directory
loosebazooka Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,12 @@
<exclude>**/*.java</exclude>
</excludes>
</testResource>
<testResource>
<directory>src/test/resources</directory>
<excludes>
<exclude>**/*.java</exclude>
</excludes>
</testResource>
<testResource>
<filtering>false</filtering>
<directory>TestFiles</directory>
Expand Down
29 changes: 27 additions & 2 deletions src/main/java/org/spdx/library/model/license/ListedLicenses.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ private ListedLicenses() {
initializeLicenseModelStore();
}

/**
* This constructor should only be called by the initializeListedLicenses method,
* to programmatically configure licenseModelStore from the application consuming this library
*/
private ListedLicenses(IListedLicenseStore licenseModelStore) {
this.licenseModelStore = licenseModelStore;
}

private void initializeLicenseModelStore() {
listedLicenseModificationLock.writeLock().lock();
try {
Expand All @@ -89,7 +97,6 @@ private void initializeLicenseModelStore() {
}

public static ListedLicenses getListedLicenses() {

ListedLicenses retval = null;
listedLicenseModificationLock.readLock().lock();
try {
Expand All @@ -110,7 +117,25 @@ public static ListedLicenses getListedLicenses() {
}
return retval;
}


/**
* Initializes the listed licenses singleton from a provided cache. This will
* ignore all configuration around fetching remote licenses.
*
* @param licenseStore a preconfigured licenseStore, see {@link SpdxListedLicenseLocalStore} for
* an example.
* @return a singleton instance
*/
public static ListedLicenses initializeListedLicenses(IListedLicenseStore licenseStore) {
listedLicenseModificationLock.writeLock().lock();
try {
listedLicenses = new ListedLicenses(licenseStore);
return listedLicenses;
} finally {
listedLicenseModificationLock.writeLock().unlock();
}
}

/**
* Resets all of the cached license information and reloads the license IDs
* NOTE: This method should be used with caution, it will negatively impact
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/org/spdx/storage/listedlicense/SpdxListedLicenseLocalStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public SpdxListedLicenseLocalStore() throws InvalidSPDXAnalysisException {
}

@Override
InputStream getTocInputStream() throws IOException {
public InputStream getTocInputStream() throws IOException {
String fileName = LISTED_LICENSE_JSON_LOCAL_DIR + "/" + LICENSE_TOC_FILENAME;
InputStream retval = SpdxListedLicenseLocalStore.class.getResourceAsStream("/" + fileName);
if (retval == null) {
Expand All @@ -48,7 +48,7 @@ InputStream getTocInputStream() throws IOException {
}

@Override
InputStream getLicenseInputStream(String licenseId) throws IOException {
public InputStream getLicenseInputStream(String licenseId) throws IOException {

String fileName = LISTED_LICENSE_JSON_LOCAL_DIR + "/" + licenseId + JSON_SUFFIX;
InputStream retval = SpdxListedLicenseLocalStore.class.getResourceAsStream("/" + fileName);
Expand All @@ -59,7 +59,7 @@ InputStream getLicenseInputStream(String licenseId) throws IOException {
}

@Override
InputStream getExceptionTocInputStream() throws IOException {
public InputStream getExceptionTocInputStream() throws IOException {
String fileName = LISTED_LICENSE_JSON_LOCAL_DIR + "/" + EXCEPTION_TOC_FILENAME;
InputStream retval = SpdxListedLicenseLocalStore.class.getResourceAsStream("/" + fileName);
if (retval == null) {
Expand All @@ -69,7 +69,7 @@ InputStream getExceptionTocInputStream() throws IOException {
}

@Override
InputStream getExceptionInputStream(String exceptionId) throws IOException {
public InputStream getExceptionInputStream(String exceptionId) throws IOException {
return getLicenseInputStream(exceptionId);
}

Expand Down
8 changes: 4 additions & 4 deletions src/main/java/org/spdx/storage/listedlicense/SpdxListedLicenseModelStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,25 +114,25 @@ public SpdxListedLicenseModelStore() throws InvalidSPDXAnalysisException {
* @return InputStream for the Table of Contents of the licenses formated in JSON SPDX
* @throws IOException
*/
abstract InputStream getTocInputStream() throws IOException;
public abstract InputStream getTocInputStream() throws IOException;
Copy link
Contributor Author

@loosebazooka loosebazooka Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were made public so consumers of this library could implement this. Although I'm not sure if there's a better way to expose that ability.

Copy link
Collaborator

@pmonks pmonks Oct 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've wondered about the feasibility of enhancing Spdx-Java-Library to be more dependency injection (Spring etc.) friendly, e.g. by using interfaces more liberally and having configuration etc. passed in via constructor params and/or setters. It would be a big change to the structure of the library though, and I don't know the functionality well enough yet to see how it might be accomplished without substantially breaking backwards compatibility.

I only mention this as such a change should make what you're attempting to do a lot easier, by (mostly) just involving implementing simple interfaces then wiring your custom implementations in via DI (or vanilla class instantiation code, if you don't use a DI framework).

But short of making such a change, what you've done here seems reasonable to me, and most of the risk/consequences of making these methods public will fall on anyone who is developing custom IListedLicenseStore implementations (which is where they should be).

Copy link
Contributor Author

@loosebazooka loosebazooka Oct 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I necessarily need dependency injection. But it might make sense to move off the global Singleton and go for more compostion (as you mentioned)?

Copy link
Collaborator

@pmonks pmonks Oct 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library itself wouldn't use or require dependency injection (that's an anti-pattern in a library), but there are ways to design libraries that make them more DI-friendly, and (as a side effect) more extensible as well. Right now Spdx-Java-Library is not designed that way, and (amongst many other things), removing the various singletons would be part of such a redesign (as would introducing more "top level" interfaces, clearly separating out implementations, etc. etc.).

The challenge is doing these things in such a way that backwards compatibility is preserved.

Copy link
Member

@goneall goneall Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the delay responding to this PR - When we originally designed the ListedLicenses model store, we were thinking the IListedLicenseStore would be the primary interface and the API's between the SpdxListedLicenseModelStore and its subclasses would be more internal.

That being said, I don't see any issue with making those more public - I don't think we would introduce any additional vulnerabilities - but something to consider before the change.

In terms of the ListedLicenses singleton class, this is public and may be used in may applications - so we should be careful about changing the API's. If we could preserve compatibility and improve the flexibility of the design, I would be very open to making some changes.

Let me know if you have any proposed design changes.

Note that with SPDX 3.0 introducing breaking changes in the spec, we're going to introduce a new version of the library with some breaking changes - so there is the possibility to improve the design at that point. We could accept these changes for now, then update the design in the 3.0 version of the libraries.

Copy link
Contributor Author

@loosebazooka loosebazooka Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The weird thing for me I guess is that in the gradle and maven plugins we still need to separately parse the license list in the LicenseManager.

Yeah, I'm okay putting this in now, as it solves our (gradle plugins) (1) network access problem and (2) discrepancy between ListedLicenses and LicenseManager.

Would love to chat about 3.0 at some point in the future too.

Copy link
Member

@goneall goneall Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmonks - you OK with merging this for now and revisiting for the 3.0 version?

Copy link
Collaborator

@pmonks pmonks Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goneall absolutely! I'm not able to offer to commit much time to a DI-friendly redesign right now, so punting on that (while still giving @loosebazooka the facilities they've implemented here in the short term) would be my preferred option.


/**
* @return InputStream for the Table of Contents of the exceptions formated in JSON SPDX
* @throws IOException
*/
abstract InputStream getExceptionTocInputStream() throws IOException;
public abstract InputStream getExceptionTocInputStream() throws IOException;

/**
* @return InputStream for a license formated in SPDX JSON
* @throws IOException
*/
abstract InputStream getLicenseInputStream(String licenseId) throws IOException;
public abstract InputStream getLicenseInputStream(String licenseId) throws IOException;

/**
* @return InputStream for an exception formated in SPDX JSON
* @throws IOException
*/
abstract InputStream getExceptionInputStream(String exceptionId) throws IOException;
public abstract InputStream getExceptionInputStream(String exceptionId) throws IOException;

/**
* Loads all license and exception ID's from the appropriate JSON files
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/org/spdx/storage/listedlicense/SpdxListedLicenseWebStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,22 +44,22 @@ private InputStream getUrlInputStream(final URL url) throws IOException {
}

@Override
InputStream getTocInputStream() throws IOException {
public InputStream getTocInputStream() throws IOException {
return getUrlInputStream(new URL(SpdxConstants.LISTED_LICENSE_URL + LICENSE_TOC_FILENAME));
}

@Override
InputStream getLicenseInputStream(String licenseId) throws IOException {
public InputStream getLicenseInputStream(String licenseId) throws IOException {
return getUrlInputStream(new URL(SpdxConstants.LISTED_LICENSE_URL + licenseId + JSON_SUFFIX));
}

@Override
InputStream getExceptionTocInputStream() throws IOException {
public InputStream getExceptionTocInputStream() throws IOException {
return getUrlInputStream(new URL(SpdxConstants.LISTED_LICENSE_URL + EXCEPTION_TOC_FILENAME));
}

@Override
InputStream getExceptionInputStream(String exceptionId) throws IOException {
public InputStream getExceptionInputStream(String exceptionId) throws IOException {
return getLicenseInputStream(exceptionId); // Same URL using exception ID rather than license ID
}

Expand Down
51 changes: 50 additions & 1 deletion src/test/java/org/spdx/library/model/license/ListedLicensesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
package org.spdx.library.model.license;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.List;
import java.util.Optional;

Expand All @@ -7,6 +10,7 @@
import org.spdx.library.SpdxConstants;

import junit.framework.TestCase;
import org.spdx.storage.listedlicense.SpdxListedLicenseModelStore;

/**
* Copyright (c) 2019 Source Auditor Inc.
Expand Down Expand Up @@ -120,7 +124,9 @@ public void testGetExceptionbyIdLocal() throws InvalidSPDXAnalysisException {
}

public void testGetExceptionIds() throws InvalidSPDXAnalysisException {
assertTrue(ListedLicenses.getListedLicenses().getSpdxListedExceptionIds().size() >= NUM_3_7_EXCEPTION);
List<String> result = ListedLicenses.getListedLicenses().getSpdxListedExceptionIds();
assertTrue(result.size() >= NUM_3_7_EXCEPTION);
assertTrue(result.contains("389-exception"));
}

public void testListedLicenseIdCaseSensitive() {
Expand Down Expand Up @@ -154,4 +160,47 @@ public void testGetExceptionIdProperty() throws InvalidSPDXAnalysisException {
assertTrue(idProp.get() instanceof String);
assertEquals(id, idProp.get());
}

public void testLicenseListInitializeListedLicenses() throws InvalidSPDXAnalysisException {
try {
ListedLicenses.initializeListedLicenses(new SpdxListedLicenseModelStore() {
@Override
public InputStream getTocInputStream() throws IOException {
return ListedLicensesTest.class.getResourceAsStream("licenses.json");
}

@Override
public InputStream getExceptionTocInputStream() throws IOException {
return ListedLicensesTest.class.getResourceAsStream("exceptions.json");
}

@Override
public InputStream getLicenseInputStream(String licenseId) throws IOException {
throw new UnsupportedOperationException("this shouldn't be used in tests");
}

@Override
public InputStream getExceptionInputStream(String exceptionId) throws IOException {
throw new UnsupportedOperationException("this shouldn't be used in tests");
}

@Override
public void close() throws Exception {
// Nothing to do for the either the in-memory or the web store
}
});
List<String> licenseIds = ListedLicenses.getListedLicenses().getSpdxListedLicenseIds();
assertEquals(1, licenseIds.size());
assertFalse(licenseIds.contains("Apache-2.0"));
assertTrue(licenseIds.contains("TEST"));

List<String> exceptionIds = ListedLicenses.getListedLicenses().getSpdxListedExceptionIds();
assertEquals(1, licenseIds.size());
assertFalse(exceptionIds.contains("389-exception"));
assertTrue(exceptionIds.contains("TEST-exception"));
} finally {
// since ListedLicenses in a singleton, reset it after running this test
ListedLicenses.resetListedLicenses();
}
}
}
17 changes: 17 additions & 0 deletions src/test/resources/org/spdx/library/model/license/exceptions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"licenseListVersion": "1.1-test",
"exceptions": [
{
"reference": "./TEST-exception.json",
"isDeprecatedLicenseId": false,
"detailsUrl": "./TEST-exception.html",
"referenceNumber": 1,
"name": "TEST Directory Server Exception",
"licenseExceptionId": "TEST-exception",
"seeAlso": [
"http://example.com/TEST_Exception_License_Text"
]
}
],
"releaseDate": "1999-01-01"
}
18 changes: 18 additions & 0 deletions src/test/resources/org/spdx/library/model/license/licenses.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"licenseListVersion": "1.1-test",
"licenses": [
{
"reference": "https://spdx.org/licenses/test.html",
"isDeprecatedLicenseId": false,
"detailsUrl": "https://spdx.org/licenses/test.json",
"referenceNumber": 1,
"name": "Some fake licenses for tests",
"licenseId": "TEST",
"seeAlso": [
"https://opensource.org/licenses/TEST"
],
"isOsiApproved": true
}
],
"releaseDate": "1999-01-01"
}