New API for location subscriptions #74
Conversation
resolved review comments
removed extra boolean variables
Add device and area
Changed yaml file based on last review comments
Resolved review comments
…ptions-api-with-event-subscription-endpoint Create location-subscriptions.yaml
chia2017
changed the title
chia2017
changed the title
chia2017
changed the title
There was a problem hiding this comment.
It is very well aligned with Commonalities, so nothing to comment there.
- Area and Circle models are being changed in another ongoing PR (fix/ location-verification: remodel-Area-inheritance #77). We will have to align there also here. We may discuss at this point if it makes sense to have a separated file with common schemas for Device and Area
- I would allow to subscribe to both events, for same Device and Area, in the same subscription
| default: http://localhost:9091 | ||
| description: API root | ||
| basePath: | ||
| default: location-subscription/v0 |
There was a problem hiding this comment.
We have to choose a more specific name for this API, as in future we are likely to have another location subscription API for geotracking. This one is related to location verification or geofencing use case. There is an open issue to choose this name: #69
Once chosen, we have to align it here and also in title, descriptions, etc
There was a problem hiding this comment.
I looked at issue #69. It seems still this issue is open and you did not finalize the decision about naming. Once is finalized I can change the name of API.
There was a problem hiding this comment.
it seems that geofencing is the name with more consensus, so you can apply it
There was a problem hiding this comment.
Thanks a lot !
Few comments.
Main one is to have a common CAMARA decision on using callback or specific endpoint. Both works but my developer feedback told me that callback did not work well with code generator.
We need also alignment on subscription starts/expires date.
| schema: | ||
| $ref: '#/components/schemas/CreateEventSubscription' | ||
| required: true | ||
| callbacks: |
There was a problem hiding this comment.
We did not have right now a decision for using call back or providing a standard endpoint for Notification.
There is is an issue in commonalities to discuss this camaraproject/Commonalities#8
Please check this last commit. All the review comments applied.
|
@chia2017 you still have to define the details in the specific events being notified, that is the device and area. |
updated eventDetail
|
|
||
| Event: | ||
| description: The event being notified | ||
| anyOf: |
There was a problem hiding this comment.
There was a discussion in QoD to favour allOf vs anyOf to model the event, due to better compatibility with some generators and coding languages, you can check the PR here: camaraproject/QualityOnDemand#177
This was proposed by Orange, @bigludo7
Deleted EventBase object, now all other events inherit from Event object and there is no "anyOf" in the Event anymore
| description: | | ||
| AREA_ENTERED - Location event triggered when the device entered the given area | ||
| AREA_LEFT - Location event triggered when the device left the given area | ||
| SUBSCRIPTION_ENDS - It is inform listener request that subscription terminates. |
There was a problem hiding this comment.
@bigludo7 Here we are adding SUBSCRIPTION_ENDS event in the operation to create a subscription and I was checking yesterday's PR from Device Status and we didn't add it there: https://github.com/camaraproject/DeviceStatus/blob/main/code/API_definitions/device-status.yaml#L513
should we align this?
There was a problem hiding this comment.
@fernandopradocabrillo Interesting point to discuss.
In device status API we have SUBSCRIPTION_ENDS event but you not have to manage a subscription for this. We consider this is 'automatically' triggered for all notification. When the notification subscription ends, the event is sent.
Here by adding it in the subscription, it means that it required a voluntary action by the developer.
I tend to prefer the approach in device status API but we absolutely need a consensus on this to have same approach in all API.
There was a problem hiding this comment.
Really, I think it makes no sense to allow a subscription to SUBSCRIPTION_ENDS event, as this event is triggered when a subscription to another event ends
There was a problem hiding this comment.
I also think that we don't need that event in the create-subscription, but in the event-notification
There was a problem hiding this comment.
I updated the yaml file , please also see the commit's comment.
Create EventTypeNotification which inherit from EventType, now in create subscription we do not have SUBSCRIPTION_ENDS and only in notification we use that.
| EventTypeNotification: | ||
| description: | | ||
| Event type used in event notification | ||
| allOf: |
There was a problem hiding this comment.
This is not correct. allOf is not extending the enum, it just requires that the object validated against the schema is compliant with all of the sub-schemas, and it cannot comply with both enum at the same time. You have to define an enum with the 3 allowed values for the NotifiedEventType.
There was a problem hiding this comment.
Yaml file is updated.
Created NotifiedEventType enum with 3 values
| default: http://localhost:9091 | ||
| description: API root | ||
| basePath: | ||
| default: location-subscription/v0 |
There was a problem hiding this comment.
it seems that geofencing is the name with more consensus, so you can apply it
change the api name to geofencing
rename yaml file to geofencing
changed the title and description of API based on new name
Hi, I renamed the api two weeks ago before I went to vacation. So I did not recieve any new comment about review, If you think everything is fine you can merge this PR, but first it need two approvers. Anyway I am available if you have any comment. |
code/API_definitions/geofencing.yaml
Outdated
| openapi: 3.0.3 | ||
| info: | ||
| title: Device geofencing API | ||
| description: Service Enabling Network Function API to create, retrieve and delete event subscriptions to realize geofencing for a user device |
There was a problem hiding this comment.
| description: Service Enabling Network Function API to create, retrieve and delete event subscriptions to realize geofencing for a user device | |
| description: API to create, retrieve and delete event subscriptions to realize geofencing for a user device |
This has been deprecated to avoid using 3GPP terminology. Anyway, API doc has to be improved after merge
code/API_definitions/geofencing.yaml
Outdated
| url: https://github.com/camaraproject/ | ||
| security: | ||
| - oAuth2ClientCredentials: [] | ||
| - three_legged: [] |
There was a problem hiding this comment.
| - three_legged: [] |
We can remove this at global level add security per operation with dedicated scopes
code/API_definitions/geofencing.yaml
Outdated
| description: API root | ||
| basePath: | ||
| default: geofencing/v0 | ||
| description: Base path for the device location subscription API |
There was a problem hiding this comment.
| description: Base path for the device location subscription API | |
| description: Base path for the device geofencing API |
| '500': | ||
| $ref: '#/components/responses/Generic500' | ||
| '503': | ||
| $ref: '#/components/responses/Generic503' |
There was a problem hiding this comment.
| $ref: '#/components/responses/Generic503' | |
| $ref: '#/components/responses/Generic503' | |
| security: | |
| - oAuth2AuthorizationCode: | |
| - "geofencing:subscriptions:write" |
| '500': | ||
| $ref: '#/components/responses/Generic500' | ||
| '503': | ||
| $ref: '#/components/responses/Generic503' |
There was a problem hiding this comment.
| $ref: '#/components/responses/Generic503' | |
| $ref: '#/components/responses/Generic503' | |
| security: | |
| - oAuth2AuthorizationCode: | |
| - "geofencing:subscriptions:read" |
code/API_definitions/geofencing.yaml
Outdated
| $ref: '#/components/responses/Generic503' | ||
| get: | ||
| tags: | ||
| - Location Event Subscription |
There was a problem hiding this comment.
| - Location Event Subscription | |
| - Geofencing |
code/API_definitions/geofencing.yaml
Outdated
| /event-subscriptions/{eventSubscriptionsId}: | ||
| get: | ||
| tags: | ||
| - Location Event Subscription |
There was a problem hiding this comment.
| - Location Event Subscription | |
| - Geofencing |
code/API_definitions/geofencing.yaml
Outdated
| $ref: '#/components/responses/Generic503' | ||
| delete: | ||
| tags: | ||
| - Location Event Subscription |
There was a problem hiding this comment.
| - Location Event Subscription | |
| - Geofencing |
code/API_definitions/geofencing.yaml
Outdated
| '500': | ||
| $ref: '#/components/responses/Generic500' | ||
| '503': | ||
| $ref: '#/components/responses/Generic503' |
There was a problem hiding this comment.
| $ref: '#/components/responses/Generic503' | |
| $ref: '#/components/responses/Generic503' | |
| security: | |
| - oAuth2AuthorizationCode: | |
| - "geofencing:subscriptions:read" |
code/API_definitions/geofencing.yaml
Outdated
| post: | ||
| tags: | ||
| - Location Event Subscription | ||
| summary: 'Create a location event subscription for a device' |
There was a problem hiding this comment.
| summary: 'Create a location event subscription for a device' | |
| summary: 'Create a geofencing event subscription for a device' |
Hi Sadeghi. I made another review. I think it is almost ready. Once @bigludo7 is back and make a further check we can merge. Thanks! |
|
I updated the API and applied all last comments that you wrote. If you have no more comment you can merge it and we can close the task. If you have further request you can contact my colleague Maximilian Laue, since I am not working with this project and I switched to the new project. |
Could you please review again. Thanks! 🚀 |
|
Thanks @chia2017 for the effort and good luck for your new project My recommendation is to wait for approval before to adjust your code. |
|
Now that camaraproject/Commonalities#56 has been merged, we should update this PR accordingly @maxl2287, @akoshunyadi. Thanks |
|
This feature is now followed here: #98 |
What type of PR is this?
enhancement/feature
What this PR does / why we need it:
Create a new API for creating subscriptions for location based events
Which issue(s) this PR fixes:
This PR is related to issue #18 Subscription for Location Area
Fixes #
Special notes for reviewers:
Changelog input
Additional documentation
This section can be blank.