oxidecomputer / typify

compiler from JSON Schema into idiomatic Rust types
Apache License 2.0
401 stars 57 forks source link

Returning more descriptive errors for invalid enum defaults #626

Open oatmealdealer opened 2 months ago

oatmealdealer commented 2 months ago

When trying to generate a REST client crate from an OpenAPI spec, I ran into this error output from progenitor:

gen fail: TypeError(InvalidValue)
Error: generation experienced errors

After tracing the error to typify I realized the schema had some properties with invalid defaults. Here's a basic example:

{
    "$schema": "http://json-schema.org/draft-07/schema#",
    "definitions": {
        "Foo": {
            "title": "Foo",
            "type": "object",
            "properties": {
                "bar": {
                    "title": "Bar",
                    "enum": [
                        "Foo",
                        "Bar"
                    ],
                    "type": "string",
                    "default": "Baz"
                }
            },
            "additionalProperties": false
        }
    }
}

It was this match arm that ended up being the one for my case: https://github.com/oxidecomputer/typify/blob/d36f65eea4356374a1d7e68009ecc210756ec755/typify-impl/src/defaults.rs#L151-L153

This is a slightly cleaned up version of the change that gave me enough info to find the problem with my schema file:

EnumTagType::External => validate_default_for_external_enum(
    type_space, variants, default,
)
.ok_or_else(|| Error::InvalidSchema {
    type_name: self.name().map(String::to_owned),
    reason: format!(
        "Invalid default {}, must be one of: {:?}",
        default,
        variants
            .iter()
            .map(|variant| variant.name.to_owned())
            .collect::<Vec<String>>()
    ),
}),

Ideally the error message could just be a JSON path like mentioned here: https://github.com/oxidecomputer/typify/issues/485#issuecomment-1896464088. I assume that would require a more structural refactor, but I'd be interested in pushing errors like this in the intended direction either way.

ahl commented 2 months ago

Thanks for the great analysis. Improved errors are a work in progress.

oatmealdealer commented 2 months ago

Thanks for the great analysis. Improved errors are a work in progress.

Thanks for the response - I hadn't seen #579 when I posted this, and I realize after the fact that "make the errors better," let alone "provide an exact JSON path with every validation error," is a way bigger ask than one might think at first glance.

I'm guessing the challenge is that typify is built on top of schemars which is built on top of serde, and serde hasn't solved this problem itself yet: https://github.com/serde-rs/json/issues/173.

It also doesn't seem like the original structure of the JSON document is explicitly preserved in memory anywhere by the time typify gets a hold of it: https://github.com/oxidecomputer/typify/blob/da943a092abc629a499f73c680c876b4ba00519b/typify-macro/src/lib.rs#L232-L239 I assume this means you'd have to extend serde to run typify's validations during deserialization while the path to a given value in the document is still available. On the other hand, from what I understand JSON schemas can also be self-referential in a lot of ways, so I'd expect at least some validations to be impossible until the schema has been fully deserialized.

I'd be interested in helping with a solution if you think it's a problem that would benefit from contribution, but I can definitely see why that might not be the case. This isn't blocking me from using progenitor for the purpose I had in mind though - so no worries if this is something that's just going to take time to figure out.