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

The ignore attribute now accepts an optional bool value #1177

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

JMLX42
Copy link
Contributor

@JMLX42 JMLX42 commented Nov 1, 2024

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:

  • #[schema(ignore)] (defaults to true)
  • #[schema(ignore = false)]
  • #[schema(ignore = <exp>)] with <exp> any expression that returns bool

@JMLX42 JMLX42 force-pushed the feat/ignore-value branch 4 times, most recently from 34622f7 to 2fe1774 Compare November 1, 2024 14:07
@JMLX42 JMLX42 changed the title The attribute now accepts an optional bool value The ignore attribute now accepts an optional bool value Nov 1, 2024
@JMLX42
Copy link
Contributor Author

JMLX42 commented Nov 1, 2024

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
Copy link
Owner

juhaku commented Nov 1, 2024

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 JMLX42 force-pushed the feat/ignore-value branch from 2fe1774 to 2da570c Compare November 1, 2024 15:23
@JMLX42
Copy link
Contributor Author

JMLX42 commented Nov 1, 2024

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
Copy link
Owner

juhaku commented Nov 1, 2024

@JMLX42 That's a valid point, If you think the fn() -> bool expression is enough and easily doable, go for it 🙂 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 JMLX42 force-pushed the feat/ignore-value branch from 2da570c to f412f8e Compare November 1, 2024 17:05
@JMLX42
Copy link
Contributor Author

JMLX42 commented Nov 1, 2024

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
Copy link
Contributor Author

JMLX42 commented Nov 1, 2024

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 JMLX42 force-pushed the feat/ignore-value branch from f412f8e to 21cbcc7 Compare November 1, 2024 17:35
@JMLX42
Copy link
Contributor Author

JMLX42 commented Nov 1, 2024

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 21cbcc7:

    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
Copy link
Owner

juhaku commented Nov 1, 2024

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.

impl Parse for Maximum {
fn parse(input: ParseStream, ident: Ident) -> syn::Result<Self>
where
Self: Sized,
{
parse_next_number_value(input).map(|number| Self(number, ident))
}
}

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

@JMLX42 JMLX42 force-pushed the feat/ignore-value branch from 21cbcc7 to 783f807 Compare November 1, 2024 17:39
@JMLX42 JMLX42 marked this pull request as ready for review November 1, 2024 17:39
@juhaku
Copy link
Owner

juhaku commented Nov 1, 2024

Great, just as I wrote some help, ignore me then 🤣

@JMLX42 JMLX42 force-pushed the feat/ignore-value branch from 783f807 to 8c04f8c Compare November 1, 2024 17:40
@JMLX42
Copy link
Contributor Author

JMLX42 commented Nov 1, 2024

@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.

Copy link
Owner

@juhaku juhaku left a comment

Choose a reason for hiding this comment

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

Pretty neat, thanks 👍 🥇

@juhaku
Copy link
Owner

juhaku commented Nov 1, 2024

@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 JMLX42 force-pushed the feat/ignore-value branch 2 times, most recently from a43f049 to 0bcb24a Compare November 1, 2024 18:22
@JMLX42
Copy link
Contributor Author

JMLX42 commented Nov 1, 2024

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 JMLX42 force-pushed the feat/ignore-value branch from 0bcb24a to 4eab0fc Compare November 1, 2024 18:24
@JMLX42 JMLX42 marked this pull request as draft November 1, 2024 18:26
@JMLX42
Copy link
Contributor Author

JMLX42 commented Nov 1, 2024

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
Copy link
Owner

juhaku commented Nov 1, 2024

Ok, I guess it can be then as is.

@JMLX42
Copy link
Contributor Author

JMLX42 commented Nov 1, 2024

Ok, I guess it can be then as is.

@juhaku nope. I'll make it work 👍

@JMLX42
Copy link
Contributor Author

JMLX42 commented Nov 1, 2024

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 ✔️ .

@JMLX42 JMLX42 marked this pull request as ready for review November 1, 2024 19:00
@juhaku
Copy link
Owner

juhaku commented Nov 1, 2024

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

@juhaku juhaku merged commit a792520 into juhaku:master Nov 1, 2024
12 checks passed
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.

2 participants