Skip to content
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

allow list of states in flaw filter #469

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Conversation

costaconrado
Copy link
Contributor

@costaconrado costaconrado commented Mar 12, 2024

This PR changes flaw API to allow filtering by a list of workflow_state.

Closes OSIDB-2208.

@costaconrado costaconrado requested a review from a team March 12, 2024 12:20
@costaconrado costaconrado force-pushed the multiple-state-filter branch 2 times, most recently from 1b45735 to f22cdbc Compare March 12, 2024 12:42
Copy link
Contributor

@jsvob jsvob left a comment

Choose a reason for hiding this comment

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

LGTM, I'm for merging this and discussing the nitpick asynchronously.

type: array
items:
type: string
description: Multiple values may be separated by commas.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: The enum in openapi.yml seems kind of useful for knowing which values are valid. We are losing that usefulness here. I see that Flaw's cve_id has the same drawback in openapi.yml, so this PR already has a precedent that it is OK. I'd just like to discuss (even after merging this) whether we want to somehow improve openapi.yml for fields that use CharInFilter, and whether anyone would even care. @RedHatProductSecurity/osidb-devs thoughts?

Copy link
Contributor

@JakubFrejlach JakubFrejlach Mar 13, 2024

Choose a reason for hiding this comment

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

For cve_id I think it is kinda different as it is not an enum and the field is pretty self explanatory, but losing an enum values might be problematic for anyone who would like to work with the API. I posted some possible solutions below, not tested them thoroughly or sure about them, just something to think about.

Copy link
Contributor

@JakubFrejlach JakubFrejlach left a comment

Choose a reason for hiding this comment

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

Sorry for sticking my nose into this but I've noticed during the standup that this is about filters so I might give a quick opinion on this. 😄

Comment on lines -3445 to -3452
type: string
enum:
- DONE
- NEW
- PRE_SECONDARY_ASSESSMENT
- REJECTED
- SECONDARY_ASSESSMENT
- TRIAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not entirely correct as we lost information about possible values of the enum, this could be easily solved if you use a different filter for the workflow_state:

from django_filters.rest_framework import ChoiceFilter, BaseInFilter

class ChoiceInFilter(BaseInFilter, ChoiceFilter):
    """
    Filter for choice csv
    """

    pass
    
workflow_state = ChoiceInFilter(field_name="workflow_state")

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this might not work correctly, there also exist MultipleChoiceFilter, not sure how that works, just pointing out that enum values should be exposed in the schema

Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

I am also in favor of finding a way how to preserve the enumeration values listed in the OpeAPI

Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

See my comment above. I forgot to request the change.

@costaconrado
Copy link
Contributor Author

Makes sense, submitted the change suggested by @JakubFrejlach, thanks you all

@JakubFrejlach
Copy link
Contributor

JakubFrejlach commented Mar 13, 2024

@costaconrado Actually I am not sure, but it might be necessary to explicitly list the choices via choices argument, I tried that quickly on my local instance and without the choices I was getting "Invalid option selected...." for valid options. https://django-filter.readthedocs.io/en/stable/ref/filters.html#choicefilter

workflow_state_choices = [
        (s.value, s.value) for s in Flaw.WorkflowState
    ]

something like this probably

@costaconrado
Copy link
Contributor Author

@JakubFrejlach I believe I can use something like:
workflow_state = ChoiceInFilter(field_name="workflow_state", choices=WorkflowModel.WorkflowState.choices)

let me test it

Copy link
Contributor

@osoukup osoukup left a comment

Choose a reason for hiding this comment

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

Much better 😺 thank you for addressing the feedback!

Signed-off-by: Conrado Costa <concosta@redhat.com>
@costaconrado costaconrado added this pull request to the merge queue Mar 13, 2024
Merged via the queue into master with commit c4ffdd1 Mar 13, 2024
10 of 11 checks passed
@costaconrado costaconrado deleted the multiple-state-filter branch March 13, 2024 15:37
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.

4 participants