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 profile to be a property on collections and specify conformance #447

Merged
merged 5 commits into from
Oct 10, 2023

Conversation

goneall
Copy link
Member

@goneall goneall commented Jul 29, 2023

This is a draft proposed solution for #365.

It changes the definition of a key property in the Core profile and should be discussed and reviewed before merging.

This PR makes the following changes:

  • Change the profile property to 2 different properties profileConformance and profileNamespace
  • Move the profile properties from CreationInfo to ElementCollection

The above changes were tentatively agreed to on the 5 Sept 2023 tech call and updated on the 12 Sept. call to only add the profileConformance since no one objected to the type. We can add back profileNamespace later as a separate PR.

@goneall goneall marked this pull request as draft July 29, 2023 17:56
@goneall goneall self-assigned this Jul 29, 2023
@goneall goneall added this to the 3.0-rc2 milestone Jul 29, 2023
@zvr
Copy link
Member

zvr commented Jul 29, 2023

I think the profile-as-namespace property was to help non-RDF serialization know what kind of properties they will encounter in a document, making it easier to parse.

Removing it, the JSON or TagValue parsers for example would have to be able to receive any key/tag, not from a predefined set.

@goneall
Copy link
Member Author

goneall commented Jul 30, 2023

I think the profile-as-namespace property was to help non-RDF serialization know what kind of properties they will encounter in a document, making it easier to parse.

@zvr - good point. I would suggest we add 2 properties - one for namespaces and a different property for conformance.

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 and clarifies the semantics.

(But the implications on validation and adding "implicit profiles" are unclear to me.)

@davaya
Copy link
Contributor

davaya commented Aug 7, 2023

Profile as conformance point and as a team/community make sense.

profile as a property doesn't make any sense to me, whether on a collection or any other type of element. A conforming implementation will be able to parse every class defined in every profile to which it claims conformance - the implementation is not going to gain or lose functionality based on the data it sees. Profile as a namespace can identify modular model files, but an implementation understands the modules it understands - seeing a profile property value won't help it understand any other modules.

I don't object to keeping it as a property, but haven't seen any use case describing the benefit of doing so.

@goneall
Copy link
Member Author

goneall commented Aug 7, 2023

profile as a property doesn't make any sense to me, whether on a collection or any other type of element.

What I would like to capture is the intent of the SPDX producer - do they intend for this profile conformance to be supported or not?

One example where I would use this is in the verification utility. If a collection claims to support a profile but does not include a required property, I would fail the validation.

@davaya
Copy link
Contributor

davaya commented Aug 10, 2023

The serialization group separated the concept into "definitionProfile" that expresses which profile defines the element class, and "conformanceProfile" that applies to Collections (plus Agents according to @jeff-schutt).

@sbarnum mentioned profiles and multiple namespaces. That might be illustrated with an example: Meetings lists teams and focus groups - people who are interested in a topic. That would be the "Profile Team" meaning (from #365).

Imagine that the Security Team, instead of defining the Security Profile, defined the "VEX Profile" and the "Verification Profile" to address how to do payload hashes and signatures. If that were true, the Classes created by the Security team would be defined under the VEX profile or the Verification profile, but not both, and not under the Security team name. That's the only meaning of "Profile Namespace" I can think of that isn't either the definition or the team, and that doesn't seem useful.

Radical Proposal:

  • Remove the "profile" property with ProfileIdentifierType values from CreationInfo, because every class is defined in exactly one profile, and knowing the class means that you know its one and only definitionProfile plus the definitionProfiles of all of its parent classes up to core.
  • Define new "conformanceProfile" or other properties that address conformance or other use cases in ElementCollection, Agent, or other classes to which they apply.

@goneall
Copy link
Member Author

goneall commented Aug 10, 2023

The more I think about this the more I like the proposal.

I have a couple reservations:

  • The conformanceProfile IMO is an expression of intent by the creator and not an intrinsic part of the collection, but I can also make an argument it is an attribute of the collection
  • It could be applicable to an individual element that is not contained in a collection - but this would be uncommon

On the benefit side:

  • Compaction is improved
  • Clears up some of the confusing parts of the model - overall simpler IMO

Fixes #365

Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall goneall changed the title Proposal to define profiles as conformance points Update profile definition to separate conformance and namespace usage into separate properties Sep 5, 2023
@goneall goneall marked this pull request as ready for review September 5, 2023 18:39
@goneall
Copy link
Member Author

goneall commented Sep 5, 2023

@jeff-schutt - ready for review

- type: ProfileIdentifierType
- minCount: 1
- profileNamespace
- type: ProfileIdentifierType
Copy link
Member

Choose a reason for hiding this comment

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

I think, that, at least for the namespace case, we should be using anyURI type.

Why use http://spdx.org/rdf/v3/Core/ProfileIdentifierType/software as the value when we already have http://spdx.org/rdf/v3/Software ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to use enumerations to restrict the range of anyURI that is used. We already have the profile enumerations defined.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I understand the advantages of a restricted set of values.

On the other hand:

  • there has to be a correspondence between the value http://spdx.org/rdf/v3/Core/ProfileIdentifierType/software and the prefix http://spdx.org/rdf/v3/Software, for a reader, for example, to check that only classes/properties of declared namespaces are being used... I tried to avoid using the words "namespace" and "map" for this 😝
  • allowing arbitrary URIs automatically takes care of all the Extension discussions we had back in the day, allowing people to add their own classes and properties.

Copy link
Contributor

@davaya davaya Sep 8, 2023

Choose a reason for hiding this comment

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

This puts namespaces into practice. An application's conformance claim would be known by the namespaces it references, both within http://spdx.org/rdf/v3/ and from non-SPDX definitions.

And to touch the third rail of namespace maps 🔥, applications can define any prefix strings they want without affecting any other applications: c:Element, s:Package, l:Apache-2.0, sec:VEXstuff, etc. That's the beauty of maps, they are locally scoped, whether for software applications or for documents. Adopting prefix string conventions is outside the scope of the model; the model just defines the prefix (namespace) IRIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we want to be literal about the use of the actual namespaces use within the collection and the namespace property, I would go back to my original position that we should not have this property at all.

It isn't that much effort to traverse the elements and properties and collect what namespaces are actually used.

Even if we have the property, in practice it is quite possible we have a collection with a namespaceProfile declared at the collection level which isn't used as well as a no namespaceProfile declared for profiles that are used. Not to mention, it is an optional property.

As a tool developer, I would never use the namespaceProfile myself due to the possibility of introducing errors and inconsistencies.

Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall
Copy link
Member Author

goneall commented Sep 8, 2023

Note: I updated the glossary definition of profile to match the proposed changes.

@goneall goneall changed the title Update profile definition to separate conformance and namespace usage into separate properties Update profile to be a property on collections and specify conformance Sep 12, 2023
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall
Copy link
Member Author

goneall commented Sep 12, 2023

@davaya @jeff-schutt - Just finished updating the PR from the tech call, ready for review

@zvr - we decided in the tech call to split out the namespaceProfile from this PR since the conformanceProfile didn't have any issues and the two different properties were confusing.

I created a branch in this repo profilenamespace which has the profileNamespace property if we want to add it back in at a later date.

@zvr zvr merged commit bbcf3a2 into main Oct 10, 2023
1 check passed
@zvr zvr deleted the profiledef branch October 10, 2023 16:07
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.

4 participants