feat: add 'Query' derive to manage custom Utoipa Query descriptions#1890
feat: add 'Query' derive to manage custom Utoipa Query descriptions#1890mrizzi merged 4 commits intoguacsec:mainfrom
Conversation
Reviewer's GuideThis PR introduces a custom derive macro and supporting crate to generate OpenAPI descriptions for query and sort parameters from struct definitions, replaces hardcoded parameter docs in the OpenAPI spec with those dynamic descriptions, refactors the advisory/vulnerability endpoints to use the new TrustifyQuery wrapper and custom query structs, and integrates the new crates into the workspace configuration. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1890 +/- ##
==========================================
+ Coverage 68.06% 68.26% +0.19%
==========================================
Files 365 367 +2
Lines 23063 23216 +153
Branches 23063 23216 +153
==========================================
+ Hits 15698 15848 +150
- Misses 6486 6488 +2
- Partials 879 880 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jcrossley3
left a comment
jcrossley3
left a comment
There was a problem hiding this comment.
If the goal is to add the valid field names to openapi.yaml, can't this entire PR be replaced by the following suggested changes?
I'm having a hard time justifying the complexity of the extra modules/macros.
| #[allow(dead_code)] | ||
| #[derive(QueryDoc)] | ||
| struct AdvisoryQuery { | ||
| average_score: i32, | ||
| average_severity: String, | ||
| modified: Date, | ||
| title: String, | ||
| } | ||
|
|
There was a problem hiding this comment.
| #[allow(dead_code)] | |
| #[derive(QueryDoc)] | |
| struct AdvisoryQuery { | |
| average_score: i32, | |
| average_severity: String, | |
| modified: Date, | |
| title: String, | |
| } | |
| /// List advisories | |
| /// | |
| /// Valid field names to use in sort/filter queries: | |
| /// - average_score | |
| /// - average_severity | |
| /// - modified | |
| /// - title | |
| /// |
There was a problem hiding this comment.
The goal is to have a customized OpenAPI description for each endpoint similar to the detailed (but hardcoded) EBNF grammar definition (without having to copy & paste the same grammar in each endpoint) currently available in https://github.com/trustification/trustify/blob/ecc14e850b4cb470cada306a29fe82ce63e38601/common/src/db/query.rs#L141-L210
Considering only this PR will have all of the derive procedural macro code, the changes required for using it will be just a matter of defining a struct with the list of fields, the same list you have in the proposed comment but with the benefit of being able to further improve/manage it in the future as we need.
| #[allow(dead_code)] | ||
| #[derive(QueryDoc)] | ||
| struct VulnerabilityQuery { | ||
| base_score: i32, | ||
| base_severity: String, | ||
| modified: Date, | ||
| title: String, | ||
| } | ||
|
|
There was a problem hiding this comment.
| #[allow(dead_code)] | |
| #[derive(QueryDoc)] | |
| struct VulnerabilityQuery { | |
| base_score: i32, | |
| base_severity: String, | |
| modified: Date, | |
| title: String, | |
| } | |
| /// List vulnerabilities | |
| /// | |
| /// Valid field names to use in sort/filter queries: | |
| /// - base_score | |
| /// - base_severity | |
| /// - modified | |
| /// - title | |
| /// |
ctron
left a comment
ctron
left a comment
There was a problem hiding this comment.
I like the PR. Learning about this and leveraging it for a seamless integration.
Maybe it makes sense to change the name away from "doc", indicating that this could be more than just doc in the future.
Cool, I've renamed it 👍 |
|
If this is purely to add in common docs, can you use something like on each operation to amend additional docs? ref: https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html |
This PR is more about having a documentation template (i.e. the EBNF grammar) populated with custom fields each endpoint manages providing a solution that allows us to further improve it. |
|
@sourcery-ai summary |
mrizzi
force-pushed
the
feat-macro-generate-openapi
branch
from
f7302a4 to
ed3e90b
Compare
mrizzi
changed the title
Signed-off-by: mrizzi <mrizzi@redhat.com>
Signed-off-by: mrizzi <mrizzi@redhat.com>
mrizzi
force-pushed
the
feat-macro-generate-openapi
branch
from
ed3e90b to
5d6172d
Compare
Signed-off-by: mrizzi <mrizzi@redhat.com>
mrizzi
force-pushed
the
feat-macro-generate-openapi
branch
from
d7d2603 to
78bde90
Compare
|
@sourcery-ai summary |
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mrizzi - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `query/query-derive/src/lib.rs:16` </location>
<code_context>
+
+fn impl_query(ast: &syn::DeriveInput) -> TokenStream {
+ let name = &ast.ident;
+ let fields = match &ast.data {
+ Data::Struct(data_struct) => data_struct
+ .fields
+ .clone()
+ .into_iter()
+ .map(|field| match field.ident {
+ Some(ref ident) => ident.to_string(),
+ None => String::default(),
+ })
+ .collect::<Vec<String>>(),
+ Data::Enum(_) => vec![],
+ Data::Union(_) => vec![],
+ };
</code_context>
<issue_to_address>
The macro currently ignores tuple and unnamed struct fields.
If tuple structs aren't supported, add a compile error or explicit panic to prevent silent failures.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
fn impl_query(ast: &syn::DeriveInput) -> TokenStream {
let name = &ast.ident;
let fields = match &ast.data {
Data::Struct(data_struct) => data_struct
.fields
.clone()
.into_iter()
.map(|field| match field.ident {
Some(ref ident) => ident.to_string(),
None => String::default(),
})
.collect::<Vec<String>>(),
Data::Enum(_) => vec![],
Data::Union(_) => vec![],
};
=======
fn impl_query(ast: &syn::DeriveInput) -> TokenStream {
let name = &ast.ident;
let fields = match &ast.data {
Data::Struct(data_struct) => {
match &data_struct.fields {
syn::Fields::Named(fields_named) => {
fields_named.named.iter()
.map(|field| field.ident.as_ref().unwrap().to_string())
.collect::<Vec<String>>()
}
syn::Fields::Unnamed(_) | syn::Fields::Unit => {
return syn::Error::new_spanned(
&ast.ident,
"Tuple structs and unit structs are not supported by #[derive(QueryDoc)]"
)
.to_compile_error()
.into();
}
}
}
Data::Enum(_) => vec![],
Data::Union(_) => vec![],
};
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `query/query-derive/src/lib.rs:69` </location>
<code_context>
+ );
+
+ let generated = quote! {
+ impl query::Query for #name {
+ fn generate_query_description() -> String {
+ #query_description.to_string()
</code_context>
<issue_to_address>
The macro assumes the `query` crate is always imported as `query`.
If `query` is renamed or missing, this will fail. Use `$crate` or document the required import path.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let generated = quote! {
impl query::Query for #name {
fn generate_query_description() -> String {
#query_description.to_string()
}
fn generate_sort_description() -> String {
#sort_description.to_string()
}
}
};
=======
let generated = quote! {
impl $crate::Query for #name {
fn generate_query_description() -> String {
#query_description.to_string()
}
fn generate_sort_description() -> String {
#sort_description.to_string()
}
}
};
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `modules/fundamental/src/advisory/endpoints/mod.rs:60` </location>
<code_context>
}
+#[allow(dead_code)]
+#[derive(Query)]
+struct AdvisoryQuery {
+ id: Uuid,
+ identifier: String,
+ version: Option<String>,
+ document_id: String,
+ deprecated: bool,
+ issuer_id: Option<Uuid>,
+ published: Option<OffsetDateTime>,
+ modified: Option<OffsetDateTime>,
+ withdrawn: Option<OffsetDateTime>,
+ title: Option<String>,
+ ingested: OffsetDateTime,
+ label: String,
+}
+
</code_context>
<issue_to_address>
The AdvisoryQuery struct uses non-Option types for some fields, which may not match query parameter semantics.
Since these fields are not Option types, they must always be provided, which may not align with how query parameters are typically used. This could lead to deserialization errors if any of these fields are missing in incoming queries.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| fn impl_query(ast: &syn::DeriveInput) -> TokenStream { | ||
| let name = &ast.ident; | ||
| let fields = match &ast.data { | ||
| Data::Struct(data_struct) => data_struct | ||
| .fields | ||
| .clone() | ||
| .into_iter() | ||
| .map(|field| match field.ident { | ||
| Some(ref ident) => ident.to_string(), | ||
| None => String::default(), | ||
| }) | ||
| .collect::<Vec<String>>(), | ||
| Data::Enum(_) => vec![], | ||
| Data::Union(_) => vec![], | ||
| }; |
There was a problem hiding this comment.
suggestion (bug_risk): The macro currently ignores tuple and unnamed struct fields.
If tuple structs aren't supported, add a compile error or explicit panic to prevent silent failures.
| fn impl_query(ast: &syn::DeriveInput) -> TokenStream { | |
| let name = &ast.ident; | |
| let fields = match &ast.data { | |
| Data::Struct(data_struct) => data_struct | |
| .fields | |
| .clone() | |
| .into_iter() | |
| .map(|field| match field.ident { | |
| Some(ref ident) => ident.to_string(), | |
| None => String::default(), | |
| }) | |
| .collect::<Vec<String>>(), | |
| Data::Enum(_) => vec![], | |
| Data::Union(_) => vec![], | |
| }; | |
| fn impl_query(ast: &syn::DeriveInput) -> TokenStream { | |
| let name = &ast.ident; | |
| let fields = match &ast.data { | |
| Data::Struct(data_struct) => { | |
| match &data_struct.fields { | |
| syn::Fields::Named(fields_named) => { | |
| fields_named.named.iter() | |
| .map(|field| field.ident.as_ref().unwrap().to_string()) | |
| .collect::<Vec<String>>() | |
| } | |
| syn::Fields::Unnamed(_) | syn::Fields::Unit => { | |
| return syn::Error::new_spanned( | |
| &ast.ident, | |
| "Tuple structs and unit structs are not supported by #[derive(QueryDoc)]" | |
| ) | |
| .to_compile_error() | |
| .into(); | |
| } | |
| } | |
| } | |
| Data::Enum(_) => vec![], | |
| Data::Union(_) => vec![], | |
| }; |
| #[derive(Query)] | ||
| struct AdvisoryQuery { | ||
| id: Uuid, | ||
| identifier: String, | ||
| version: Option<String>, | ||
| document_id: String, | ||
| deprecated: bool, | ||
| issuer_id: Option<Uuid>, | ||
| published: Option<OffsetDateTime>, | ||
| modified: Option<OffsetDateTime>, |
There was a problem hiding this comment.
issue (bug_risk): The AdvisoryQuery struct uses non-Option types for some fields, which may not match query parameter semantics.
Since these fields are not Option types, they must always be provided, which may not align with how query parameters are typically used. This could lead to deserialization errors if any of these fields are missing in incoming queries.
| }; | ||
| use actix_web::{HttpResponse, Responder, delete, get, post, web}; | ||
| use query::TrustifyQuery; | ||
| use query_derive::Query; |
There was a problem hiding this comment.
you can add a pub use query_derive::Query; to query in order to just import use query::Query;.
ctron
left a comment
ctron
left a comment
There was a problem hiding this comment.
Two small nits. but not blocking. looks great.
Signed-off-by: mrizzi <mrizzi@redhat.com>
4 participants
Implementing the
QueryDoccustomDerivemacro in order to use the referencedstructas the source for the fields allowed to:qquery parametersortquery parameterAdvisoryQueryandVulnerabilityQueryare two examples of how to use such a macro from now on.The actual descriptions format might need further changes based on the LLM/MCP effectiveness.
@ctron in the first review I would like to understand if the overall structure for the macro is fine and eventually apply refactoring to move it somewhere else in the project or rename them. Thank you.
Summary by Sourcery
Introduce a custom derive macro and supporting crates to automate generation of Utoipa query and sort parameter descriptions based on struct fields
New Features:
Enhancements: