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

fix: deserialize relationships filter from query params #1256

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

jcrossley3
Copy link
Contributor

Fixes #1232

Copy link
Contributor

@chirino chirino left a comment

Choose a reason for hiding this comment

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

lgtm.

chirino
chirino previously requested changes Feb 5, 2025
Copy link
Contributor

@chirino chirino left a comment

Choose a reason for hiding this comment

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

So the openapi client will genereate requests that look like:

http://localhost:3000/api/v2/analysis/component/test?ancestors=10&relationships[]=contains&relationships[]=dependency

Any chance we could parse that?

@chirino
Copy link
Contributor

chirino commented Feb 5, 2025

Ah shit maybe we just need to fix the test to do it the openapi way!..

@chirino
Copy link
Contributor

chirino commented Feb 5, 2025

@jcrossley3 I think you can close, #1257 is the simple fix.

Ideally, there'd be a way to ref the values in the Relationship enum,
but I didn't see an obvious way to do that.

Signed-off-by: Jim Crossley <[email protected]>
@jcrossley3
Copy link
Contributor Author

@jcrossley3 I think you can close, #1257 is the simple fix.

I agree that fix is simple, but it changes the test, and it forces me to remember to add [] after the field name. Does anyone really want that?

@carlosthe19916
Copy link
Member

My understanding from the internal chat is that we need to pass an array of values through the query params.

From my experience:

  • If the queries get to a very complex level we could consider migrating GET to POST. E.g. GET /myendpoint?complex_params to POST /myendpoint with a body . Main point, set multiple combination of queries in the body of the POST request
  • We can use what we have already in all the search endpoints (my example is without encoding).
/api/v2/vulnerability?limit=10&offset=0&q=average_severity=low|medium|high|critical

Notice that there is a variable average_severity that is receiving an array of possible values.

@chirino
Copy link
Contributor

chirino commented Feb 6, 2025

@carlosthe19916 so the js openapi client can already pass the array with a js call like:

getComponent({
    path: { key: "test" },
    query: {
      ancestors: 10,
      relationships: ["contains", "dependency"],
    },
  });

where the relationships field is defined as: relationships?: Array<Relationship>; so you get a nice typesafe interface.

The above would generate this URL: http://localhost:3000/api/v2/analysis/component/test?ancestors=10&relationships[]=contains&relationships[]=dependency

But sadly this PR breaks the above but in exchange, users gain the ability to manually construct URLs easier.
This PR would require the URL to be : http://localhost:3000/api/v2/analysis/component/test?ancestors=10&relationships=contains,dependency

#1257 keeps the openapi interface working and also supports constructing the URL easy by adding a new field.
You could use http://localhost:3000/api/v2/analysis/component/test?ancestors=10&relationships[]=contains&relationships[]=dependency
to filter on multiple or
http://localhost:3000/api/v2/analysis/component/test?ancestors=10&relationship=contains to filter for a single relationship.

Copy link
Collaborator

@JimFuller-RedHat JimFuller-RedHat left a comment

Choose a reason for hiding this comment

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

LGTM! and thx for litigating the whole conversation on this!

@jcrossley3 jcrossley3 dismissed chirino’s stale review February 7, 2025 20:47

Dismissing in light of 1257 being closed -- not sure this PR is our final answer but it's an evolutionary step.

@jcrossley3 jcrossley3 added this pull request to the merge queue Feb 7, 2025
Merged via the queue into trustification:main with commit e448dc6 Feb 7, 2025
2 checks passed
@jcrossley3 jcrossley3 deleted the 1232 branch February 7, 2025 21:00
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.

api/v2/analysis/component/{}?relationships= throws deserialize error
4 participants