Skip to content
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.

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

Update to.md to reflect what min:0 means #623

Merged
merged 2 commits into from
Mar 28, 2024
Merged

Update to.md to reflect what min:0 means #623

merged 2 commits into from
Mar 28, 2024

Conversation

kestewart
Copy link
Contributor

Min 0 allows producers to indicate that there are no known relationships of the given type.

Signed-off-by: Kate Stewart kstewart@linuxfoundation.org

Min 0 allows producers to indicate that there are no known relationships of the given type.

Signed-off-by:  Kate Stewart <kstewart@linuxfoundation.org>
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

Thanks @kestewart - Very minor wording suggestion.

model/Core/Properties/to.md Outdated Show resolved Hide resolved
@goneall
Copy link
Member

goneall commented Jan 30, 2024

Fixes #527

@goneall goneall mentioned this pull request Jan 30, 2024
32 tasks
Co-authored-by: Gary O'Neall <gary@sourceauditor.com>
@kestewart
Copy link
Contributor Author

kestewart commented Jan 30, 2024

Your wording suggestion looks good to me. Applied. @goneall - can you re-review?

Copy link
Member

@maxhbr maxhbr left a comment

Choose a reason for hiding this comment

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

LGTM

@jeff-schutt
Copy link
Collaborator

Discussed on the tech call. There was confusion as to the reason for this change. Team agreed it needs further discussion with @kestewart and @goneall

@sbarnum
Copy link
Collaborator

sbarnum commented Jan 30, 2024

This incomplete relationship approach seems very problematic as it makes it difficult for people to understand the intended semantics, does not really make sense to have a relationship with only one end, and will break any sort of graph semantic or structual analysis out there.

It seems like a much cleaner approach to this would be to define two new Individuals of type Element (yeah, I know Element is supposed to be abstract but this seems like the first clear reason to maybe violate that):

  1. NONE - asserts that there do not exist any elements (to be more precise "a set of Elements with cardinality of zero")
  2. UNKNOWN- asserts that is unknown how many elements there are (including zero) (to be more precise "a set of Elements of unknown cardinality")

In this way the appropriate Individual could simply be used as the value of the to property on the relationship.
If you wanted to say A contains B you would say Relationship (from=A; relationshipType="contains"; to=B)
If you wanted to say that A contains nothing you would say Relationship (from=A; relationshipType="contains"; to=NONE)
If you wanted to say that it is unknown what A contains you would say Relationship (from=A; relationshipType="contains"; to=UNKOWN)

This is much more semantically clear in approach and in value, it does not violate graph semantics, and it would be valid under various approaches and technologies for graph semantic or structual analysis out there.

@kestewart
Copy link
Contributor Author

@JPEWdev - can you please review Sean's comments, and weigh in here.
@sbarnum - we had similar semantics in 2.3, that you're outlining above. Suggest we just revert back to them.

Specifically in the 2.3 spec there is:

In cases where there are "known unknowns", the use of the keyword NOASSERTION can be used on the right hand side of a relationship to indicate that the author is not asserting whether there are other SPDX elements (package/file/snippet) that are connected by relationships or not. That is, there could be some, but the author is not asserting one way or another.

Similarly, the use of the keyword NONE can be used to indicate that an SPDX element (package/file/snippet) has no other elements connected by some relationship to it.

The use of NOASSERTION or NONE is not mandatory for any relationship. If no relationship of a particular type is specified, then the document author is not presumed to be asserting whether or not there are relationships of that type. If some relationships of a particular type are specified, then the document author is not presumed to be asserting whether there are more possible relationships of that type.

@goneall - do you want to weigh in on this?

@goneall
Copy link
Member

goneall commented Jan 31, 2024

I (really) like Sean's proposal above with one caveat. I just merged PR #588 which created a NoneLicense and a NoAssertionLicense which have a type restriction of AnyLicenseInfo which is a subclass of Element. Since NONE and NOASSERTION will have a type restriction on Element with very similar semantics, we need to make sure these don't get confused. Should we name these NoneElement and NoAssertionElement? Should we keep them named as NONE and NOASSERTION and use the same individuals in the licensing case (this is what we do in the SPDX 2.3 RDF model)?

@kestewart
Copy link
Contributor Author

I'd be in favor of keeping them as NONE & NOASSERTION at this point. NoneElement as a term may be confusing.

@goneall
Copy link
Member

goneall commented Jan 31, 2024

I'd be in favor of keeping them as NONE & NOASSERTION at this point. NoneElement as a term may be confusing.

@kestewart - do you think we should keep the current NoneLicense and NoAssertionLicense if we go with NONE and NOASSERTION? See PR #588 for context.

@kestewart
Copy link
Contributor Author

@goneall - I'm fine with keeping NoneLicense & NoAssertionLicense, if that's what the legal team requested to use on License expressions. I can see it simplifying some things, but if it wasn't their request, let's just revert back to NONE & NOASSERTION as we have it in 2.3.

@sbarnum
Copy link
Collaborator

sbarnum commented Feb 1, 2024

Given that NoneLicense and NoAssertionLicense involve a type restriction to AnyLicenseInfo I think it makes sense to keep them as is with the "License" specific extensions in their names.

@goneall
Copy link
Member

goneall commented Feb 1, 2024

Given that NoneLicense and NoAssertionLicense involve a type restriction to AnyLicenseInfo I think it makes sense to keep them as is with the "License" specific extensions in their names.

Agree - @sbarnum - do you want to create a separate PR with the proposed solution?

@kestewart
Copy link
Contributor Author

Note: new PR will need to be generated, with min to be updated to 1; and the NONE & NOASSERTION elements created. @sbarnum - can you take a pass at this PR, so Gary and I can review while traveling this week?

@kestewart
Copy link
Contributor Author

Also, it should be noted that we have https://github.com/spdx/spdx-3-model/blob/main/model/Core/Vocabularies/RelationshipCompleteness.md in the model already, and this should be factored into any changes so this doesn't accidentally overlap and cause confusion.

@goneall
Copy link
Member

goneall commented Feb 5, 2024

Per discussion with @sbarnum - @sbarnum will create a pull request with the proposed solution. Sean will take into consideration the RelationshipCompleteness when documenting the proposal.

@sbarnum
Copy link
Collaborator

sbarnum commented Feb 5, 2024

@goneall After looking at RelationshipCompleteness while in the process of creating the PR I realized that noAssertion in RelationshipCompleteness has no overlap or conflict with NOASSERTION as a subclass of Element and used in the to property of Relationship.
noAssertion in RelationshipCompleteness is stating that no assertion is being made about the completeness of the relationship not about the nature of the relationship itself.
NOASSERTION when used in the to property of Relationships it is the ONLY value present and asserts that no such relationships exist.
noAssertion when used in the completeness property of Relationship does not affect how many values may be in the to property (there could be one or more) but rather asserts that no assertion is being made regarding whether any values present in the to property represent the complete set of "related" elements.
So, no change is necessary there.

@kestewart
Copy link
Contributor Author

@maxhbr, @goneall - can you review #629 - If you're good with it, we'll close this one.

@sbarnum
Copy link
Collaborator

sbarnum commented Feb 6, 2024

Consensus of the Feb 06, 2024 Tech call was to pursue approving and merging #629 to resolve this issue.

It was discussed very briefly that there is a potential for confusion between the use of NOASSERTION in the to property and the use of noAssertion in the completeness property.
The added documentation in #629 seems to be clear but the documentation on noAssertion within the RelationshipCompleteness md file may need a little fleshing out to more clearly describe its intended meaning and use.

@kestewart
Copy link
Contributor Author

Need to decide between this one and #629

@goneall goneall modified the milestones: 3.0-rc2, 3.0 Feb 16, 2024
@goneall goneall modified the milestones: 3.0, 3.0-rc3 Mar 5, 2024
@kestewart kestewart merged commit ea15fe6 into main Mar 28, 2024
1 check passed
@kestewart kestewart deleted the kestewart-patch-4 branch March 28, 2024 15:50
@JPEWdev
Copy link
Contributor

JPEWdev commented Mar 28, 2024

Per discussion on #629 , this will be merged and then #629 will be discussed as a future option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants