camaraproject / QualityOnDemand

Repository to describe, develop, document and test the QualityOnDemand API family
https://wiki.camaraproject.org/x/zwOeAQ
Apache License 2.0
37 stars 60 forks source link

Simplify Event Model for Notification in v0.9.0 #174

Closed patrice-conil closed 11 months ago

patrice-conil commented 1 year ago

I'm not sure to understand why we have Event, EventBase and QosStatusChangedEvent. IMHO we can define an Event that contains all EventBase properties and a specialized version QosStatusChangedEvent. See below.

 Event:
      description: The event being notified
      type: object
      required:
        - eventId
        - eventType
        - eventTime
      properties:
        eventId:
          $ref: "#/components/schemas/EventId"
        eventType:
          $ref: "#/components/schemas/EventType"
        eventTime:
          $ref: "#/components/schemas/EventTime"
      discriminator:
        propertyName: eventType
        mapping:
          QOS_STATUS_CHANGED: "#/components/schemas/QosStatusChangedEvent"

QosStatusChangedEvent:
      type: object
      allOf:
          - $ref: "#/components/schemas/Event"
      required:
          - sessionId
          - qosStatus
      properties:
          sessionId:
                $ref: "#/components/schemas/SessionId"
          qosStatus:
                $ref: "#/components/schemas/EventQosStatus"
          statusInfo:
                $ref: "#/components/schemas/StatusInfo"

This way, returning a QosStatusChangedEvent as an event is quite simple because QosStatusChangedEvent is derived from Event. There's also no need to group specific properties into an eventDetail, someone who wants to ignore the detailed information will just have to cast the specialized object to an Event.

jlurien commented 1 year ago

Hi @patrice-conil, We already had this discussion while commenting the PR by @akoshunyadi. Indeed this proposal as enhancement is similar to the second option I proposed. I see you commented there that both options worked for you as well. Akos chose the other one, not sure if there was a reason for that preference.

Regarding the use of eventDetail, that is indicated explictly in the guidelines in Commonalities.

patrice-conil commented 1 year ago

Hi @jlurien, No problem for eventDetail ... it only impact structure of object so I can live with it. ;) But I have a real problem to solve when QosStatusChangedEvent doesn't extend Event. To be able to send a QosStatusChangedEvent in notification we need that this object derive from Event or we have to forge the Json manually.

patrice-conil commented 1 year ago

To keep eventDetail (to be compliant with commonalities) what do you think about this new version:

 Event:
      description: The event being notified
      type: object
      required:
        - eventId
        - eventType
        - eventTime
      properties:
        eventId:
          $ref: "#/components/schemas/EventId"
        eventType:
          $ref: "#/components/schemas/EventType"
        eventTime:
          $ref: "#/components/schemas/EventTime"
      discriminator:
        propertyName: eventType
        mapping:
          QOS_STATUS_CHANGED: "#/components/schemas/QosStatusChangedEvent"

QosStatusChangedEvent:
      type: object
      allOf:
          - $ref: "#/components/schemas/Event"
      required:
          - eventDetail
      properties:
         eventDetail:
               type: object
               properties:
                    sessionId:
                          $ref: "#/components/schemas/SessionId"
                    qosStatus:
                          $ref: "#/components/schemas/EventQosStatus"
                    statusInfo:
                          $ref: "#/components/schemas/StatusInfo"