ids-specification icon indicating copy to clipboard operation
ids-specification copied to clipboard

Issues in JSON-Schema Definition

Open schoenenberg opened this issue 1 year ago • 11 comments

Hi,

I am currently going in detail through JSON-Schema Definitions and I found some inconsistencies on these definitions. I am not sure how these were created - manually or generated. Please triage them and if necessary fix them:

  • [x] Typo: compare https://creativecommons.org/schema.rdf https://github.com/International-Data-Spaces-Association/ids-specification/blob/524039dcf1d52eceb9acf00c90c7a237c97afd95/negotiation/message/schema/contract-schema.json#L291
  • [ ] Not defined: https://github.com/International-Data-Spaces-Association/ids-specification/blob/524039dcf1d52eceb9acf00c90c7a237c97afd95/negotiation/message/schema/contract-schema.json#L330
  • [x] Differs from ODRL definition: see https://www.w3.org/ns/odrl/2/ODRL22.ttl https://github.com/International-Data-Spaces-Association/ids-specification/blob/524039dcf1d52eceb9acf00c90c7a237c97afd95/negotiation/message/schema/contract-schema.json#L326
  • [x] JSON-Schema Reference instead of a definition name: https://github.com/International-Data-Spaces-Association/ids-specification/blob/524039dcf1d52eceb9acf00c90c7a237c97afd95/negotiation/message/schema/contract-schema.json#L105
  • [x] @target instead of odrl:target: https://github.com/International-Data-Spaces-Association/ids-specification/blob/84101623fa57d347f5440932a0a31f881e682d57/negotiation/message/schema/contract-schema.json#L156
  • [x] Agreement recursively referring to Agreement (to what purpose? same agreement, but different permissions?): https://github.com/International-Data-Spaces-Association/ids-specification/blob/84101623fa57d347f5440932a0a31f881e682d57/negotiation/message/schema/contract-schema.json#L118 https://github.com/International-Data-Spaces-Association/ids-specification/blob/84101623fa57d347f5440932a0a31f881e682d57/negotiation/message/schema/contract-schema.json#L21
  • [ ] References to odrl:prohibition without definition of it: https://github.com/International-Data-Spaces-Association/ids-specification/blob/84101623fa57d347f5440932a0a31f881e682d57/negotiation/message/schema/contract-schema.json#L97 https://github.com/International-Data-Spaces-Association/ids-specification/blob/84101623fa57d347f5440932a0a31f881e682d57/negotiation/message/schema/contract-schema.json#L147
  • [ ] The dspace:timestamp is basically a RFC3339 timestamp: https://github.com/International-Data-Spaces-Association/ids-specification/blob/84101623fa57d347f5440932a0a31f881e682d57/negotiation/message/schema/contract-schema.json#L134
  • [x] AbstractPolicyRule does not contain odrl:target: https://github.com/International-Data-Spaces-Association/ids-specification/blob/84101623fa57d347f5440932a0a31f881e682d57/negotiation/message/schema/contract-schema.json#L72

I will extend the list when I find more inconsistencies.

schoenenberg avatar Feb 15 '24 13:02 schoenenberg

Great observations, I already prepared a fix for the first and third one.

RightOperand is not defined on purpose, as it can appear in many ways in JSON. If you can provide a pattern, very much appreciated!

The construct for Offer is also on purpose, due to the fact that we need to express that target is forbidden in most cases (everytime the schema validation goes through the Offer path) but not always (when coming through MessageOffer directly).

sebbader-sap avatar Feb 16 '24 17:02 sebbader-sap

Let's use #236 to derive a solution.

sebbader-sap avatar Feb 16 '24 17:02 sebbader-sap

The construct for Offer is also on purpose, due to the fact that we need to express that target is forbidden in most cases (everytime the schema validation goes through the Offer path) but not always (when coming through MessageOffer directly).

@sebbader-sap I cannot completely follow what you are describing. But from what I understood you are referring with target to the Agreement.odrl:target, which is three references away. To be honest I think this is a way to complex data structure. Just imagine how somebody should implement it. You need separate classes/structs anyway, otherwise you are not able to implement a data structure, which in one case has a required attribute and in another case it is not required. A specs purpose is to be implemented, therefore it should strive for simplicity and not complexity. I would suggest to refactor this, I can make a suggestion but I need some guidance what else depends on it and needs to change accordingly.

Actually I was referring with the fourth issue to the fact, that this Json-Schema definitions-part is a key-value map and the key in this case is #/definitions/Offer instead of Offer.

schoenenberg avatar Feb 19 '24 10:02 schoenenberg

the key in this case is #/definitions/Offer instead of Offer.

I see, this is of course a correct finding but already merged, see https://github.com/International-Data-Spaces-Association/ids-specification/blob/36960607a67793e3fc5089655102ac6d5b9b5445/negotiation/message/schema/contract-schema.json#L105

sebbader-sap avatar Feb 27 '24 11:02 sebbader-sap

We need to continue this discussion as soon as the Eclipse Project has been established. As the 2024-1 release in this repo is nearly done, any further corrections / extensions need to happen in the new project.

sebbader-sap avatar Feb 27 '24 11:02 sebbader-sap

@sebbader-sap What is the status on this? I need these reported issues to be fixed or clarified. Otherwise I am partly blocked and cannot further review it..

Please also add the bug label to this issue, so that it gets better visibility..

schoenenberg avatar Mar 14 '24 12:03 schoenenberg

@schoenenberg the problem is the current situation between the work inside IDSA (past) and Eclipse (future). Therefore, the general activity here is quite low. It will get better as soon as the new setup has been established.

sebbader-sap avatar Mar 21 '24 12:03 sebbader-sap

Adding another yet one:

  • [x] Context in line 58 is missing a comma

sebbader-sap avatar Mar 28 '24 07:03 sebbader-sap

AbstractPolicyRule does not contain odrl:target:

The clause with not(required) didn't really work out as intended, therefore, this line has been deleted.

sebbader-sap avatar Jun 20 '24 08:06 sebbader-sap

Open topics for the Eclipse Dataspace Group:

  1. Schema definition of the RightOperand
  2. Decide on odrl:prohibition - is it needed? If so, how shall it be modelled?

@ssteinbuss please move these topics to the new repo. Apart of that, I regard the findings of this issue as solved as I also, for now, do not see a need to change the timestamp format.

sebbader-sap avatar Jun 20 '24 08:06 sebbader-sap

[...] Apart of that, I regard the findings of this issue as solved as I also, for now, do not see a need to change the timestamp format.

@sebbader-sap I think this is rather unconventional. There are standardized timestamp formats defined like RFC3339, which is pretty similar to the Regex defined there. I compared the Regex with the definition from RFC3339 and the differences leave room for interpretation, which results in not compatible or complex implementations.

If there is no reason for it to differ from the standard, why reinventing the wheel?

schoenenberg avatar Jul 10 '24 10:07 schoenenberg