Make ListedLicenses more user configurable #211
Conversation
Signed-off-by: Appu Goundan <appu@google.com>
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@pmonks - you OK with merging this for now and revisiting for the 3.0 version?
There was a problem hiding this comment.
@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.
|
Oops lemme fix that test |
Signed-off-by: Appu Goundan <appu@google.com>
|
sorry about that, need a workflow re-run |
This should allow the spdx-gradle-plugin to work better in offline mode by registering our own cached license files.
see: spdx/spdx-gradle-plugin#16