Skip to content

Conversation

@chinaunicomyangfan
Copy link
Collaborator

@chinaunicomyangfan chinaunicomyangfan commented Oct 15, 2024

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:

  • enhancement/feature

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

Added filter parameter in the request parameters to filter the roaming status and type of the device
philipxujin
philipxujin previously approved these changes Oct 15, 2024
Copy link
Collaborator

@philipxujin philipxujin left a 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

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.

Copy link
Collaborator Author

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.

Copy link

@gregory1g gregory1g Oct 16, 2024

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?

Copy link
Collaborator Author

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
Copy link

@hakkiToran hakkiToran left a 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 .

@chinaunicomyangfan
Copy link
Collaborator Author

@gregory1g @philipxujin Please review this PR and approve it, then I can proceed with the merge

philipxujin
philipxujin previously approved these changes Oct 28, 2024
Copy link
Collaborator

@philipxujin philipxujin left a 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:

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

Copy link
Collaborator Author

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?

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']

Copy link
Collaborator Author

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.

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.

Copy link
Collaborator

@philipxujin philipxujin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chinaunicomyangfan chinaunicomyangfan merged commit 605e168 into camaraproject:main Nov 4, 2024
@chinaunicomyangfan chinaunicomyangfan deleted the addFilterFunction branch November 4, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add filtering function to Region Device Count API

4 participants