Resolve two issues related to ExternalDocumentRef

[KAWAHARA-souta]

ExternalDocumentRef.DocumentRefID does not perform required prefix processing because it is a string, although comments states like below.

spdx/v2/v2_3/document.go (L.19~23):

type ExternalDocumentRef struct {
	// DocumentRefID is the ID string defined in the start of the
	// reference. It should _not_ contain the "DocumentRef-" part
	// of the mandatory ID string.
	DocumentRefID string `json:"externalDocumentId"`

I've fixed it and also fixed related issue. (#267, #268)

[kzantow]

I do not think this change is correct. Adding an element ID reference to the document ID specifier is not what we should do.

It's a little unclear to me whether the JSON external document reference should include DocumentRef- or not -- please note that the JSON format has some oddities that don't always match the TagValue and other formats, but I think it probably should based on the examples in the spec.

I think the correct change to make here is not to use the common.DocElementID, but instead create a new typed string of type common.DocumentID string, which has the behavior of prepending and removing the DocumentRef-, this field which is used in the External Reference objects, as well as the common.DocElementID for consistency. This would end up being less of a breaking change, too, since initializers using strings would continue to work.

[kzantow]

common.DocElementID is the combination of 3 things: external document reference, element reference, and special ID. The DocumentRefID is supposed to be only an ID to reference the external document within this document, this should not include ElementID or special references, this is why it is a string. It is a little unclear if this should include DocumentRef-. If you look at the JSON schema, it makes no reference to this except in the license section, which indicates DocumentRef- is only used in an element ID reference when you are referring to an external document using the docrefIdString, which I think is supposed to match the value here: DocumentRef-[docrefIdString]:.

[KAWAHARA-souta]

Thanks @kzantow ,

I agree that creating a new typed string of type common.DocumentID string.
I will modify the PR accordingly. After that, Could you review it again once I complete the changes?

I also have a question regarding the creation of type common.DocumentID string.

Currently, DocElementID is structured as follows:

type DocElementID struct {
	DocumentRefID string
	ElementRefID  ElementID
	SpecialID     string
}

Would it be OK to change the type of DocElementID.DocumentRefID to the newly created common.DocumentID ?
Additionally, like my commit ea4f07f , would it be OK to make it possible to output a DocElementID as a simple Document ID even when it only has DocumentRefID ?

My main concern is that the type of items specified in Relationship is common.DocElementID:

type Relationship struct {
	// 11.1: Relationship
	// Cardinality: optional, one or more; one per Relationship
	//              one mandatory for SPDX Document with multiple packages
	// RefA and RefB are first and second item
	// Relationship is type from 11.1.1
	RefA         common.DocElementID `json:"spdxElementId"`
	RefB         common.DocElementID `json:"relatedSpdxElement"`

My main intention is to enable the creation of Relationships to Documents (including ExternalDocumentRefs)

[KAWAHARA-souta]

Hi, @kzantow
I've modified and force updated my PR. Could you review it?
I've tried to focus on the issues we need to fix now and make the PR simple.

Addressed issues from this review

[kzantow]

This looks a lot closer, I noticed there is a test that was returning an error which no longer is, I think that case is actually still supposed to be an error, so that will need to be updated, unless I've misunderstood. Also, I think a lot of these changes could be undone -- initializers don't need the casts, do they? And I think the tagvalue changes could be simplified even more to just cast the value from the struct to/from strings rather than updating everything to the new type, since these are just serialization codes, right?

[kzantow]

I think this should still be an error -- there is no ElementID specified

[KAWAHARA-souta]

Hmm, as I mentioned in my previous comment, this was the one of what I wanted to implement..

Additionally, like my commit ea4f07f , would it be OK to make it possible to output a DocElementID as a simple Document ID even when it only has DocumentRefID ?
My main concern is that the type of items specified in Relationship is common.DocElementID:

type Relationship struct {
	// 11.1: Relationship
	// Cardinality: optional, one or more; one per Relationship
	//              one mandatory for SPDX Document with multiple packages
	// RefA and RefB are first and second item
	// Relationship is type from 11.1.1
	RefA         common.DocElementID `json:"spdxElementId"`
	RefB         common.DocElementID `json:"relatedSpdxElement"`

My main intention is to enable the creation of Relationships to Documents (including ExternalDocumentRefs)

For now, revert it back as you suggested.

[kzantow]

Sorry, I'm trying to understand what you are trying to do -- are you trying to refer to the document itself? It has an element ID of SPDXRef-DOCUMENT, I think there should always be an element referenced here.

[kzantow]

Maybe it would be worthwhile defaulting the ElementID to SPDXRef-DOCUMENT if the documentID is set but not the element ID? But I think it's very likely to be a programming error if the element ID is not specified...

[KAWAHARA-souta]

I see, so references to the default document can be handled with SPDXRef-DOCUMENT...
I apologize. It seems I was severely mistaken.
When handling SPDX documents, you inevitably deal with relationships to the document itself, and I thought it would be useful to treat that as a DocumentID.
However, it's fine to just treat it as an ElementID like SPDXRef-DOCUMENT, isn't it...
I'm sorry, please disregard this.

[kzantow]

To be clear: I don't really know what the right thing to do here is, but since we must refer to the document with SPDXRef-DOCUMENT, for example to specify the root relationships, I think it would implicitly have that ID in all cases, and if you were to refer to it with a document ref, it would be <document-id>:SPDXRef-DOCUMENT. I went ahead and merged this fix, as I think it corrects the main issue, we can address this separately, if this approach does not work for you, for some reason

[kzantow]

Looks great, just one small thing: we should be lenient in reading documents, I think

[kzantow]

Thanks for the fix @KAWAHARA-souta!