Create location-subscriptions.yaml #1
Conversation
| url: https://github.com/camaraproject/ | ||
| security: | ||
| - oAuth2ClientCredentials: [] | ||
| - three_legged: |
There was a problem hiding this comment.
this three_legged auth is not used anywhere
There was a problem hiding this comment.
If we do not need it , we can remove this line
| default: http://localhost:9091 | ||
| description: API root | ||
| basePath: | ||
| default: locationSubscription/v0 |
There was a problem hiding this comment.
uri should be defined in lower case with hyphen
There was a problem hiding this comment.
I did not find it in the guidline, so I change it to location-subscription/v0
| openapi: 3.0.3 | ||
| info: | ||
| title: Device location subscription event API | ||
| description: Service Enabling Network Function API which create, retrieve and delete subscription event for device location |
There was a problem hiding this comment.
the API doesn't create/delete/etc. events, but subscriptions for events
maybe:
... subscriptions for events related to the location of a device
This property should contain a detailed documentation also, see qod api. But we could add it later during the review process in Camara too.
There was a problem hiding this comment.
like this: Service Enabling Network Function API which create, retrieve and delete subscriptions for events related to the location of a device?
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/EventSubscriptionInfo' |
| parameters: | ||
| - name: eventSubscriptionsId | ||
| in: path | ||
| description: delete a given event subscription by ID |
| '503': | ||
| $ref: '#/components/responses/Generic503' | ||
|
|
||
| /eventNotifications: |
There was a problem hiding this comment.
uri's are in lowercase with hyphen
actually I would add as callback within POST /event-notifications... I've commented on a similar PR in CAMARA, if we change there we should change it here too.
| authorizationCode: | ||
| authorizationUrl: https://auth.example.com/authorize | ||
| tokenUrl: https://auth.example.com/token | ||
| scopes: |
There was a problem hiding this comment.
I'm not sure we need scopes here, since we have only 1 scope the event handling
There was a problem hiding this comment.
we can have the authorizationCode flow but without scopes, as for the clientCredentials flow
| Event: | ||
| description: The event being notified | ||
| anyOf: | ||
| - $ref: '#/components/schemas/FindLocationDeviceEvent' |
There was a problem hiding this comment.
What does FindLocationDeviceEvent mean? We have 2 events area entered and area left. So we can make 2 events AreaEnteredEvent, AreaLeftEvent or just one like AreaEvent, but I tend to 2 events.
There was a problem hiding this comment.
This is just a sample which return point(longlitude, latitude) of device. But I can instead of it make two another event as we mentioned.
| - point | ||
| properties: | ||
| point: | ||
| $ref: "#/components/schemas/Point" |
There was a problem hiding this comment.
What does that point in the area-entered/left-event mean?
| eventType: | ||
| $ref: "#/components/schemas/EventType" | ||
| required: | ||
| - eventType |
There was a problem hiding this comment.
Don't we need some area information in the request? Otherwise I don't understand how it should work.
There was a problem hiding this comment.
The previous comment is still valid. And also which device is considered?
resolved review comments
removed extra boolean variables
| longitude: 7.10066 | ||
| accuracy: 50 | ||
|
|
||
| Point: |
There was a problem hiding this comment.
Do you Point is not used?! if you check line 211, point is used as a property of Circle object
| scopes: {} | ||
|
|
||
| schemas: | ||
| Area: |
There was a problem hiding this comment.
Regarding Area in line 191, it is used as property of AreaLeft object in line 415 and as a property of AreaEntered object in line 424 is used.
| eventType: | ||
| $ref: "#/components/schemas/EventType" | ||
| required: | ||
| - eventType |
There was a problem hiding this comment.
The previous comment is still valid. And also which device is considered?
Add device and area
| content: | ||
| application/json: | ||
| schema: | ||
| type: array |
There was a problem hiding this comment.
Better create a SubscriptionInfoList type
There was a problem hiding this comment.
I changed EventSubscriptionInfo to SubscriptionInfo, actually here SubscriptionInfoList does not make sence, only SubscriptionInfo is enough
Changed yaml file based on last review comments
Resolved review comments
chia2017
merged commit f81540b
into
define-new-location-subscriptions-api-with-event-subscription-endpoint
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for reviewers:
Changelog input
Additional documentation
This section can be blank.