Cannot use ExternalExtractedLicenseInfo about spdx-java-library HOT 10 CLOSED

@nicoweidner The issue is that the ExternalExtractedLicenseInfo is intended to be defined in an external document - not within the document containing the reference. Think of ExternalExtractedLicenseInfo and ExternalSpdxElement as pointers to a definition of a license or element outside of the SPDX document itself. The list of ExtractedLicenseInfos in the document is definitions of ExtractedLicenseInfos defined within the document.

You don't need to add an ExternalExtractedLicenseInfo to the list ExtractedLicenseInfos to use it within a document. In fact, trying to do so should cause an error - which doesn't today. We should add a check in the library for this. There does seem to be a check in the serializer for this - which is OK.

from spdx-java-library.

Hm... I see, thanks for the explanation. So an intended usage of external license info would be as follows, I suppose? Here, I just create the ExternalExtractedLicenseInfo and then reference it in a file.

        var externalDocumentRef = doc.createExternalDocumentRef("DocumentRef-1",
                "externalDocUri", sha1Checksum);        
        var externalLicenseInfo = new ExternalExtractedLicenseInfo(doc.getModelStore(),
                doc.getDocumentUri(), "DocumentRef-1:LicenseRef-XXX",
                doc.getCopyManager(), true);
        var file = doc.createSpdxFile("SPDXRef-file", "./foo.txt", null,
                        List.of(), null, sha1Checksum)
                .build();
        file.setLicenseConcluded(externalLicenseInfo);

In this case, licenseConcluded on the file shows up as externalDocUri#LicenseRef-XXX, which looks ok to me.

One issue I have with the current structure is that ExternalExtractedLicenseInfo is a subclass of ExtractedLicenseInfo. Not being able to use it in ExtractedLicenseInfos seems like a violation of the Liskov substitution principle to me (or, in simpler terms, a confusing data structure).

from spdx-java-library.

So an intended usage of external license info would be as follows, I suppose?

Yes - that is correct

One issue I have with the current structure is that ExternalExtractedLicenseInfo is a subclass of ExtractedLicenseInfo.

I agree it is confusing. Both ExternalSpdxElement and ExternalExtractedLicenseInfo are intended to be used referenced wherever an SpdxElement or an ExtractedLicenseInfo is allowed. So, from that perspective they can be "substituted". Where it gets confusing is that you cannot fetch some of the properties and you can not write any of the properties associated with those data structures, so it is not fully substitutable. It is similar to having an immutable version of a standard class where you can't write to the properties.

I'm open to suggestions on alternative structures that are more straightforward. The requirements would be that any property expecting an Element or ExtractractedLicenseInfo type would accept the ExternalSpdxElement and ExternalExtractedLicenseInfo typed objects resp. Since these objects are not contained within the document, we would not be able to access the individual property values with the exception of the ID.

from spdx-java-library.

My first idea would be to have a common parent class (probably abstract), and have the "local" and "external" variants as separate subclasses. Then one could distinguish in usages whether it's okay to have either a local or external element, or whether only one of two is accepted (e.g. for extractedLicenseInfos on the doc, only local elements are accepted). It would also avoid having the external version inherit unwanted behavior from the local version.
So, something like an abstract class SpdxElement with subclasses LocalSpdxElement and ExternalSpdxElement.

There is a lot of code in these classes already, though, so I am not sure which problems this could cause

from spdx-java-library.

@nicoweidner Thanks for the suggestion. I like the idea of having an abstract parent class. Why didn't I think of that earlier!

I don't think this would break anything if we make the parent class abstract.

I'll work on a PR and see if it tests out OK.

from spdx-java-library.

Moving this over to the library repo.

from spdx-java-library.

@nicoweidner I made a PR #137 which refactors the ExtractedLicenseInfo and ExternaExtractedLicenseInfo to inherit from an AbstractExtractedLicenseInfo class.

I also added some more documentation to the Javadocs which hopefully helps make this less confusing.

I don't think this will cause any incompatibilities since it would be very uncommon to have code using an ExtractedLicenseInfo class and expect an ExternalExtractedLicenseInfo. Most implementations which use a class expecting either internal or external license infos would use AnyLicenseInfo or SimpleLicensingInfo superclasses. That being said, it is possible this may break some code.

I didn't make any similar changes to the SpdxElement and it's subclasses. Referencing SpdxElement directly would be very common, so introducing a new abstract class would likely be a breaking change. I'm also thinking it is less of an issue since SpdxElement is an abstract class.

We could refactor these out when we move to a 3.0 release where breaking changes are allowed.

Let me know what you think.

from spdx-java-library.

@nicoweidner - Let me know if you think PR #137 helps solve this issue. I'm planning on a new release of the library and trying to decide if this should be included.

from spdx-java-library.

Sorry, too many things going on, this dropped off my radar 😅 . I'll go have a look now.
IIRC, I didn't even have a case using SpdxElement, in that case it was a purely theoretical concern. So I think it's fine to fix only the ExtractedLicenseInfo part for now

from spdx-java-library.

Thanks @nicoweidner for the reviews and comments!

from spdx-java-library.