-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: allow searching for SBOMs #167
Conversation
f26b338
to
ad0dbde
Compare
A couple of thoughts, not suitable for a specific code review... Per conversation with @jcrossley3 yesterday, I think we decided Also, I'd hoped to move any format-specific document-parsing and weaving into the graph out of I welcome disagreement. |
@@ -19,6 +19,10 @@ pub struct SearchOptions { | |||
pub struct FoundAdvisory { |
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've similar DTOs (AdvisorySummary, AdvisoryDetails) in the graph models for DTOing.
|
||
// `filters` should be of the form, "full text search({field}{op}{value})*", e.g. | ||
// "some text&published>=2020/11/11&location=localhost&severity=low|high&modified=true" | ||
pub async fn search_sboms<'a>( |
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'd hoped this would just be graph.sboms(...same params...)
which could return the entire collection or a filtered subset.
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.
You mean locate_sbom
? I've no idea how I would need to map the query to that.
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.
Not necessarily locate_sbom(...)
but perhaps a new .sboms(...)
method to do the fetch/filter.
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.
Hm, I can't seem to find a method with that name.
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.
Yeah, we probably need to write it.
sort: String, | ||
paginated: Paginated, | ||
) -> Result<PaginatedResults<FoundSbom>, Error> { | ||
let mut select = sbom::Entity::find() |
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've also used this pattern, and while not on your shoulders, I feel like we are repeating ourselves a lot, and perhaps @jcrossley3 could get us some more ergonomics around using search/sort.
I kept it aligned with the current pattern. We can of course refactor, but maybe it makes sense to do this outside of this PR. Either before or after merging this one.
Makes sense, but right now that's tricky, because the tests of |
Also, agree with subsequent PR to shuffle locations of things. |
Superseded by #169 |
No description provided.