omissis / go-jsonschema

A tool to generate Go data types from JSON Schema definitions.
MIT License
558 stars 89 forks source link

Validation breaks unmarhalling into non-empty struct #250

Open kleijnweb opened 2 months ago

kleijnweb commented 2 months ago

By default, when you unmarshal bytes passing a struct, it will keep the existing data. This is particularly useful for PATCH updates: you fetch the data, then apply the patch, and THEN validate. Alternative would be to have separate structs for POST/PUT and PATCH which is a huge PITA for simple applications that have only 2 concerns: performance and correctness. Also on top of the need for separate schemas, you would then have to validate the result after copying over all the data from the partial struct to the full struct, which as it stands would mean marshalling and unmarshalling it, as the only way to do validation is to unmarshall. Long story short, it's completely unusable in its current state, which is a shame because it could be a huge time saver if it worked a bit better.

The generated code has 2 issues that prevents partial decodes:

  1. It first decodes into a map

Aside from the fact that this starts with an empty map (and will thus fail), this is terribly inefficient, because it marshalls the data twice. I recognize that it is impossible to distinguish between a string that is not marshalled into and one that had an empty string marshalled into it, and that if a field is required but without minLenght > 0, that is a valid value. I don't really have a solution for that.

But, the marshalling into an map also happens when the minLength is > 0 everywhere, and EVEN when there are 0 required fields. A complete waste of resources that is fixable.

Proposed fix: only decode into a map if there are properties that are required and where the zero-value is valid. Which is common but often not what people want, especially with strings. In many cases you can then get rid of the double unmarshall by fixing your schema.

  1. It unmarshalls into an empty value of the type (an alias of it)

Obviously existing values are lost when you do that.

Proposed fix:

type Plain MyType

// copy original
plain := Plain(*j)

// unmarshal into copy
if err := json.Unmarshal(b, &plain); err != nil {
    return err
}

// validate fields of `plain`, then copy the updated data back into the original
*j = MyType(plain)