-
Notifications
You must be signed in to change notification settings - Fork 29
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
Subscription-Issue4: Need to change the scope pattern for explicit subscriptions #163
Comments
Makes sense to me - regarding proposed syntax, But
or there should be a definition for the supported short names in the |
Hello As the guidelines states:
What about something like this:
device-status:subscriptions:create is a wild card for all event type. For the GET & DELETE we can keep it simple:
and
|
@Kevsy @bigludo7 Thanks for your comments. @Kevsy : Yes, I was trying to indicate with the example that may be for explicit subscriptions, some shorter naming convention could be defined in the design guidelines and then used accordingly (and was hoping that api name is not part of it :) ). @bigludo7: I agree with the idea of keeping the get and delete simple as suggested above. Regarding create: BTW, I heard that device status subproject is currently discussing the idea about splitting the API going ahead.
Is this understanding correct? BTW is it decided if device status will align more closely to device location and move subscription to a separate API or not? |
Hello @shilpa-padgaonkar For splitting the device status API this is an in progress discussion and not decision yet. In this case, yes the event should change to I realized that in Geofencing API we have |
@bigludo7 : Yes I am aware that there is no decision yet :) hence I mentioned
Thanks for creating the issue, and my guess is that at least the subprojects which I had listed here in the comment, follow the same format for scope #153 (comment) Not sure if they have seen this discussion here though... |
@jlurien @PedroDiez : Any feedback on this topic? |
Hi, @shilpa-padgaonkar, @bigludo7 we are checking this internally. Expect to comment on this tomorrow |
Hello, thanks to raise this topic @shilpa-padgaonkar. This topic fully makes sense and we think has benefits to tackle it. From our point of view, we agree about the main ideas exposed by all in this thread. Our considerations would be:
Additionally, another point we would like to share with you all, as we consider it is relevant in this issue is how to deal with the case of creating a subscription when the scope "rights" do not match with the susbcription detail. E.g. in device status an API client has scope to create subscriptions for roaming status so scope: device-status:roaming-status:subscriptions:create An it tries to create a subscription with subscription detail body according to type: In this scenario, we propose to raise an HTTP 403 exception with code:
|
@PedroDiez Thanks & good to see that we're aligned. |
@PedroDiez and @bigludo7 : Good to know that we are aligned on the fact that event types are relevant in defining scopes. @PedroDiez : SUSBCRIPTION_MISMATCH sounds good. |
@shilpa-padgaonkar @PedroDiez Do you want that we wait for a global alignement on all subscriptions related topic to update the guidelines or can we already propose PR? For this one, probably we can propose an updated here https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#1161-scope-naming but looking on your view before to craft a PR. |
Yes, better SUSBCRIPTION_MISMATCH in the context of this topic |
To me is fine to move forward on this @bigludo7 |
@bigludo7 : I am ok to move forward with a PR for the scope topic, as most of us are aligned here. |
BTW, I looked at recent comments in device status separate endpoint issue and looked at the possible API names suggested then as per the discussed pattern above
This means that we repeat the API name twice,
Do we need to repeat it twice or could we get rid of api name from the event type field? |
Fair point @shilpa-padgaonkar ! More globally should we make following proposal:
I tend to prefer keeping the API-name in the event type as this one will circulate 'alone' in event. |
Good point @shilpa-padgaonkar When proposing this approach:
Indeed i was meant (i was thinking about): Because in the context of this functionality:
In other words within a given API, the event-name must be unique so as it is the relevant key to identify the event |
@bigludo7 Would be ok to get the api name out of the scope as you have suggested above. |
@bigludo7 May I am overthinking ;) Let's start with your proposal |
@shilpa-padgaonkar, @bigludo7 |
Agree on the interpretation @PedroDiez but don't you think that we can raffine this ?
Result: If we want to avoid duplicate of the Removing the alternate will be to keep the duplicate but this not very elegant. WDYT? |
@PedroDiez @bigludo7 I have created a PR to just see how the changes would look like. If we think it's too premature, I don't mind changing the PR to draft PR. Just thought, it could get us started somehow :) |
Let's talk tomorrow. The model proposed is therefore this?
|
@PedroDiez
@bigludo7 supports option2. I lean towards option1, but if the group decides that option2 is the way to go, I won't object. |
Problem description
The API design guidelines state that scopes should be represented as:
For explicit subscriptions, giving an example of device-status, this pattern results in
device-status:subscriptions:create
And with this subscription, any one of the below event types could be subscribed to:
The scope device-status:subscriptions:create doesn't give any insights on the event types.
Expected behavior
For e.g.
device:roaming-status-sub:create
Alternative solution
Additional context
The text was updated successfully, but these errors were encountered: