-
Notifications
You must be signed in to change notification settings - Fork 3
Add Filter Function #38
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
Add Filter Function #38
Conversation
Added filter parameter in the request parameters to filter the roaming status and type of the device
philipxujin
left a comment
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.
LGTM
| - 'human device' | ||
| - 'IoT device' | ||
| - 'other' | ||
| minItems: 1 |
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.
Suggest to add:
allOf:
- anyOf:
- required: ['roamingStatus']
- required: ['deviceType']
on the Filter level to indicate that if Filter is used at least one of the criteria must be provided. I.e. a filter without any criteria is not allowed.
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.
@gregory1g I understand your point that at least one criterion needs to be provided in the filter parameter. I have modified the description of the filter in commit to see if it meets your requirements.
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.
Highlight this in the description is a good idea. Any reasons why not enforce this on the technical level?
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.
Using anyOf logic has added verification from a technical level. I'm not sure if my implementation is correct, please check.
update filter desciption
Add anyOf logic
hakkiToran
left a comment
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.
The changes are OK .
|
@gregory1g @philipxujin Please review this PR and approve it, then I can proceed with the merge |
philipxujin
left a comment
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.
LGTM
| - 'IoT device' | ||
| - 'other' | ||
| minItems: 1 | ||
| anyOf: |
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.
After the "anyof", objects with all possible combinations of filter criteria are listed. For 2 filters this gives 3 different objects, for 4 this will be much more. With copy-pasted definition for every criteria.
This is not a good way to describe that at least one filter criteria must be provided
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.
Yes,you are correct.Do you have any recommended way to implement this function?
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.
as far as I understand, the following syntax should work:
Filter:
description: |
This parameter is used to filter devices. Currently, two filtering criteria are defined, `roamingStatus` and `deviceType`, which can be expanded in the future. `IN` logic is used used for multiple filtering items within a single filtering criterion, `AND` logic is used between multiple filtering criteria.
- If a filtering critera is not provided, it means that there is no need to filter this item.
- At least one of the criteria must be provided,a filter without any criteria is not allowed.
- If no filtering is required, this parameter does not need to be provided.
For example ,`"filter":{"roamingStatus": ["roaming"],"deviceType": ["human device","IoT device"]}` means the API need to return the count of human network devices and IoT devices that are in roaming mode.`"filter":{"roamingStatus": ["non-roaming"]}` means that the API need to return the count of all devices that are not in roaming mode.
type: object
properties:
roamingStatus:
description: Filter whether the device is in roaming mode,'roaming' represents the need to filter devices that are in roaming mode,'non-roaming' represents the need to filter devices that are not roaming.
type: array
items:
type: string
enum:
- 'roaming'
- 'non-roaming'
minItems: 1
deviceType:
description: Filtering by device type, 'human device' represents the need to filter for human network devices, 'IoT device' represents the need to filter for IoT devices, and 'other' represents the need to filter for other types of devices.
type: array
items:
type: string
enum:
- 'human device'
- 'IoT device'
- 'other'
minItems: 1
oneOf:
- required: ['roamingStatus']
- required: ['deviceType']
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.
@gregory1g Thanks a lot.I have made the yaml changes based on your suggestions, please check.
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 reviewed the code. I think, it is OK.
philipxujin
left a comment
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.
LGTM
Added filter parameter in the request parameters to filter the roaming status and type of the device
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Add filtering functionality to the API to meet users' needs for counting the number of different types of devices
Which issue(s) this PR fixes:
Fixes #36