Skip to content

Update to Shapeshifter specification version 3.1.0#162

Merged
tomwetjens merged 2 commits intoshapeshifter:mainfrom
tomwetjens:update-to-spec-3.1.0
May 2, 2025
Merged

Update to Shapeshifter specification version 3.1.0#162
tomwetjens merged 2 commits intoshapeshifter:mainfrom
tomwetjens:update-to-spec-3.1.0

Conversation

@tomwetjens
Copy link
Collaborator

No description provided.

Signed-off-by: Tom Wetjens <tom.wetjens@edsn.nl>
@albertGopacs
Copy link
Contributor

albertGopacs commented May 1, 2025

Als ik kijk naar hoe nu de validatie is geïmplementeerd voor MessageId's en de eigenschap 'unsolicited' dan heb ik paar vragen danwel opmerkingen:

  • mijns inziens betekent backwards compatibility dat als de eigenschap 'unsolicited' ontbreekt of null is, dat het oude gedrag vertoond wordt. Uitgaande van de specificaties (zoals ik die begrijp) betekent dit voor FlexOrder dat als unsolicited ontbreekt, FlexOfferMessageID verplicht is. Dit is niet zo in UnsolicitedFlexOrderValidator geïmplementeerd. (Voor FlexOffer zou het betekenen dat er niet gevalideerd wordt, wat nu ook zo in UnsolicitedFlexOfferValidator geïmplementeerd is.)
  • ik zie dat er al twee validators bestaan die controleren op messageId. Namelijk ReferencedFlexRequestMessageIdValidator (valideert FlexOffer) en ReferencedFlexOfferMessageIdValidator (valideert FlexOrder). Beide staan toe dat de messageID ontbreekt en controleren alleen of een eventueel messageID bestaat.
    • En ik vraag me af of het niet wenselijk is om de validatielogica die nu in de nieuwe UnsolicitedXXXValidators zit, te verhuizen naar deze twee bestaande validators.

@tomwetjens
Copy link
Collaborator Author

  • mijns inziens betekent backwards compatibility dat als de eigenschap 'unsolicited' ontbreekt of null is, dat het oude gedrag vertoond wordt. Uitgaande van de specificaties (zoals ik die begrijp) betekent dit voor FlexOrder dat als unsolicited ontbreekt, FlexOfferMessageID verplicht is. Dit is niet zo in UnsolicitedFlexOrderValidator geïmplementeerd. (Voor FlexOffer zou het betekenen dat er niet gevalideerd wordt, wat nu ook zo in UnsolicitedFlexOfferValidator geïmplementeerd is.)

You are correct. I will update the UnsolicitedFlexOrderValidator to have the correct backwards compatibility logic.

  • En ik vraag me af of het niet wenselijk is om de validatielogica die nu in de nieuwe UnsolicitedXXXValidators zit, te verhuizen naar deze twee bestaande validators.

The main reason everything is in separate validator classes currently, is that we can have different rejection reasons (see UftpValidator#getReason).

  1. Either we would have a very generic rejection reason (which might be okay),
  2. or we'd have to refactor UftpValidator:
    • Refactor isValid method so that it returns a ValidationResult (containing a reason) instead of a boolean, so it can return different reasons
    • Remove getReason() method
    • This would mean a breaking change in the library.

What would you feel is the best course of action?

Signed-off-by: Tom Wetjens <tom.wetjens@edsn.nl>
@albertGopacs
Copy link
Contributor

  • En ik vraag me af of het niet wenselijk is om de validatielogica die nu in de nieuwe UnsolicitedXXXValidators zit, te verhuizen naar deze twee bestaande validators.

The main reason everything is in separate validator classes currently, is that we can have different rejection reasons (see UftpValidator#getReason).

  1. Either we would have a very generic rejection reason (which might be okay),

  2. or we'd have to refactor UftpValidator:

    • Refactor isValid method so that it returns a ValidationResult (containing a reason) instead of a boolean, so it can return different reasons
    • Remove getReason() method
    • This would mean a breaking change in the library.

What would you feel is the best course of action?

Well, breaking changes for version 3 are out of question, I guess. And I think specific rejection reasons are a good reason to have separate validators. But an approach like jakarta validation with a ValidationResult is worth considering for version 4, I would say.

@nielsgijsen
Copy link

I approve

@tomwetjens tomwetjens merged commit a6f6066 into shapeshifter:main May 2, 2025
3 checks passed
@tomwetjens tomwetjens deleted the update-to-spec-3.1.0 branch May 2, 2025 08:35
<xs:complexType name="FlexOfferResponseType">
<xs:complexContent>
<xs:extension base="PayloadMessageResponseType">
<xs:attribute name="FlexOfferMessageID" type="UUIDType" use="optional">
Copy link
Contributor

Choose a reason for hiding this comment

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

voor een FlexOfferResponse is het FlexOfferMessageID niet optional.

<xs:extension base="PayloadMessageResponseType">
<xs:attribute name="FlexOfferMessageID" type="UUIDType" use="optional">
<xs:annotation>
<xs:documentation>MessageID of the FlexOffer message this request is based on, if applicable.</xs:documentation>
Copy link
Contributor

Choose a reason for hiding this comment

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

"if applicable" kan weer weg. En er staat "... this request is based on...", maar dat moet zijn "... this response is based on...". Ook bij FlexRequestResponseType staat dit verkeerd.

<xs:extension base="PayloadMessageResponseType">
<xs:attribute name="FlexOrderMessageID" type="UUIDType" use="required">
<xs:annotation>
<xs:documentation>MessageID of the FlexOrder that has just been accepted or rejected.</xs:documentation>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ik vraag me nu af of deze tekst niet in lijn gebracht zou moeten worden met wat er bij FlexRequestResponseType en FlexOfferResponseType staat...

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