juhaku / utoipa

Simple, Fast, Code first and Compile time generated OpenAPI documentation for Rust
Apache License 2.0
2.49k stars 197 forks source link

The `ignore` attribute now accepts an optional bool value #1177

Closed JMLX42 closed 3 weeks ago

JMLX42 commented 3 weeks ago

Allow to statically ignore field using const generics:

#[derive(Debug, Serialize, ToSchema)]
pub struct SuccessResponsePayload<T: ResourceObject, const IGNORE_INCLUDED: bool = true> {
    pub data: Vec<T>,
    #[schema(ignore = IGNORE_INCLUDED)]
    pub included: Vec<T::RelationshipValue>,
    #[schema(inline)]
    pub links: LinksObject<T>,
}

The following syntaxes are also supported:

JMLX42 commented 3 weeks ago

I think accepting any expression is misleading and breaks very easily. It would be simpler/more sane to accept either a literal bool or a path to a Fn() -> bool.

If ones wants to use generics const, it's still possible like this:

#[derive(Debug, Serialize, ToSchema)]
pub struct SuccessResponsePayload<T: ResourceObject, const IGNORE_INCLUDED: bool = true> {
    pub data: Vec<T>,
    #[schema(ignore = Self::ignore_included())]
    pub included: Vec<T::RelationshipValue>,
    #[schema(inline)]
    pub links: LinksObject<T>,
}

impl<T: ResourceObject, const IGNORE_INCLUDED: bool> SuccessResponsePaylod<T, IGNORE_INCLUDED> {
  fn ignore_included() -> bool { IGNORE_INCLUDED }
}

Any input on this @juhaku ?

juhaku commented 3 weeks ago

Yup, I am fine even without the support of expression and const generic support or fn expression. But if there is a need for such functionality I am not against it. Though the simpler the better.

JMLX42 commented 3 weeks ago

Yup, I am fine even without the support of expression and const generic support or fn expression. But if there is a need for such functionality I am not against it. Though the simpler the better.

@juhaku I'm handling it because I need it. I need some fields to be statically ignored based on some associated const value/associated function.

juhaku commented 3 weeks ago

@JMLX42 That's a valid point, If you think the fn() -> bool expression is enough and easily doable, go for it :slightly_smiling_face: Though, as with what comes with the generic expression support, I believe as the saying goes "great power comes great responsibility". The responsibility lies on the user to use that feature responsibly. Like the LitStrOrExpr also does not really care what user has input it just expects user has input valid content.

JMLX42 commented 3 weeks ago

If you think the fn() -> bool expression is enough and easily doable, go for it

@juhaku I have updated the PR to implement LitBoolOrExprCall instead of LitBoolOrExpr.

I'll see how it reports errors on my code base and report back.

JMLX42 commented 3 weeks ago

image

error[E0308]: mismatched types
   --> crates/jsonapi_types/src/response.rs:651:28
    |
651 | #[derive(Debug, Serialize, ToSchema)]
    |                            ^^^^^^^^ expected `bool`, found `i32`

The error does mention the type error. But it highlights ToSchema when I'd rather have it highlight Self::ignore_included2(). Not sure how to do that for now: I'm still very noob on macro error reporting. I'll dig in and see what I can do about that.

JMLX42 commented 3 weeks ago

The error does mention the type error. But it highlights ToSchema when I'd rather have it highlight Self::ignore_included2(). Not sure how to do that for now: I'm still very noob on macro error reporting. I'll dig in and see what I can do about that.

Done in 21cbcc7a7122901e2268caf45011070a5bef66bc:

    let value = api_doc! {
        struct SchemaIgnoredField {
            value: String,
            #[schema(ignore = 42)]
            this_is_not_private: String,
        }
    };
error: expected literal bool or function call that returns bool, found: 42
    --> utoipa-gen/tests/schema_derive_test.rs:5722:31
     |
5722 |             #[schema(ignore = 42)]
     |                               ^^

The trick was to use quote_spanned!.

juhaku commented 3 weeks ago

Self::ignore_included2()

Just a thought, could this be changed to syntax, similar to schema_with = ... attribute has? The schema_with attribute supports function syntax but allows the function be defined without parenthesis ().

Not sure how to do that for now: I'm still very noob on macro error reporting. I'll dig in and see what I can do about that.

You need a correct span for that. The error need to be spanned with the span of the parsed attribute. For example here in validation.rs features we parse the feature with the ident to get the span. And further above we are returning error with that span when validating. https://github.com/juhaku/utoipa/blob/ca643efa43942f2ceee01bafe74d412153b40cc6/utoipa-gen/src/component/features/validation.rs#L135-L142

You could use dbg!() to see what kind of ident is the ident provided to the parse function of Ignore feature. Or you could get the ident of the expression parsed within the Ignore. The fundamental part is to get the span of the attribute and use that in the error where ever the condition for "input not being valid" materializes.

Hope this helps

juhaku commented 3 weeks ago

Great, just as I wrote some help, ignore me then :rofl:

JMLX42 commented 3 weeks ago

@juhaku IMHO I'm good to go. I've updated the doc and the CHANGELOG too. Let me know if there is anything else required.

juhaku commented 3 weeks ago

@JMLX42 It looks good to me. Just wondering whether the syntax of the function parsing can be altered to be without the ending parenthesis ()? This way it would be similar to how serde defines supports functions and how the schema_with is

JMLX42 commented 3 weeks ago

Just wondering whether the syntax of the function parsing can be altered to be without the ending parenthesis ()? This way it would be similar to how serde defines supports functions and how the schema_with is

@juhaku done!

JMLX42 commented 3 weeks ago

Just wondering whether the syntax of the function parsing can be altered to be without the ending parenthesis ()? This way it would be similar to how serde defines supports functions and how the schema_with is

@juhaku actually no, it does not work as expected:

#[derive(Debug, Serialize, ToSchema)]
pub struct SuccessResponsePayload<T: ResourceObject> {
    pub data: Vec<T>,
    #[schema(ignore = Self::ignore_included())]
    #[serde(skip_serializing_if = "Option::is_none")]
    pub included: Option<Vec<T::RelationshipValue>>,
    #[schema(inline)]
    pub links: LinksObject<T>,
}

Error message:

expected literal bool or path to a function that returns bool, found: Self :: ignore_included()
juhaku commented 3 weeks ago

Ok, I guess it can be then as is.

JMLX42 commented 3 weeks ago

Ok, I guess it can be then as is.

@juhaku nope. I'll make it work :+1:

JMLX42 commented 3 weeks ago

it does not work as expected:

@juhaku actually it does work exactly as expected: I wrote #[schema(ignore = Self::ignore_included())] instead of #[schema(ignore = Self::ignore_included)] (note the unwanted parentheses that I forgot to remove).

So I guess we're all good :heavy_check_mark: .

juhaku commented 3 weeks ago

Nice, well great. We are good to go, I merge this then, thanks for the patch :slightly_smiling_face: