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

WIP: Init custom extensions - suggestion needed #82

Closed
wants to merge 4 commits into from

Conversation

pjankiewicz
Copy link

@pjankiewicz pjankiewicz commented Apr 21, 2021

Related to #50, #81.

I've implemented the ability to define custom tags so it is possible to define them like this:

#[derive(Default, Debug, JsonSchema, Serialize)]
#[schemars(extension = "struct_tag_1", extension = "struct_tag_2")]
pub struct Struct {
    #[schemars(extension = "field_tag_1", extension = "field_tag_2")]
    foo: i32,
    bar: bool,
}

fn struct_tag_1() -> (String, String) { ("x-tag-1".into(), "struct-tag-1".into()) }
fn struct_tag_2() -> (String, String) { ("x-tag-2".into(), "struct-tag-2".into()) }
fn field_tag_1() -> (String, String) { ("x-tag-1".into(), "field-tag-1".into()) }
fn field_tag_2() -> (String, String) { ("x-tag-2".into(), "field-tag-2".into()) }

I based the implementation on examples (I think it is very close in terms of handling) field and make it quite generic.

I currently have 1 problem because when I test the solution it shows differences in Metadata.extensions, and SchemaObject.extensions it is because I don't deserialize custom tags into proper containers. Generated json is ok, but the structs are not.

To be honest I don't know how to deal with this. Options:

  1. I can assume that every json key starting with x- must deserialize to extension fields.
  2. Skip checking extension fields
  3. Write custom test that only checks generated raw json and not SchemaObject

@GREsau
Copy link
Owner

GREsau commented Apr 25, 2021

I think I prefer the [schemars(extension("x-foo-bar" = "baz"))] syntax I suggested in #50, as it feels like a cleaner interface to adding extensions - at least when the keys of the extension properties are known at compile time, which I assume they generally would be. The downside of that is that it's much harder to implement in schemars_derive, as it doesn't conform to Meta, so we would no longer be able to use parse_meta() to parse schemars attributes

@pjankiewicz
Copy link
Author

I see your point. Your syntax would be ideal for String => String but values need to be more flexible than just strings. On the other hand a solution where

[schemars(extension("x-foo-bar" = "baz"))]

x-foo-bar is a String and baz is a function I think is worse than having a single function that returns both things. Maybe there should be 2 extensions types:

[schemars(extension("x-foo-bar" = "baz"))]   // String -> String
[schemars(extension_with("x-foo-bar" = "module::baz_func"))]  // String -> Serializable

In our code we have situations where the values is a nested structure so a second type of extension would be needed.

I don't know if you could accept the current flavour of the solution with possible rewrite in the future because:
a) it solves the problem
b) schemars_derive as you noticed is much more complicated solution

Still, I will dive deeper into schemars_derive. I don't know the code well enough to say whether it is 1 hour task or 2 days task.

@rustrial
Copy link

rustrial commented May 3, 2021

I guess String only custom extensions are not powerful enough. For example consider JSON Schemas from the Kubernetes OpenAPI specification respectively from CustomResourceDefinitions (CRD). In that case the keys are known at compile time, but the values might be arbitrary JSON values (String, bool, array of String and array of Object).

@clux
Copy link

clux commented May 11, 2021

String only extensions would still work in that use case for kubernetes extensions, i think, provided they are just passed along and attached correctly / flattened into the schema as opaque serde_json::Value instances.

kube-derive kind of does something like this with large keys like printcolum by encouraging rawstrings: https://github.com/clux/kube-rs/blob/286bac5f74f281638c6368f9ec70b855f3ccfc37/examples/crd_derive.rs#L22
a proc_macro could potentially try to coerce that into a struct at runtime to give some type safety.

though, if that interface gets awkward at size, people could always do it the manual way

@kahirokunn
Copy link

I need this feature.

@kahirokunn
Copy link

@pjankiewicz Are there any more plans to work here?
If not, would you like me to take over for you so that we can move forward with this feature?

@pjankiewicz
Copy link
Author

@kahirokunn I'm closing this PR. My initial proposal was good for me and it worked in the production. I don't plan to do anything beyond this since 2 years passed since I was looking at this.

@kahirokunn
Copy link

@pjankiewicz In other words, if you want an extension, create a new one with similar functionality. Is that what you are saying?

@pjankiewicz
Copy link
Author

@kahirokunn You can reuse my solution, but if you read previous messages this wasn't exactly the way @GREsau wanted it to be implemented.

@kahirokunn
Copy link

Okay. Thx

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.

None yet

5 participants