supabase / pg_jsonschema

PostgreSQL extension providing JSON Schema validation
Apache License 2.0
1.03k stars 30 forks source link

feat: add the validate utils #32

Closed duguorong009 closed 1 year ago

duguorong009 commented 1 year ago

What kind of change does this PR introduce?

Add the validate utils for getting the actual validation errors thrown while validating the json schema and instance.

What is the current behavior?

Closes #14

What is the new behavior?

Receives the actual errors thrown, as Vec<String>

Additional context

olirice commented 1 year ago

hi @duguorong009 please take a look at the CI errors when you get a minute thanks!

olirice commented 1 year ago

sorry for the delay getting back to you, its been a hectic week will leave a review nlt tomorrow

olirice commented 1 year ago

@duguorong009 sorry for the back and forth

It's not clear to me what we're solving here. #14 requested exposing validate() which I understood to mean adding a function that would return an array of error strings.

Given that there is a performance hit for using validate vs is_valid we're trading performance for hints.

But check constraints won't bubble up those hints, logs would need to be set to verbose to include hints, and supabase studio doesn't display hints (AO 2023-07). So the only time the hints would be visible is if a user interactively connected to the database. Due to that, I'm not sure this is an appropriate trade to make.


If you'd be interested in changing this to a single immutable function (json only, not jsonb)

fn jsonschema_validation_errors(schema: Json, instance: Json) -> Vec<String>

that returns an array of errors or an empty array (if there are no errors) I'd be happy to approve

thanks!

duguorong009 commented 1 year ago

@duguorong009 sorry for the back and forth

It's not clear to me what we're solving here. #14 requested exposing validate() which I understood to mean adding a function that would return an array of error strings.

Given that there is a performance hit for using validate vs is_valid we're trading performance for hints.

But check constraints won't bubble up those hints, logs would need to be set to verbose to include hints, and supabase studio doesn't display hints (AO 2023-07). So the only time the hints would be visible is if a user interactively connected to the database. Due to that, I'm not sure this is an appropriate trade to make.

If you'd be interested in changing this to a single immutable function (json only, not jsonb)

fn jsonschema_validation_errors(schema: Json, instance: Json) -> Vec<String>

that returns an array of errors or an empty array (if there are no errors) I'd be happy to approve

thanks!

Thanks for the detailed tips! @olirice

olirice commented 1 year ago

looks good

please include tests for 0 errors, 1 error, and multiple errors:

e.g.

// No errors
crate::jsonschema_validation_errors(
    Json(json!({ "maxLength": 4 })),
    Json(json!("foo")),
)

// One error
crate::jsonschema_validation_errors(
    Json(json!({ "maxLength": 4 })),
    Json(json!("123456789")),
)

// Multiple errors
crate::jsonschema_validation_errors(
    Json(json!(
    {
        "type": "object",
        "properties": {
            "foo": {
                "type": "string"
            },
            "bar": {
                "type": "number"
            },
            "baz": {
                "type": "boolean"
            },
            "additionalProperties": false,
            "required": ["foo", "bar", "baz"]
        }
    })),
    Json(json!({"foo": 1, "bar": [], "bat": true})),
)
olirice commented 1 year ago

thanks!