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

feat: add message schemas and shapes #3

Merged
merged 9 commits into from
Feb 22, 2023
Merged

Conversation

sebbader-sap
Copy link
Contributor

@sebbader-sap sebbader-sap commented Dec 20, 2022

Closes #17
Closes #18
Closes #19

},
"ids:rejectionReason": {
"type": "string",
"enum": [
Copy link
Contributor

@jimmarino jimmarino Dec 20, 2022

Choose a reason for hiding this comment

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

I don't think we should define this (the enum)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already part of the current info model. We can take a look if there is a different vocabulary out there. Not defining its values is however not an option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not an option? If you leave it open as String, one could add information as needed. And its processing COULD be defined in the protocol binding - if you want to enforce a unified processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from our call:

  • enum is not the right keyword
  • syntactic or message-related errors must result in protocol-native error codes
  • we can recommend certain "well-known" values and a pattern (e.g. "COUNTER_OFFER_NOT_ACCEPTABLE")
  • Idea: move the rejectionReason to the NegotiationTerminationMessage field terminationReason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update the files accordingly.

@sebbader-sap sebbader-sap marked this pull request as ready for review January 20, 2023 16:55
@sebbader-sap
Copy link
Contributor Author

@jimmarino @juliapampus I am done with the first iteration in the message schemas.

catalog/catalog.protocol.md Outdated Show resolved Hide resolved

**Response**: [ContractNegotiation](./message/contract.negotiation.json) containing the negotiation id or ERROR.
**Response**: [ContractNegotiationMessage](./message/contract-negotiation-message.json) containing the negotiation id or ERROR.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. The response should not be a message but the object, in this case ids:ContractNegotiation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

dct:description : MultilanguageProperty
}

enum ids:RejectionReason {
Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't want to predefine rejection reasons, right? Because the error code should be protocol binding specific. So just ids:errorCode or ids:code of type String.

@@ -1,7 +1,7 @@
{
"@context": "https://w3id.org/idsa/v5/context.json",
"@id": "urn:uuid:a343fcbf-99fc-4ce8-8e9b-148c97605aab",
"@type": "ids:ContractNegotiation",
"@type": "ids:ContractNegotiationMessage",
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, this thing here defines the ids:ContractNegotiation that should not be wrapped into a ContractNegotiationMessage as response.

@type : "ids:ContractNegotiationTerminationMessage"
ids:processId : String
ids:code : ids:TerminationCode
ids:reason : ids:Reason
Copy link
Contributor

Choose a reason for hiding this comment

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

Referring to my comment above, we should keep ids:code and not replace it by ids:rejectionReason. So, IMO it is okay to have this reason value as an add-on. Nevertheless, I don't think we should put effort in pre-defining ids rejection reasons.

@sebbader-sap
Copy link
Contributor Author

sebbader-sap commented Feb 15, 2023

I have finished the main work on the schemas. A colleague of mine will do a quality check and also provide schemas for DCAT Catalogs and ODRL later.
Following topics are open:

  • TransferSuspensionMessage or not?
  • Making ids:code --> String makes it unusable for any interoperable use case. Either we remove it or provide an (extendable) list of initial codes. Actually, Clarify the Attributes of the ContractNegotiationTerminationMessage #8 asks the same question. We can discuss it there and fix it outside of this PR.
  • same for rejectionReason
  • Is processId optional?

@juliapampus
Copy link
Contributor

juliapampus commented Feb 16, 2023

I think some changes need a dedicated approval/discussion @jimmarino

  • ContractAgreementVerificationMessage has undiscussed content (?)
  • ids:rejectionReason is introduced as an enum (+ inconsistent with transfer message JSONs)
  • dct:description is introduced for some objects (why at all, or why not for all objects?)
  • ids:code is introduced as enum (not everywhere)
    • TransferTerminationMessage introduces ids:code as object
  • incosistent naming of ids:reasons / ids:reasons (I remember we agreed on singular?)
  • in contract.negotiation.protocol.md line 86, callbackAdress is defined as URL

I have some local cleaned up history, however, this will require more effort than just cherry-picking some commits/changes. How to proceed?

@juliapampus juliapampus self-assigned this Feb 17, 2023
@juliapampus juliapampus force-pushed the sebbader/schema-shapes branch from ecc5503 to 6a4be50 Compare February 17, 2023 10:34
@juliapampus juliapampus force-pushed the sebbader/schema-shapes branch from 6a4be50 to b2c5efe Compare February 17, 2023 14:10
@juliapampus juliapampus force-pushed the sebbader/schema-shapes branch from b2c5efe to 6950efe Compare February 17, 2023 14:52
@juliapampus juliapampus requested review from jimmarino and removed request for jimmarino February 17, 2023 14:53
@juliapampus
Copy link
Contributor

Relates #35

@juliapampus juliapampus changed the title Message Schemas feat: add message schemas and shapes Feb 17, 2023
@juliapampus juliapampus force-pushed the sebbader/schema-shapes branch from 6950efe to 281f5fb Compare February 17, 2023 15:17
@sebbader-sap
Copy link
Contributor Author

@juliapampus your changes are fine with me, thanks for the effort!
The PR can be merged from my end. Additional tasks, e.g. #39, can be done in a follow-up PR.

Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

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

Due to the length, it's a bit hard to go through specifics. I'm fine in principle as long as the enums are removed/

@juliapampus juliapampus merged commit 15be6d2 into main Feb 22, 2023
@juliapampus juliapampus deleted the sebbader/schema-shapes branch February 22, 2023 11:58
sebbader-sap pushed a commit that referenced this pull request Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants