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

Issue 572 resolution #573

Closed
wants to merge 2 commits into from
Closed

Conversation

Klowes
Copy link
Contributor

@Klowes Klowes commented Jun 1, 2024

Correcting Multiplicity of @context properties where 2 items are required

Correcting Multiplicity of @context properties where 2 items are required
@Klowes
Copy link
Contributor Author

Klowes commented Jun 1, 2024

fixes #572

@justinpitcher
Copy link
Contributor

Yeah, this looks like an oversight to me (errata level). Not sure if this has an impact on the certification suite.

@ottonomy
Copy link
Contributor

@xaviaracil does the 1EdTech tooling that autogenerates various things have support for 2..*/n..* multiplicity?

@xaviaracil
Copy link
Collaborator

The common_credentials.line file is used by internal 1EdTech tooling to define the specification's model. From this model, various artifacts are automatically generated, such as JSON schemas, Open API definition files, some sections of the specification document, etc.

The file format is thus suited for these tools. Regarding the cardinality of properties in a class, the tool expects one of the following: 0..1, 1, 0..*, 1..*, which corresponds to "zero or one", "exactly one", "zero or many", and "one or many". There isn't this concept of "two or many," and merging this change will effectively prevent the tool from loading the file, preventing the generation of the derived artifacts.

However, thanks to the next directive in the file (Constraint items [https://www.w3.org/ns/credentials/v2, /^https:\/\/purl\.imsglobal\.org\/spec\/ob\/v3p0\/context(-3\.\d\.\d)*\.json$/]), we are telling the model that @contextshould have, at least, these two values, making the tooling generate the artifacts with the right minimumValue (see line 10 of https://purl.imsglobal.org/spec/ob/v3p0/schema/json/ob_v3p0_achievementcredential_schema.json):

    "@context" : {
      "type" : "array",
      "minItems" : 2,
      "items" : [ {
        "enum" : [ "https://www.w3.org/ns/credentials/v2" ]
      }, {
        "type" : "string",
        "pattern" : "^https:\\/\\/purl\\.imsglobal\\.org\\/spec\\/ob\\/v3p0\\/context(-3\\.\\d\\.\\d)*\\.json$"
      } ],
      "additionalItems" : {
        "$ref" : "#/$defs/Context"
      }
    }

The only misunderstanding that this 1..* can lead to is in the documentation of the class in the spec document (https://www.imsglobal.org/spec/ob/v3p0#achievementcredential), where the multiplicity column of the @contextattribute displays [1..*]. However, anyone can see in the description of the property that a minimum of two items are required:

The value of the @context property MUST be an ordered set where the first item is a URI with the value 'https://www.w3.org/ns/credentials/v2', and the second item is a URI with the value 'https://purl.imsglobal.org/spec/ob/v3p0/context-3.0.3.json'.

With all the above said, I wouldn't merge this PR.

@Klowes
Copy link
Contributor Author

Klowes commented Jun 14, 2024

@xaviaracil where is this tool.

@xaviaracil
Copy link
Collaborator

@Klowes This tool is internal within 1EdTech. We can always add this functionality by adding a new cardinality of 2..* to the allowed list.

@Klowes
Copy link
Contributor Author

Klowes commented Jun 14, 2024

@xaviaracil That seems appropriate to me.

Perhaps we should also add 3..* to the allowed list as per CLR 2.0 ClrCredential's @context property requiring 3 items

I'd also be willing to explore deriving the cardinality/Multiplicity from the specification's schema itself in a build step. This would keep the documentation in step with the real source of truth as well as unlock support for n..* cardinality/Multiplicity. We'd also have to update the validation in turn, but if a build step generated this text we need not validate it.

@xaviaracil
Copy link
Collaborator

Not merging as fixing has been done via internal tooling

@xaviaracil xaviaracil closed this Jul 31, 2024
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.

4 participants