-
Notifications
You must be signed in to change notification settings - Fork 62
Adding new query atttributes for retrieve QoS profile #434
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
base: main
Are you sure you want to change the base?
Adding new query atttributes for retrieve QoS profile #434
Conversation
|
One one side I appreciate that you are able to filter on any of the attributes in the profile. On the other side from the working group meeting today I got the impression that very few of these attributes are set on QoS Profiles today. If that is the case adding a filter will only result in getting nothing back in the response. Do we have any indication of information always set in the profile and if so will it make sense to start with enabling filtering only on those? Also what makes sense from an API invoker perspective. What would be most important to filter on? |
@JoachimDahlgren thanks for the feedback. I don't think that the API should limit what can be filtered. These decisions should be left to the API provider. I think we provide a mechanism and documentations to clearly state how to handle when attributes are not set. One option would be to return an error 422 query attribute not supported. We could also add a new get call to get a list of supported query attributes to better automate the process. Operators could also provide a default value for attributes that are not customizable through the API. |
As I mentioned in the meeting last Friday, my preference is to allow the API consumer to include a flag in their request indicating whether or not they want profiles included in the response that do not have a value set for an attribute they are filtering on. |
@eric-murray I like this proposal. Should this flag be required or optional and if optional, is there a suggestion for the default value? I tend to lean towards a simpler API call, which would have this flag as optional and to default to only returning matching profiles. I'm open to feedback on this point. |
I agree - having a default value implicitly makes it an optional parameter (for the API consumer), and I agree the default should be to narrow the range of responses to profiles for which values are specified for the filter parameters |
|
An additional comment. For documentation, I'd prefer if the applicability of a given filter parameter (i.e. is it and exact match, or greater or equal to, or less than or equal to) be specified in a description property for each individual parameter, rather than being in the overall description for the request body. This makes it easier for the API consumer to understand how the filter will be applied when they are looking at the parameter itself. This can be done using the |
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 have to admit that I'm not in favor to add this functionality at all at the current time -- it will make the implementation of the API more complex, hence slowing the adoption across operators, and has from my perspective currently limited value for API consumer.
We have no experience yet how profiles will actually look like and how many will be there. We have omitted pagination, as the general assumption is that it will be a small number.
If we add the functionality there is at least one change needed.
| required: | ||
| - name | ||
| - 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.
Why are name and status required in this request parameter object? I don't think that this will return the expected results ...
| QosProfileDeviceRequest: | ||
| description: | | ||
| Request object for QoS Profiles for a given device | ||
| Request object for QoS Profiles for a given 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.
Looking on the description of the operation this sentence is not correct ... the operation allows also to use the request object without a given device.
BTW: the operation description might need to be updated as well a bit.
|
As decided within Sub Project call on June 13th: Changing the PR to draft and backlog |
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Adds new query capabilites
Which issue(s) this PR fixes:
Fixes #147
Special notes for reviewers:
Changelog input
Additional documentation
This section can be blank.