fix(openapi): Add inheritance beetween Event and QosStatusChangedEvent

[patrice-conil]

What type of PR is this?

• enhancement/feature

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.

ClassA:
   allOf:
      - $ref: ClassB
      - type: object
            properties:
                 prop1:

That says ClassA inherit from ClassB and anonymous inlined object containing property prop1 is replaced by:

ClassA:
   type: object
   allOf:
      - $ref: ClassB
   properties:
      prop1:

That means: ClassA extends ClassB and contains property prop1.

Changelog input

Add inheritance beetween Event and QosStatusChangedEvent
Simplify Notification payload model and align it with DeviceStatus proposal 

Additional documentation

[jlurien]

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.

[jlurien]

As commented in camaraproject/WorkingGroups#255 (review), It is OK to model the event following inheritance instead of polymorphism but we should stick as much as possible to the examples in the spec

[patrice-conil]

Comes back to common usage of allOf following suggestions of @jlurien

[jlurien]

LGTM

[hdamker]

/LGTM

[hdamker]

@akoshunyadi @eric-murray can you have a final look before I merge?

[eric-murray]

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.

[akoshunyadi]

LGTM, but then the guideline should be updated, which contains that additional layer of eventNotification

[patrice-conil]

@eric-murray,
QOS_STATUS_CHANGED added as Event example

[hdamker]

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 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?

[patrice-conil]

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 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

[jlurien]

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.

[patrice-conil]

@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?

[hdamker]

@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 camaraproject/Commonalities#8). We need to adapt in upcoming release.

[shilpa-padgaonkar]

Please take a look at camaraproject/Commonalities#8 (comment) and provide your feedback in commonalities

[patrice-conil]

Hi @hdamker, @jlurien,
Thank you for your constructive comments,
I've reintroduced EventNotification to the spec... hope this helps moving forward soon.

[hdamker]

/LGTM

[hdamker]

@jlurien @eric-murray @akoshunyadi @bigludo7 Just waiting for one other approval of the latest changes before I merge.

[bigludo7]

LGTM

[jlurien]

LGTM

[eric-murray]

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.

[patrice-conil]

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.

Example added thanks to @eric-murray