Updating names qos-profile of endpoints #348

RandyLevensalor · 2024-08-14T16:06:56Z

What type of PR is this?

Add one of the following kinds:

correction

What this PR does / why we need it:

Updates the endpoint names to include a verb.

Adds security scope to /retrieve-qos-profiles

Updates security scope on /retrieve-qos-profiles/{name}

Which issue(s) this PR fixes:

Fixes #345

Special notes for reviewers:

Should /retrieve-qos-profiles/{name} be singular /retrieve-qos-profile/{name} since it only returns a single qos-profile.

Changelog input

 release-note

Update qos-profile endpoint names to align with commonalites.  Updates the endpoint names to include a verb.

`/qos-profiles` is now `/retrieve-qos-profiles` and `/qos-profiles/{name}` is now `/retrieve-qos-profiles/{name}`

Additional documentation

This section can be blank.

docs

hdamker · 2024-08-14T16:23:17Z

@RandyLevensalor thanks for the PR!

I don't see a need to rename /qos-profiles/{name} as this is a GET and returns actually a qos-profile (and yes, nouns in the path always in plural, even if there is only a singular result). But maybe I'm wrong here ... @jlurien?

eric-murray · 2024-08-15T08:44:12Z

GET /qos-profiles/{name} is correct. qos-profiles is the name of the "resource" where we are looking for one named {name}. This is what developers familiar with "REST" APIs (I use the term loosely) will expect.

POST /retrieve-qos-profiles is a workaround resulting from Tim Berners-Lee's oversight in not making it sufficiently clear that GET requests may have a body, and that any proxy passing on a GET request must look for a body as well in case one is there. So a body could be defined for GET /qos-profiles to contain the filter parameters, but whether that body reaches the API gateway is not at all certain. Hence POST.

I'll be interested to see what developers' reaction to POST /retrieve-qos-profiles will be. If 3-legged tokens become "standard" for calling this API, or nobody want to filter by device, then it may yet be that we re-introduce GET /qos-profiles to keep them happy.

hdamker · 2024-08-15T15:22:01Z

I'll be interested to see what developers' reaction to POST /retrieve-qos-profiles will be. If 3-legged tokens become "standard" for calling this API, or nobody want to filter by device, then it may yet be that we re-introduce GET /qos-profiles to keep them happy.

Interesting enough it was your proposal to start the experiment (#318 (comment)) 😄 ... I agree that it might make sense to reintroduce GET /qos-profiles, making the returned list only dependent on the access token. But first of all we need to see if the API will be implemented at all broadly, or API Provider just offer a short list of "standard" profiles, agreed outside of the API.

RandyLevensalor · 2024-08-15T15:26:00Z

I'll be interested to see what developers' reaction to POST /retrieve-qos-profiles will be. If 3-legged tokens become "standard" for calling this API, or nobody want to filter by device, then it may yet be that we re-introduce GET /qos-profiles to keep them happy.

Interesting enough it was your proposal to start the experiment (#318 (comment)) 😄 ... I agree that it might make sense to reintroduce GET /qos-profiles, making the returned list only dependent on the access token. But first of all we need to see if the API will be implemented at all broadly, or API Provider just offer a short list of "standard" profiles, agreed outside of the API.

Agreed. Though now that we have the post, I'd like to explore some of the other filtering options including using an application profile from connectivity insight as a filter in the next release.

eric-murray · 2024-08-19T16:17:56Z

Agreed. Though now that we have the post, I'd like to explore some of the other filtering options including using an application profile from connectivity insight as a filter in the next release.

Yes, that is a good point. It is much simpler to pass complex objects in a request body than using query parameters. A good enough reason in itself to have the POST option.

RandyLevensalor · 2024-08-20T14:38:03Z

@camaraproject/quality-on-demand_codeowners I don't see any objections. Do you want to approve and merge?

code/API_definitions/qos-profiles.yaml

hdamker

LGTM ... just a kind of question about the scope name for the operation, see my comment above.

Co-authored-by: Herbert Damker <herbert.damker@telekom.de>

RandyLevensalor · 2024-08-20T18:58:16Z

LGTM ... just a kind of question about the scope name for the operation, see my comment above.

Accepted the scope change. This resets the approval. Thanks!

eric-murray · 2024-08-21T07:54:38Z

code/API_definitions/qos-profiles.yaml

@@ -124,20 +128,20 @@ paths:
        "503":
          $ref: "#/components/responses/Generic503"

-  /qos-profiles/{name}:
+  /retrieve-qos-profile/{name}:


I thought we were going to keep GET /qos-profiles/{name}. That would be the expected name for such an endpoint.

eric-murray · 2024-08-21T07:56:24Z

code/API_definitions/qos-profiles.yaml

      description: |
        Returns a QoS Profile that matches the given name.

        The access token may be either a 2-legged or 3-legged access token. If the access token is 3-legged, a QoS Profile is only returned if available to all end users associated with the access token.

      security:
        - openId:
-            - qos-profiles:qos-profiles:read
+            - qos-profiles:retrieve-qos-profiles


I don't see the need to have different scopes for the two endpoints. I'd suggest this endpoint also has scope qos-profiles:read.

jlurien · 2024-08-21T12:38:28Z

code/API_definitions/qos-profiles.yaml

    get:
      tags:
        - QoS Profiles
      summary: "Get QoS Profile for a given name"
-      operationId: getQosProfile
+      operationId: retrieveQosProfile


The original one was OK in line with the comment above

hdamker · 2024-08-23T06:54:05Z

I'm feeling guilty to have created some confusion here as I have overseen that @RandyLevensalor had renamed both endpoints, the POST /qos-profiles and the GET /qos-profiles/{name}, and then I had accidentally commented on the second one.

I agree here now with @eric-murray and @jlurien: the GET /qos-profiles/{name} should not be changed by this PR, as it is correct. Only the POST operation which is not creating a resource has to be renamed with a verb.

hdamker · 2024-08-23T12:17:49Z

Ready for Codeowner approvals.

Added PRs #348, #349, #354

Updating names of endpoints

12ac2c5

RandyLevensalor requested review from hdamker, eric-murray and jlurien as code owners August 14, 2024 16:06

RandyLevensalor changed the title ~~Updating names of endpoints~~ Updating names qos-profile of endpoints Aug 14, 2024

hdamker reviewed Aug 20, 2024

View reviewed changes

code/API_definitions/qos-profiles.yaml Show resolved Hide resolved

hdamker previously approved these changes Aug 20, 2024

View reviewed changes

Update code/API_definitions/qos-profiles.yaml

442e0c3

Co-authored-by: Herbert Damker <herbert.damker@telekom.de>

RandyLevensalor dismissed hdamker’s stale review via 442e0c3 August 20, 2024 18:57

hdamker previously approved these changes Aug 20, 2024

View reviewed changes

eric-murray reviewed Aug 21, 2024

View reviewed changes

jlurien reviewed Aug 21, 2024

View reviewed changes

Incorporating feedback

8cac219

RandyLevensalor dismissed hdamker’s stale review via 8cac219 August 23, 2024 03:25

hdamker mentioned this pull request Aug 23, 2024

Scope of QualityOnDemand for Fall24 CAMARA release #292

Closed

Removing verb from get call

0f43a3b

eric-murray approved these changes Aug 23, 2024

View reviewed changes

hdamker approved these changes Aug 23, 2024

View reviewed changes

jlurien approved these changes Aug 26, 2024

View reviewed changes

hdamker merged commit 686c478 into camaraproject:main Aug 27, 2024
1 check passed

hdamker added a commit that referenced this pull request Aug 29, 2024

Update CHANGELOG.md

4a70dc9

Added PRs #348, #349, #354

gauravn00b mentioned this pull request Sep 30, 2024

QoS Profile Get ALL #366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating names qos-profile of endpoints #348

Updating names qos-profile of endpoints #348

RandyLevensalor commented Aug 14, 2024

hdamker commented Aug 14, 2024

eric-murray commented Aug 15, 2024

hdamker commented Aug 15, 2024 •

edited

Loading

RandyLevensalor commented Aug 15, 2024

eric-murray commented Aug 19, 2024

RandyLevensalor commented Aug 20, 2024

hdamker left a comment

RandyLevensalor commented Aug 20, 2024

eric-murray Aug 21, 2024

eric-murray Aug 21, 2024

jlurien Aug 21, 2024

hdamker commented Aug 23, 2024

hdamker commented Aug 23, 2024

Updating names qos-profile of endpoints #348

Updating names qos-profile of endpoints #348

Conversation

RandyLevensalor commented Aug 14, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for reviewers:

Changelog input

Additional documentation

hdamker commented Aug 14, 2024

eric-murray commented Aug 15, 2024

hdamker commented Aug 15, 2024 • edited Loading

RandyLevensalor commented Aug 15, 2024

eric-murray commented Aug 19, 2024

RandyLevensalor commented Aug 20, 2024

hdamker left a comment

Choose a reason for hiding this comment

RandyLevensalor commented Aug 20, 2024

eric-murray Aug 21, 2024

Choose a reason for hiding this comment

eric-murray Aug 21, 2024

Choose a reason for hiding this comment

jlurien Aug 21, 2024

Choose a reason for hiding this comment

hdamker commented Aug 23, 2024

hdamker commented Aug 23, 2024

hdamker commented Aug 15, 2024 •

edited

Loading