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 diagrams for v3.0.1 #852

Merged
merged 30 commits into from
Aug 27, 2024
Merged

Update diagrams for v3.0.1 #852

merged 30 commits into from
Aug 27, 2024

Conversation

bact
Copy link
Collaborator

@bact bact commented Aug 13, 2024

Changes

  • Updated modified property names based on CHANGELOG
    • Replace contentType -> /Core/contentType
    • Add alder32
    • Rename parameters -> parameter
    • Rename imports -> import
    • Rename hasInputs -> hasInput
    • Rename hasOutputs -> hasOutput
  • Fix casing in types
  • Fix casing in Classes in from->to relationship description
  • Typo: locationHint: anyUR -> locationHint: anyURI
  • Fix type: DateTime, from "String" to xsd:dateTimeStamp
  • Add prefix xsd: to XML Schema Datatypes (xsd:string, xsd:decimal, xsd:boolean, xsd:positiveInteger, xsd:nonNegativeInteger)
  • Sort enums
  • Fix typo in VexAffectedVulnAssessmentRelationship/actionStatement type: Strting -> string
  • Fix relevant typos in model descriptions (found during the diagram checks)
  • Remove outdated model-security-profile.*

Post-merge notes

bact added 2 commits August 13, 2024 04:19
Signed-off-by: Arthit Suriyawongkul <[email protected]>
- Updated modified property names based on change log
- Fix casing in types
- Fix casing in Classes in from-to relationship description
- Sort enums

Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact bact added this to the 3.0.1 milestone Aug 13, 2024
@kestewart kestewart requested a review from goneall August 13, 2024 11:23
@kestewart
Copy link
Contributor

@bobmartin3000, @sbarnum - can you please review the diagrams before we merge.

Copy link
Contributor

@kestewart kestewart left a comment

Choose a reason for hiding this comment

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

Cross checked the changes, and LGTM.

@kestewart kestewart requested a review from zvr August 13, 2024 11:45
@sbarnum
Copy link
Collaborator

sbarnum commented Aug 13, 2024

There are two issues that need fixed before merging this PR.

  1. In the 'Pedigree' section within the RelationType enumeration in Core, the "copiedTo" and "expandsTo" values need swapped in order such that "copiedTo" is listed before "expandsTo"
  2. The removal of Software/contentType from the model listed in the CHANGELOG is not reflected in the diagrams
    • On the Software and the Core+Software tabs of the diagram the following changes need to be made for the "contentType" property on the File class
      • Change "+ contentType: MediaType[0..1]" to "+ /Core/contentType: MediaType[0..1]"
        • an example of this can be seen in the "/SimpleLicensing/licenseText" property on the License class in the ExpandedLicensing namespace depicted in the Licensing tab of the diagrams
      • background color and property ordering changes (such as are seen in the AI and Dataset tabs of the diagrams are unnecessary in this case as those formatting approaches apply only to cardinality restrictions of superclasses
    • New .png and .svg image files will need to be created and committed for the Software and the Core+Software tabs of the diagram

@bact
Copy link
Collaborator Author

bact commented Aug 14, 2024

@sbarnum I see that in the diagram we labelled

  • xsd:anyURI with anyURI; and
  • xsd:string with String

Should I change the casing of String to string?
I can quickly do this with some regex and will recheck with eyes.
(or if you like to have xsd: prefix added, that's doable as well, with no additional step in the process)

anyURI string

@bact
Copy link
Collaborator Author

bact commented Aug 14, 2024

Should DateTime a xsd:dateTimeStamp ?,
according to https://github.com/spdx/spdx-3-model/blob/main/model/Core/Datatypes/DateTime.md

Screenshot 2024-08-14 at 15 26 17

Its one-line description can be:

DateTime: xsd:dateTimeStamp
dateTimeStamp constrained to a ISO-8601 format, with resolution of seconds and UTC time zone.

bact added 4 commits August 14, 2024 16:10
Also fix casing and add prefix for XML Schema Datatypes

Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
@zvr
Copy link
Member

zvr commented Aug 14, 2024

Should DateTime a xsd:dateTimeStamp ?

I thought it had to be an xsd:string in order to have a pattern restricting it, but I was assured in discussion(s) that this will also work on any type, so we changed the model to have xsd:dateTimeStamp as superclass.

So yes, the diagram has to be fixed.

@bact
Copy link
Collaborator Author

bact commented Aug 14, 2024

Should DateTime a xsd:dateTimeStamp ?

I thought it had to be an xsd:string in order to have a pattern restricting it, but I was assured in discussion(s) that this will also work on any type, so we changed the model to have xsd:dateTimeStamp as superclass.

So yes, the diagram has to be fixed.

Thank you @zvr I will fix that then.

@bact bact marked this pull request as draft August 14, 2024 21:43
@bact bact marked this pull request as ready for review August 15, 2024 04:10
@goneall
Copy link
Member

goneall commented Aug 15, 2024

Looks like Relationship has two "subclass of" arrows point to Element. Can we delete one of them?

Screenshot 2024-08-15 at 04 45 12

Yes - looks like it can be removed

@sbarnum
Copy link
Collaborator

sbarnum commented Aug 15, 2024

I apologize for delayed response.
I have not been getting notifications of the continuing comments and changes here.

  1. I do not object to the use of the specific XSD datatypes (e.g., xsd:string) here but do worry that this is a fairly broad and substantive change at this late date that brings with it non-insignificant risk. If we decide to do this we need to ensure that the textual spec explicitly specifies these types, that the generated TTL ontology files both import the XSD namespace and explicitly specify these types, that the JSON-LD context explicitly specifies these to their full expanded URI identifiers, and that ALL examples explicitly assert these types directly. If we cannot guarantee that all of this is done immediately then this change should not be made to the diagrams.
  2. I agree with Alexios that if the spec now currently explicitly specifies xsd:dateTimeStamp then we should use it in the diagrams as well for the base type of the DateTime simple type.
  3. Personally, I would recommend not decoupling both the 'element' and the 'rootElement' arrows (with their links to Element and their 'NOT' links to SpdxDocument) from 'ElementCollection'. I agree it is more legible when decoupled but would assert that it may make it actually more difficult for some readers to understand as "property" arrows from classes in these sorts of UML class diagrams (even though this is NOT technically a formal UML class diagram) are not typically duplicated to multiple different classes. I do not object strongly if everyone else feels strongly it should be decoupled and won't confuse readers but I would vote to keep them as they were.
  4. I agree that the double subclassing arrows from Relationship to Element is an error and one should be removed. Not sure how that creeped in there. It looks like the current branch for this PR still has both arrows though.
  5. I reviewed through this list of changes in the edited first comment for this PR and it looks like they are all implemented as described (including the two issues I called out in my previous comment).

@sbarnum
Copy link
Collaborator

sbarnum commented Aug 15, 2024

Given my notification issues, if you need anything further from me to help get us across the 3.0.1 finish line please ping my cell (in my email sig).

@bact
Copy link
Collaborator Author

bact commented Aug 16, 2024

Thank you Sean. For very detailed comments. I will start to look at all of them now

@bact
Copy link
Collaborator Author

bact commented Aug 16, 2024

1. I do not object to the use of the specific XSD datatypes (e.g., xsd:string) here but do worry that this is a fairly broad and substantive change at this late date that brings with it non-insignificant risk. If we decide to do this we need to ensure that the textual spec explicitly specifies these types, that the generated TTL ontology files both import the XSD namespace and explicitly specify these types, that the JSON-LD context explicitly specifies these to their full expanded URI identifiers, and that ALL examples explicitly assert these types directly.

Agree on the point on the big change set in this late date.

I have checked quickly and found that at Line 8 of the TTL ontology: https://spdx.github.io/spdx-spec/v3.0/model/spdx-model.ttl we already have XSD import:

@prefix xsd: <http://www.w3.org/2001/XMLSchema#> .

And Lines 19-22, we have one of the XSD types specified as:

        [ sh:datatype xsd:string ;
            sh:maxCount 1 ;
            sh:nodeKind sh:Literal ;
            sh:path <https://spdx.org/rdf/3.0.0/terms/AI/informationAboutTraining> ],

I think I can produce two sets of diagrams, ones with and without the xsd: prefix. And we can pick one from our decision here.

2. I agree with Alexios that if the spec now currently explicitly specifies xsd:dateTimeStamp then we should use it in the diagrams as well for the base type of the DateTime simple type.

Changed.

3. Personally, I would recommend not decoupling both the 'element' and the 'rootElement' arrows (with their links to Element and their 'NOT' links to SpdxDocument) from 'ElementCollection'. [...]

The two arrow groups got merged again.

4. I agree that the double subclassing arrows from Relationship to Element is an error and one should be removed. Not sure how that creeped in there. It looks like the current branch for this PR still has both arrows though.

Removed.


PNGs and SVGs to be upload.

@bact
Copy link
Collaborator Author

bact commented Aug 16, 2024

@sbarnum new PNGs and SVGs are uploaded.

@bact bact requested a review from sbarnum August 16, 2024 09:45
bact added a commit to bact/spdx-spec that referenced this pull request Aug 16, 2024
From updates in spdx/spdx-3-model#852

Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact bact added the publishing Dependency for publishing final version of spec label Aug 16, 2024
kestewart pushed a commit to spdx/spdx-spec that referenced this pull request Aug 16, 2024
From updates in spdx/spdx-3-model#852

Signed-off-by: Arthit Suriyawongkul <[email protected]>
@kestewart
Copy link
Contributor

ping @sbarnum - can you take another pass and confirm we're good to merge.

Copy link
Collaborator

@sbarnum sbarnum left a comment

Choose a reason for hiding this comment

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

The new model.drawio and related PNG & SVG files look good to me.

There remains one significant issue.

The 'images' folder in the branch to merge in this PR contains 2 files (model-security-profile.drawio and model-security-profile.png) that should NOT be present in the model. These appear to be an attempted duplication of the content in the model-security tab of the model.drawio diagram and its associated PNG and SVG images. They are both SIGNIFICANTLY out of date, incomplete, and incorrect. They should be removed.
Once that is done, this looks ready to merge.

Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact
Copy link
Collaborator Author

bact commented Aug 22, 2024

The 'images' folder in the branch to merge in this PR contains 2 files (model-security-profile.drawio and model-security-profile.png) that should NOT be present in the model. These appear to be an attempted duplication of the content in the model-security tab of the model.drawio diagram and its associated PNG and SVG images. They are both SIGNIFICANTLY out of date, incomplete, and incorrect. They should be removed. Once that is done, this looks ready to merge.

@sbarnum Thank you. I have removed both files.

@bact bact requested a review from sbarnum August 22, 2024 16:10
@bact
Copy link
Collaborator Author

bact commented Aug 22, 2024

Note: The PR to update the diagrams in spdx-spec repo to the most updated ones (from 22 Aug 2024, from this PR) is at spdx/spdx-spec#1077

(Files in spdx-spec are ones that will be displayed on our website at https://spdx.github.io/spdx-spec/v3.0.1-draft/annexes/rdf-model/ )

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.

Based on tech call on 27 Aug 2024 - approved

@goneall goneall merged commit a37e2e4 into spdx:main Aug 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
publishing Dependency for publishing final version of spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check if diagrams in "RDF model" chapter up-to-dated for v3.0.1
5 participants