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

Add logical types generator code #2

Merged
merged 11 commits into from
Jul 24, 2023
Merged

Add logical types generator code #2

merged 11 commits into from
Jul 24, 2023

Conversation

davaya
Copy link
Collaborator

@davaya davaya commented Jul 6, 2023

As this is a playground the code is not official. But creating logical types is the first step in generating logical values needed to evaluate the serialization methods.

Note: as the model is updated, it is critical that the serialization examples remain in sync. For that reason, logical types are tagged with the model version they were generated from.

Thanks again @armintaenzertng for the idea of using class variables as the implementation of logical values.

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.

LGTM (a non-Python developer)

@davaya davaya requested review from zvr and maxhbr July 7, 2023 02:10
```
class Relationship:
from: SpdxId = None # *
to: SpdxId = None # * optional Set[1..*]
Copy link
Member

Choose a reason for hiding this comment

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

the type of to should be some collection of SpdxId, e.g. to: list[SpdxId] = [], right? The comment also hints to that.

The value is also not optional, as there needs to be at least one member in the to side.

Copy link
Collaborator Author

@davaya davaya Jul 9, 2023

Choose a reason for hiding this comment

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

Thanks @maxhbr.

  1. "to" cannot be optional, but that's what the model file says (click on the Relationship header to go there). When the model is fixed, the Relationship class will show it (in a new generated folder tagged with the new model commit date).
  2. the type files have no values (=None). Examples based on the types would have value assignments filled in:
  to = ["http://spdx.org/spdxdocs/spdx-example-444504E0-4F89-41D3-9A0C-0305E82C3301#JaneDoe",
       "http://spdx.org/spdxdocs/spdx-example-444504E0-4F89-41D3-9A0C-0305E82C3301#SuzanneReviewer"]

which would be validated according to the comment - Set means that order doesn't matter, and there can't be duplicates - two Jane Does would be an error. List means that order does matter and there can be duplicates. The 2.3 SBOM file surprisingly contains a Bag example!:

      "hasFiles": [
        "SPDXRef-Specification",
        "SPDXRef-Specification",
        "SPDXRef-CommonsLangSrc",
        "SPDXRef-Specification",
        "SPDXRef-CommonsLangSrc",
        "SPDXRef-JenaLib",
        "SPDXRef-Specification",
        "SPDXRef-CommonsLangSrc",
        "SPDXRef-JenaLib",
        "SPDXRef-DoapSource",
        "SPDXRef-Specification",
        "SPDXRef-CommonsLangSrc",
        "SPDXRef-JenaLib",
        "SPDXRef-DoapSource"
      ],

there are many duplicates, and presumably order doesn't matter. If something like this is to be valid in 3.0, it can't be defined and validated as a Set.

Copy link
Collaborator Author

@davaya davaya Jul 9, 2023

Choose a reason for hiding this comment

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

Comments with "*" indicates content that doesn't (yet) exist in the model files. PR #407 requests that the SpdxId class be added to the model. The playground generator script allows new classes to be patched into the model for demonstration and testing but it flags all patched classes with the "*".

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let me rephrase, why does your comment say optional Set[1..*] instead of Set[0..*]?

List means that order does matter and there can be duplicates. The 2.3 SBOM file surprisingly contains a Bag example!:

in 2.3 the to side as not a list, but instead there are multiple relationships. The thing you are quoting (hasFiles) is just a shorthand to write them. That is not a single relationship. Also in the future there might be multiple overlapping hasFile relationships and your set structure only partially restricts that.

Have we decided that duplicates on the to side are not allowed? Can you point me to the notes or model files where this decision was made? There might be some relationships where duplicates are possible.

Copy link
Collaborator Author

@davaya davaya Jul 9, 2023

Choose a reason for hiding this comment

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

why does your comment say optional Set[1..] instead of Set[0..]

Because property values can be optional and also have minCount = 4. Say you want to define a quadrilateral shape (square, rectangle, parallelogram, trapezoid), a container of 4 coordinates with minCount=4 and maxCount=4. You can have a "Shape" type with type quadrilateral and optional coordinates, but the coordinates can't have minCount=0, maxCount=4.

And for the normal case of [0..*], that's the difference between not making an assertion and making an assertion of NONE. CreationInfo has createdUsing Tool[0..*]. But

  "profile": ["Core"],
  "created": "2011-03-13T00:00:00Z"
  ...

is logically different than

  "profile": ["Core"],
  "created": "2011-03-13T00:00:00Z".
  "createdUsing": [],
  ...

If a property is optional it doesn't need to be specified. But if it isn't optional then it must be specified even if the specification is [] (the empty container - NONE).

Have we decided that duplicates on the to side are not allowed?

No. The decision could be Yes or No, and the model represents that decision. Bag says allowed, Set says not allowed. But if the model doesn't support Set or Bag then it can't represent any decision.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also in the future there might be multiple overlapping hasFile relationships and your set structure only partially restricts that.

If there are relationships, then each one of them has an SpdxId. The to of one SpdxId can certainly (and would be expected to) overlap with the to of a different SpdxId. But there's no reason to have multiple copies of the same SpdxId (Relationship Element). Set says two copies of the identical relationship Element is meaningless, which it is.

Copy link
Member

Choose a reason for hiding this comment

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

But there's no reason to have multiple copies of the same SpdxId (Relationship Element).

I am not sure. With our amount of relationshipTypes there might be one where there is semantic meaning in being related twice. I would not generally exclude that.

Copy link
Collaborator Author

@davaya davaya Jul 9, 2023

Choose a reason for hiding this comment

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

I'm not sure what it could mean to say there is a graph with multiple copies of the same node. Copies are at the physical layer, multiple machines running copies of the same graph. But at the graph layer a node is a node, there are no copies.

@davaya davaya merged commit e8a8df3 into spdx:main Jul 24, 2023
@davaya davaya deleted the code branch July 24, 2023 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants