-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding notification/subscription for Device Status #31
Conversation
New proposal following our discussion on issue #17 . Not for merging at this stage but to get team feedback - I will like to use the work we're doing here to contribute to API guidelines for subscription/notification model.
Fixed an issue on line 149
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed small issue
Removed mention to LOSS OF CONNECTIVITY & REACHABILITY Fixed example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed mention to LOSS OF CONNECTIVITY & REACHABILITY
Fixed example.
Updated: - Align with Subscription & Notification model - Splitted in 4 event type for roaming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated:
- Align with Subscription & Notification model
- Splitted in 4 event type for roaming
Solve issue #52
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solve issue #52
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @bigludo7.
I added some comments comparing it with the guidlines and some additional point to discuss with the working group.
In general I suggest we use parameters descriptions from Glossary with some adjustments but they might be helpful for the developers.
- address/mask - an IP number as above with a mask width of the form 1.2.3.4/24. | ||
In this case, all IP numbers from 1.2.3.0 to 1.2.3.255 will match. The bit width MUST be valid for the IP version. | ||
Ipv6Addr: | ||
Ipv4Address: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowing privateAddress
as a device identifier makes it easier for the device itself (or an app on the device) to make the API call. But for this API, I don't see why the device itself would need to use an API call to the network to determine its roaming (or connectivity) status. So I don't think it is needed here to support that scenario.
Having said that, an API consumer other than the device itself may know the private IP address, and so this would still be a valid way of identifying the device. In fact, it is easier for the network to identify the device by privateAddress
+ publicAddress
compared to publicAddress
+ publicPort
, so we should encourage use of privateAddress
if that is known.
So my preference is to follow QoD and Device Location when it comes to identifying the device by privateAddress
, even if the justification is not so strong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -79,8 +110,167 @@ paths: | |||
$ref: "#/components/responses/Generic500" | |||
'503': | |||
$ref: "#/components/responses/Generic503" | |||
'504': | |||
$ref: "#/components/responses/Generic504" | |||
/roaming/event-subscription: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/roaming/event-subscription: | |
/roaming/event-subscriptions: |
I think we must keep it in plural.This is what the guideline says:
Note on the operation path: The recommended pattern is to use /event-subscriptions path for the subscription operation. But API design team, for specific case, has the option to append /event-subscriptions path with a prefix (e.g. /roaming/event-subscriptions and /connectivity/event-subscriptions). The rationale for using this alternate pattern should be explicitly provided (e.g. the notification source for each of the supported events may be completely different, in which case separating the implementations is beneficial).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -170,19 +365,17 @@ components: | |||
description: The Mobile country code (MCC) as an geographic region identifier for the country and the dependent areas. | |||
type: integer | |||
CountryName: | |||
description: The ISO 3166 ALPHA-2 country-codes of mapped to mobile country code(MCC). If there is mapping of one MCC to multiple countries, then we have list of countries. If there is no mapping of MCC to any country, then an empty array [] shall be returned. | |||
description: The ISO 3166 ALPHA-2 country-codes of mapped to mobile country code(MCC). If there is mapping of one MCC to multiple countries, then we have list of countries. If there is no mapping of MCC to any country, then ["xx"] will be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description: The ISO 3166 ALPHA-2 country-codes of mapped to mobile country code(MCC). If there is mapping of one MCC to multiple countries, then we have list of countries. If there is no mapping of MCC to any country, then ["xx"] will be used. | |
description: The ISO 3166 ALPHA-2 country-codes of mapped to mobile country code(MCC). If there is mapping of one MCC to multiple countries, then we have list of countries. If there is no mapping of MCC to any country, then an empty array [] shall be returned. |
This is what we have in the main branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
type: array | ||
items: | ||
$ref: '#/components/schemas/DeviceStatusEventType' | ||
example: [ ROAMING_STATUS] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example: [ ROAMING_STATUS] | |
example: [ROAMING_STATUS] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
- name: Device status subscription | ||
description: Operation to manage subscription on device status event (roaming, connectivity) | ||
- name: Notification Callback | ||
description: Notification received on subscription listener side |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could describe it a bit more to clarify this note on the guidelines:
Note: The notification is the message posted on listener side. We describe the notification message in the CAMARA OAS but it could confusing because this endpoint should be implemented on the business API consumer side. This notice should be explicited mentioned in all CAMARA API documentation featuring notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
$ref: '#/components/schemas/SubscriptionId' | ||
event: | ||
$ref: '#/components/schemas/Event' | ||
Event: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It must be mandatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Updated the proposal after @monamok review (huge thanks for your detailed review 🥇 ) , integrated outcomes of meeting discussion to simplify the path of the subscription, and as suggested by @eric-murray aligned device definition with QoD API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the proposal after @monamok review (huge thanks for your detailed review 🥇 ) , integrated outcomes of meeting discussion to simplify the path of the subscription, and as suggested by @eric-murray aligned device definition with QoD API.
…name classes to conform to naming rules.
paths: | ||
/roaming: | ||
/notifications: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we put the notification endpoint inside POST /event-notifications as callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akoshunyadi - This is a great question. We saw that QoD API went in this direction;
My developer colleagues are not comfortable with this pattern (have the callback in the end point) as it is not really supported by code generator. A good point to discuss in Commonalities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is to have the notification endpoint defined as a callback. However, I understand that code generators can have issues with callbacks, and having the separate notifications path does comply with the design guidelines (though these make it look like the endpoint should be called eventNotifications
- maybe some editing is needed there). So I will approve this PR so that we can move towards v0.5.0.
Ongoing, I agree this would be a good topic to raise in Commonalities. I have seen three methods for defining notifications discussed:
- As a callback (as we have in QoD API)
- As a separate notifications path, with appropriate documentation (as we have here)
- As a completely separate OAS definition for use by the API consumer notification endpoint
I don't think these methods are incompatible. Why not have all three? But this may also cause issues with code generators if both a callback and separate notification path are defined, so should be discussed in Commonalities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our proposal with @bigludo7 is to provide the definition as a callback because it documents very well what is happening (and will align with how it's done in QoD). Also add a separate OAS definition to help implementers of this callback.
As you say, these approaches are not incompatible, so we can benefit from them.
We agree with you to discuss this point in the commonalities to help harmonization between APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @patrice-conil
So is the proposal to modify this PR to include the notification endpoint as a callback? Or will that wait until after the discussion in Commonalities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eric-murray,
I think it can wait until after discussion in Commonalities. I will propose a new PR after discussion if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @patrice-conil
Once there are the additional approvals from @monamok and @akoshunyadi, I can merge this.
Cleanup model
Updated swagger (event model) after @patrice-conil code review. |
$ref: '#/components/schemas/Ipv4Addr' | ||
ipv6Addr: | ||
$ref: '#/components/schemas/Ipv6Addr' | ||
phoneNumber: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would like to confirm if IMSI not be part of input for getting roaming status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CAMARA does not use IMSI as a mobile subscription identifier for any API
fix(openapi): Align with common usage of allOf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the effort @bigludo7.
I added some more comments.
expiresAt: | ||
type: string | ||
format: date-time | ||
description: date time when subscription will expire or expired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to have the expired option if we're creating a subscription? And for GETs endpoint I think we only return those that are still active, dont we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure to understand the comment. subscriptionExpireTime
is the expiration date wished by the requester. Suppose you want to have one month subscription.
for GET I'm not sure we should enforce this rule. As subscription will be monetized should we keep 'for some time' before to delete them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From our discussion, I change expiresAt
description to date time when subscription expired
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think it should be "date time when subscription will expire". We are creating a subscriptionand the response says, this subscription starts at "x time" will expire in "y time", aren't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we're aligned....
The requester can provide a date wished for subscription: subscriptionExpireTime
; fine for that.
July 5th, 2023 requester asks for a subscription till July 25,2023 - in the subscription both subscriptionExpireTime
& expireDate
will be valued to July 25th.
Subscription server could also fill a future expire date. For example a subscription server did no support subscription for more the 3 months. If today is July 5th, & requester did no provided subscriptionExpireTime
or subscriptionExpireTime
>3 month from now, then the expireDate
will be October 5th.
I assume at this point we're aligned :)
Now, what are we expecting once the expireDate
is reached?
Do we assume that the subscription resource is deleted and not anymore accessible by GET
? or does this resource is still accessible for Tracking for example? I'm not sure about an unified pattern and that's why at last I prefer the initial wording: Date time when subscription expired or will expire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sachinvodafone
I had anyway been assuming that GET /event-subscriptions
would only return active subscriptions. What is the history that you think the API consumer might still need access to for an expired subscription, remembering that CAMARA APIs are designed to be called by code and not by people?
Providing details of expired subscriptions can anyway be implementation specific. But you would need a good argument if filtering options are to be included in the API design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eric-murray Thanks for your notes. I was referring the notes between @bigludo7 and @monamok on the same topic so thought to clarify if we have reached to any alignment or not.
Regarding whether subscription history (subscription ID , creation date, expire date, deletion status) is required or not, while not all API consumers may require access to historical data of expired subscriptions, there are scenarios where it can still be relevant and valuable for client. For example:
- In Audit and Compliance where it may be necessary to maintain a complete audit trail of all subscriptions, including expired/deleted ones.
- In Billing and Financial Records where access to these data may be valuable for accounting, billing reconciliation, or financial reporting purposes.
- In Analysis and Reporting where historical data on expired subscriptions may be useful for analytical purposes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current API definition would allow GET /event-subscriptions
to return expired subscriptions, but I don't see that any of the reasons you list would make this mandatory. API consumers can keep their own records of expired subscriptions (there is not that much data per subscription), and the API provider would anyway need to keep records for the reasons you list, but that does not mean they need to make them available to the API consumer through the CAMARA service API.
So I still don't see any reason to mandate that GET /event-subscriptions
returns expired subscriptions, and hence no reason to add filtering options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are sharing only active subscriptions, the description of the "ExpireAt" field should indicate the "date and time when the subscription will expire" instead of mentioning both "date time when subscription will expire or expired"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already said that API providers can give details of expired subscriptions if they choose to implement that. But there is no reason to mandate it, nor to prohibit it.
API consumers should not rely on the API for historical records of their subscriptions. They need to be more organised than that.
$ref: '#/components/schemas/CreateRoamingSubscription' | ||
required: true | ||
responses: | ||
'201': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the point of view of having synchronous response in create/deleteSubscriptions.
Having said that, during notification guidelines track, as per PR camaraproject/WorkingGroups#173, the response codes for successful operations were not discussed. Thus no decision was made about including only synchronous operations or we could include both mechanisms for “write” subscription operations (in case of this API. POST -createSubscription-, DELETE -deleteSubscription-). In subscription section of the guideline specifies the error codes for eachpoint, but not the success codes. There are some examples given for success cases, but other than that, no clear instruction which says we can't consider asynchronous responses.
On the other hand, in https://github.com/camaraproject/WorkingGroups/blob/main/Commonalities/documentation/API-design-guidelines.md#32-http-response-codes, it is reflected the use of 202 Accepted for asynchronous process.
In Telefonica, an asynchronous-based subscription model is designed, because this “write” operations manage actions in several underlying systems and that could take some time. For that reason, we would like to support both:
- 201 and 202 for create (we are returning required parameter eventSubscriptionId anyway)
- 202 and 204 for delete
In case we allow 202 responses, we should mention in the guideline that GET /subscriptions/{subscriptionId} must be used to ensure existence of the subscription.
Update swagger after @monamok review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update swagger after @monamok review
RoamingEventType: | ||
type: string | ||
description: | | ||
ROAMING_STATUS - Event triggered when the device switch from roaming ON to roaming OFF and conversely |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the event generated by a ROAMING_STATUS event subscription, both ON/OFF as well as country code/name is returned.
I thereby assume that subscribing to ROAMING_STATUS will give you notifications for on, off as well as change of country.
For clarity this should be reflected in the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I, with a UK SIM, were in, for example, France and travelled to Germany, my roaming
state would remain unchanged at true
. So I would not expect a ROAMING_STATUS
change event to be generated.
But if this is not clear, then indeed the description should be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good comment Eric. Then I am wondering about the ROAMING_CHANGE_COUNTRY event type. That clearly indicate that I would get a roaming status notification in the situation you describe above to indicate that you move from one country to another while roaming.
I was thinking of the ROAMING_ON, ROAMING_OFF and ROAMING_CHANGE_COUNTRY as a subset of ROAMING_STATUS. Maybe that is not the case after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is the other way around. ROAMING_ON
and ROAMING_OFF
are subsets of ROAMING_STATUS
, which is itself a subset of ROAMING_CHANGE_COUNTRY
. So, in my example, if I was to return to the UK, that would generate a ROAMING_OFF
, ROAMING_STATUS
and ROAMING_CHANGE_COUNTRY
event notification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we are saying the same thing but if I do not misunderstand the 3GPP standard you will get a notification only when changing from HPLMN to VPLMN and vice versa. You will not get a notification when moving from one VPLMN to another VPLMN. Even though there are implementations that will give you also notifications when changing between VPLMNs we cannot rely on it. Thereby the name of the event ROAMING_CHANGE_COUNTRY is a bit misleading as it indicates that you will always get such notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that part understood! My question is, what is the difference between subscribing only to ROAMING_STATUS
or subscribing to both ROAMING_ON
and ROAMING_OFF
? What are the benefits of subscribing to ROAMING_STATUS
only apart of having to subscribe only to one event?
In the current model, if I subscribe only to ROAMING_ON
because I am only interested in that event, I do not receive information on which country is the SIM roaming, I am forced to subscribe also to ROAMING_STATUS
to get that information, so I end up having notifications on events I don't want/need or having to call the /roaming endpoint once I receive the ROAMING_ON
notification right?
And If we say that the ROAMING_STATUS
represents a change in roaming status, why don't we include the country change in the actions that cover this event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is that you only have one subscription to manage instead of two. The only reason to subscribe to both ROAMING_ON
and ROAMING_OFF
rather than ROAMING_STATUS
, is because you need separate subscriptions, possibly with separate notification endpoints and certainly with different eventSubscriptionId
. Otherwise, use ROAMING_STATUS
.
There is an argument for adding roaming country for ROAMING_ON
notifications, but I don't know the use case for those events. It may be that the use case only requires the API consumer to know that the device is now roaming, but it does not matter where. This is not a use case that Vodafone is interested in.
ROAMING_STATUS
notifications can include the new country that triggered the event. Have a look at the example in the API definition. If a client wants to know what the previous country was, they need to store the previous notification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking , even if we include country information in the ROAMING_ON and ROAMING_OFF events, can't we control which clients are authorized to view the country code information and restrict it for those who don't have the necessary privileges . Sorry , if doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey then, fine from my side!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking , even if we include country information in the ROAMING_ON and ROAMING_OFF events, can't we control which clients are authorized to view the country code information and restrict it for those who don't have the necessary privileges . Sorry , if doesn't make sense.
Information that can be provided to API consumers will be controlled by OAuth scopes. For notifications, this will be the OAuth scope used to create the notification. It complicates the business logic of course, but that is the nature of APIs that have different behaviour dependent on the API consumer.
This is being discussed with the Identity and Consent Sub-Project, so is not something we should try to second-guess here. If we think country information would be useful for some use cases, then the possibility to include it should be added even if, in the end, it cannot be provided to all API consumers.
examples: | ||
ROAMING_STATUS: | ||
value: | ||
subscriptionId: 123654 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I'm new here so please ignore this if it causes any noise :D
Shouldn't the property in the examples also be eventSubscriptionId
? It happens in all examples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fernandopradocabrillo
@bigludo7
Can you update the notification examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @eric-murray,
@bigludo7 is OoO, I made PR #64 to fix typos
BR,
Patrice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @patrice-conil. I merged these changes into the PR, but that invalidates previous approvals.
@sachinvodafone, @akoshunyadi, @JoachimDahlgren Can you review again?
@fernandopradocabrillo Can you review and approve for Telefonica?
fix(openapi): replace subscriptionId by eventSubscriptionId
de445db
eventTime: 2023-01-17T13:18:23.682Z | ||
eventDetail: | ||
device: | ||
phoneNumber: 123456789 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example given for the events all include phoneNumber as example of the identification of the device. When we set up a subscription there are several ways to identify the device for which we want to subscribe to device status. All those being optional.
I would assume that whatever you input as an identifier is also what you will receive back with the event. The reason for that would be that if the application have not been trusted with knowing the actual phoneNumber of the device this should not be exposed when calling the Device Status API. Is this a common understanding and in that case should it be mentioned in the documentation of the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is my understanding - the API should return at most the same device identifiers as were provided by the client, but could maybe remove some if they were not valid or not used by the API provider.
For example, if the API client provides both phoneNumber
and ipv4Address
, but of those two the implementation only uses phoneNumber
to identify the device, then only phoneNumber
should be included in responses. But that is my view - it would not be wrong to return ipv4Address
as well even if that could be interpreted as confirming that the device is using that IP address when that has not been checked.
My view is that returning these values also gives the API provider an opportunity to "clean up" the formatting to show the API client the preferred field formatting. As we tighten up format patterns, there is less scope for the API client to use formats that are difficult to parse but, for example, if the client provided +123456789
and the API returned 123456789
, that would indicate that we would prefer the optional +
not to be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with both of you and not sure it we need an OAS update as of now (but sure it must be in the documentation). The answer must provide same identifier than the one provided in the response
About you example @eric-murray I'm always a bit uncomfortable when client provides more than one identifier (here phoneNumber
and ipv4Address
) - Does it means that a consistency check must be done by the server ? and then what is the expected behavior if for any reason they did not match?
Not specific about this api but probably requires some 'ruling' as it could occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a topic for the TSC, but the point is that the current definition does allow the API consumer to provide more than one device identifier. This is inherited from QoD, where the motivation to allow more than one is that the device may be dual-stack, and we needed a way that the API consumer could specify if they wanted to implement the enhanced QoS for the IPv4 flow, IPv6 flow or both (providing just MSISDN or External Id would imply both if dual-stack).
Obviously this motivation does not apply here, but also I feel we should encourage the API consumer to provide the device identifiers they know, because some are easier to work with than others. It is much easier to determine the roaming status of a MSISDN than a public IP address, and if we restricted the number of device identifiers to one, they might just give us the IP address.
My feeling it is up to the API implementor how they process multiple device identifiers. They can do a consistency check if they want, or just take the identifier most convenient for their implementation. I don't really see that we need to standardise here. Device identifiers are inherently CSP specific.
properties: | ||
device: | ||
$ref: '#/components/schemas/Device' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this eventDetail should contain the property terminationReason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be an additional property in the SubscriptionEnds
schema.
But what reasons are there for the subscription ending other than it has expired (i.e. the subscriptionExpireTime
has been reached)? I can see that there may be other reasons in theory, but I don't see that they will be encountered in practice. Why would the API provider choose to terminate the subscription early?
In any case, the API client can infer "other reasons" for subscription termination if subscriptionExpireTime
has not been reached.
So I don't have a problem with adding a terminationReason
property, but what would be the reasons other than SUBSCRIPTION_EXPIRED
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our guideline says "For this specific event, the eventDetail must feature terminationReason attribute."
I QoD we have beside the expiry also a NETWORK_TERMINATED reason, I think this is also possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, fair point that this is a requirement of the CAMARA API Design Guidelines.
The example used in the design guidelines suggests that terminationReason
is just a freeform string. So can we just add the property with no pre-defined values? Or should it be an enum, maybe {SUBSCRIPTION_EXPIRED
, NETWORK_TERMINATED
}?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it could be both... I tend to use enums, it's more uniform through all the APIs and can be evaluated programmatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I added the terminationReason property as an enum
Added terminationReason property to SubscriptionEnds schema
As @fernandopradocabrillo steps in for @monamok I removed review request from Mona. |
@fernandopradocabrillo May I merge or do you want to review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
New proposal following our discussion on issue #17 .
Not for merging at this stage but to get team feedback - I will like to use the work we're doing here to contribute to API guidelines for subscription/notification model.