Closed patrice-conil closed 1 year ago
This new proposal to model inheritance may work but I have concerns about not being totally aligned with the OAS specification, which states that:
The OpenAPI Specification allows combining and extending model definitions using the
allOf
property of JSON Schema, in effect offering model composition.allOf
takes an array of object definitions that are validated independently but together compose a single object.
You are now moving the item that defined the child event out of the array. Even if this JSON-schema is valid and may work for code generation, it is not what it is shown in all examples in the specs, which follow the approach to model the child properties within the allOf
https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/
ExtendedErrorModel: allOf: # Combines the BasicErrorModel and the inline model - $ref: '#/components/schemas/BasicErrorModel' - type: object required: - rootCause properties: rootCause: type: string
I think it is safer to follow the examples in the spec as much as possible, as others may claim that their tools don't work well with this simplification.
Comes back to common usage of allOf following suggestions of @jlurien
@akoshunyadi @eric-murray can you have a final look before I merge?
Small point, but the Swagger editor does not create a valid example for the notification request body, so I'd suggest to put an example of type QosStatusChangedEvent
within the definition of the Event
schema to give a better idea as to what a notification will look like.
But otherwise it looks fine and I'll add my approval.
@eric-murray, QOS_STATUS_CHANGED added as Event example
An important point we should clarify before we merge:
BREAKING CHANGE: Remove EventNotification to embed Event as notification payload
The guideline defines eventNotification
with event
as one of it's attributes. The current PR would require an update of the Design Guidelines, as @akoshunyadi rightfully commented. As far as I can see DeviceLocation is following the schema of the guideline in https://github.com/camaraproject/DeviceStatus/pull/31
@patrice-conil What's the rational for this change? @bigludo7 as author of the guideline, could you review our PR as well and comment?
An important point we should clarify before we merge:
BREAKING CHANGE: Remove EventNotification to embed Event as notification payload
The guideline defines
eventNotification
withevent
as one of it's attributes. The current PR would require an update of the Design Guidelines, as @akoshunyadi rightfully commented. As far as I can see DeviceLocation is following the schema of the guideline in camaraproject/DeviceStatus#31@patrice-conil What's the rational for this change? @bigludo7 as author of the guideline, could you review our PR as well and comment?
@hdamker, As a follower of the KISS principle, I just try to keep it as simple as possible. As EventNotification does not provide any additional data and requires the creation of an additional object before posting the notification I've proposed to remove it in the PR. I agree it opens a new discussion... but as it will be reused a lot and is quite structuring maybe it's worth it (in commonalities?). What is the original motivation for defining an EventNotification that simply encapsulates an Event in a field? Do we plan to notify anything else? I will be on vacation this Friday, If you can save 5 min to discuss this point with our partners, I will follow the decision of the working group and adapt the model. Sorry to open so many questions but it is with use that we realize the problems.
BR, Patrice
I think the reason to have an event
encapsulating the data is because for notifications coming from explicit subscriptions we may have a subscriptionId
, next to the event:
{
"eventSubscriptionId": "456g899g",
"event": {
...
}
}
In QoD we have implicit notifcations`, and as Patrick comments it adds an additional level.
The decision should come from Commonalities. We may decide to move all attributes of event
to the parent level, next to eventSubscriptionId
, and remove the event
object.
If we have to take a quick decision here, I think we should stick to Commonalities, or keep it open until a new resolution is taken there, which may take some time.
@hdamker, @jlurien, I missed the eventSubscriptionId ... thanks Jose What if to keep the commonalities we keep eventSubscriptionId in EventNotification (we could set it with sessionId)? What do you think about?
@jlurien @patrice-conil My view is that if we wait for a commonality decision now, we would delay the release for several weeks. I would go with the current structure in commonalities and the proposal above from @jlurien. But that's just my view.
Beyond that I expect changes of the notification/subscription structure as soon there will be common OAS for notification endpoint on listener side(see https://github.com/camaraproject/Commonalities/issues/8). We need to adapt in upcoming release.
Please take a look at https://github.com/camaraproject/Commonalities/issues/8#issuecomment-1629615969 and provide your feedback in commonalities
Hi @hdamker, @jlurien, Thank you for your constructive comments, I've reintroduced EventNotification to the spec... hope this helps moving forward soon.
@jlurien @eric-murray @akoshunyadi @bigludo7 Just waiting for one other approval of the latest changes before I merge.
Happy to approve to progress this, but at some point we should add an explicit example for the EventNotification
callback, because the example generated by the Swagger editor includes a value for eventSubscriptionId
, which is not correct.
Happy to approve to progress this, but at some point we should add an explicit example for the
EventNotification
callback, because the example generated by the Swagger editor includes a value foreventSubscriptionId
, which is not correct.
Example added thanks to @eric-murray
What type of PR is this?
What this PR does / why we need it:
Fix ability to send QosStatusChangedEvent from client side (Telco) to the server side of /notifications (QoS API Consumer) for those who use strongly typed Languages. As QosStatusChangedEvent extends Event we can now send it as payload
Which issue(s) this PR fixes:
Fixes #174
Special notes for reviewers:
BREAKING CHANGE: Remove EventNotification to embed Event as notification payload(reverted, see bbe5812)This specification replaces multiple inheritance schemes with single inheritance. This way the generator will stop to generate anonymous XXXAllOf.java classes which are not useful at all.
That says ClassA inherit from ClassB and anonymous inlined object containing property prop1 is replaced by:
That means: ClassA extends ClassB and contains property prop1.
Changelog input
Additional documentation