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

Remove additional properties in schema generation #1307

Closed
ethanfrey opened this issue May 16, 2022 · 13 comments · Fixed by #1345
Closed

Remove additional properties in schema generation #1307

ethanfrey opened this issue May 16, 2022 · 13 comments · Fixed by #1345
Milestone

Comments

@ethanfrey
Copy link
Member

This is a bit annoying with the generated typescript types, with {[k: string]: unknown}.

We can turn it off by adding additionalProperties: false in the json schema we export.

@ethanfrey
Copy link
Member Author

It seems you can accomplish this with.. #[serde(deny_unknown_fields)]: https://github.com/GREsau/schemars/blob/master/CHANGELOG.md#074---2020-05-16

@webmaster128
Copy link
Member

additionalProperties is used already. It came with some upgrade of schemars many months ago.

Where exactly do you see the problem?

@ethanfrey
Copy link
Member Author

ethanfrey commented May 16, 2022

Basically, it works if you just use:

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
#[serde(deny_unknown_fields)]
pub struct MigrateMsg {
    pub payout: String,
}

@webmaster128
Copy link
Member

I see. Then we only have "additionalProperties": false for the enums now.

@ethanfrey
Copy link
Member Author

Yes, but only on the top level, not inside each variant. Or the results.

@ethanfrey
Copy link
Member Author

@JakeHartnell check this out

@pyramation
Copy link

pyramation commented May 26, 2022

Should we start using #[serde(deny_unknown_fields)] in other contracts or should we explore other ways of setting the additional properties field to false?

I'm not a rust dev, but I'm still curious — is it possible to make a macro for the macro?

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
#[serde(deny_unknown_fields)]

becomes super simplified, something like

#[cosmjson(JsonSchema)]

which would expand into the stuff it needs. I guess it could also give us flexibility later to change whatever the macro does in case we create new methods for packaging contracts.

@webmaster128
Copy link
Member

Should we start using #[serde(deny_unknown_fields)] in other contracts

I think this is fine for now. It will disallow additional fields during deserialization as a side effect which might even be helpful to detect wrong JSON requests sent by the client.

A custom macro would be more work, also not trivial because of interactions with other macros. We can go that route mid term for sure but it will not be quick. Good to know what we need before.

@iboss-ptk
Copy link

iboss-ptk commented Jul 1, 2022

@webmaster128 shouldn't this be simple as like

#[proc_macro_attribute]
pub fn cosmwasm_schema(_metadata: proc_macro::TokenStream, input: proc_macro::TokenStream)
                 -> proc_macro::TokenStream {
    let input: TokenStream = input.into();
    let output = quote! {
        #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
        #[serde(deny_unknown_fields)]
        #input
    };
    output.into()
}
#[cosmwasm_schema]
struct QuerySomething { .. }

utilizing attribute macro to modify derives.

@webmaster128
Copy link
Member

@uint could you check out this suggestion? 👆

@iboss-ptk
Copy link

And with attribute macro, we should theoretically, do things like

#[cosmwasm_schema_mod]
mod msg {
  #[cosmwasm_schema]
  pub enum QueryMsg { .. }
}

That is generate single export_schema command so that whenever new type (mostly response type) being introduced, there is no need to keep adding lines in schema.rs.

@uint
Copy link
Contributor

uint commented Jul 2, 2022

@uint could you check out this suggestion? point_up_2

Yeah, I tried to avoid attribute macros since derives seem less "invasive", but they also complicate things for us implementation-wise. I think we should go for the attribute macro.

I don't know if I'd go so far as to create an attribute macro for the whole module. At that point we're duplicating the work Sylvia is already doing and we should probably focus on releasing Sylvia faster instead.

@uint
Copy link
Contributor

uint commented Jul 11, 2022

@iboss-ptk I started a PR that implements your suggestion: #1345

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 a pull request may close this issue.

5 participants