search

camaraproject / DeviceStatus

Repository to describe, develop, document and test the Device Status API family
Apache License 2.0
11 stars 33 forks source link

Minor corrections in Subscription model before release #84

Closed fernandopradocabrillo closed 7 months ago

fernandopradocabrillo commented 7 months ago

Problem description

data property should be required in callback request body In the operacion "postNotification", the POST /subscriptions callback, we have the property "data" as optional, but shouldn't it be required? Otherwise it exists the posibility of sending an empty notification. In the Sim Swap Subscriptions yaml we have it as required to avoid this issue.

subscriptionDetail.device property should be required We have device as optional in all subscription endpoints, request body and responses. I think this one should be required too. For the /roaming and /connectivity operations it is required so the system knows what devices are you asking for. It should be the same for the notifications. If we leave it like it is right know a client can create a subscription and not specify for what device. The same for the the GET endpoints, right now, it is allowed to not provide information regarding the device the subscriptions belongs to.

akoshunyadi commented 7 months ago

I also think that we won't have events without this context specific info. However the CloudEvents spec says that the data field is optional. So to be conform we should keep it optional. But that endpoint is anyway implemented on the client side, which can receive any CloudEvents, not just Camara ones.

In the subscription we are free, so there I think we could add the device, since also for me no DeviceStatus use cases is known which doesn't need a device.

akoshunyadi commented 7 months ago

The PR for that should be a simple one, I think, if everybody's agreed, we could close it quickly. @bigludo7 @eric-murray @sachinvodafone @JoachimDahlgren @SyeddR

bigludo7 commented 7 months ago

Hello @fernandopradocabrillo I'm not sure for subscriptionDetail.device as device is not present in Carrier billing subscription. Perhaps the rule should be: if device is present in data then it is recommended to be mandatory.

For data, I prefer also to provide a recommendation to make it mandatory but not enforce it. Indeed, we can imagine in future some case of the event type by itself could be enough

sachinvodafone commented 7 months ago

Regarding "data", what I understood from Camara API Design Guideline that "its occurence can be set to mandatory by given CAMARA API " so I believe, we can set as "mandatory" for device status.

sachinvodafone commented 7 months ago

Regarding "subscriptionDetail.device", I am aligned that we can include it as "mandatory" for device status purposes, but this requirement should not extend to all other APIs. Therefore, we need to update the API design document accordingly.

Additionally, I observed that in the documentation, in example section, "ueId" is listed under "subscriptionDetail" instead of "device". I believe , given example is not aligned with our current device status's changes:

Screenshot 2023-12-01 at 15 42 09
bigludo7 commented 7 months ago

Hello Please discard my comment - I was thinking we discussed at Commonalitie level. Fully aligned with @fernandopradocabrillo proposal here for this API.

hdamker commented 7 months ago

As expressed within https://github.com/camaraproject/DeviceStatus/issues/81 there is some urgency to get this issue done as it is blocking the v0.5.0 release candidate.

data property should be required in callback request body In the operacion "postNotification", the POST /subscriptions callback, we have the property "data" as optional, but shouldn't it be required? Otherwise it exists the posibility of sending an empty notification. In the Sim Swap Subscriptions yaml we have it as required to avoid this issue.

I understand here, that the data object within the specific Event schemata should be made mandatory, right? E.g. in EventRoamingStatus, EventRoamingOn, ... just not sure if this need to explicitly defined.

subscriptionDetail.device property should be required We have device as optional in all subscription endpoints, request body and responses. I think this one should be required too. For the /roaming and /connectivity operations it is required so the system knows what devices are you asking for. It should be the same for the notifications. If we leave it like it is right know a client can create a subscription and not specify for what device. The same for the the GET endpoints, right now, it is allowed to not provide information regarding the device the subscriptions belongs to.

This is agreed as far as I can see.

@fernandopradocabrillo @akoshunyadi can one of you asap create the PR for it? In Spain are this week holidays and tomorrow a bridge day, maybe it is faster if @akoshunyadi is taking it.