#1449: JSON Schema validation and verification #5486

jkulvich · 2022-12-15T14:09:10Z

Added new built-in functions:

# Accepts 1 argument which can be string or object and checks if it is valid JSON schema.
jsonschema.is_valid(schema): bool

# Accepts 1 argument which can be string or object and checks if it is valid JSON schema.
# Returns string in case of error or empty string otherwise.
jsonschema.validate(schema): string

# Accepts 2 arguments both can be string or object and verifies if the document matches the JSON schema.
# Returns true if the document matches the schema.
json.is_match_schema(document, schema): bool

# Accepts 2 arguments both can be string or object and verifies if the document matches the JSON schema.
# Returns an array of errors or empty array.
# Returns an empty array if no errors are found.
# In case of internal error returns empty array.
json.match_schema(document, schema): []struct{ error: string, type: string, field: string, desc: string }

schema and document can be JSON string or object.

Signed-off-by: Yuri Kulagin jkulvichi@gmail.com

netlify · 2022-12-15T14:10:20Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`1330cdf`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/6400791c359a4c000826ae42
😎 Deploy Preview	https://deploy-preview-5486--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

srenatus

Thanks for contributing. Some inline comments, and some tests would be required, too. See test/cases/testdata

topdown/jsonschema.go

Signed-off-by: Yuri Kulagin <jkulvichi@gmail.com>

jkulvich · 2022-12-23T21:44:16Z

@srenatus, I have covered the functions with tests, and added a bit more comments. Can I have your review? :)

srenatus

Sorry I wasn't explicit enough -- it would be preferable to have the tests in test/cases/testdata, which is a bunch of yaml files. They are also used by other projects to ensure conformance when the builtins are implemented in other ways -- e.g. in Wasm, or in IR-based projects.

That said, I'm not completely certain about the error-returning there; I don't believe we've done that in any other place. @philipaconrad, what do you think? I suppose having some alignment between the graphql schema verification builtins we already have, and the new ones here, would make sense.

topdown/jsonschema.go

topdown/jsonschema_test.go

ast/builtins.go

srenatus · 2023-01-02T10:00:24Z

ast/builtins.go

+	),
+}
+
+// JSONMatchSchema returns empty array if the document matches the JSON schema,


Do we need both json.is_match_schema and json.match_schema then? It seems like this rego snippet would do to implement the former:

is_match_schema(doc, schema) if json.match_schema(doc, schema) == []

Agree. Do we also need jsonschema.is_valid?
This can be implemented in the same way using jsonschema.verify.

philipaconrad · 2023-01-17T15:59:14Z

That said, I'm not completely certain about the error-returning there; I don't believe we've done that in any other place.

@srenatus You are correct-- no other builtins return error messages as part of their normal output. We've had requests in the past for something like this, for example as part of the GraphQL builtins.

I believe we've avoided implementing any such feature so far, because doing so could expose a lot of unnecessary implementation details, and returning error messages does not mesh with the existing style of error-handling/validation logic for policies. 🙁

ashutosh-narkar · 2023-01-23T19:07:07Z

@jkulvich these builtins would be a great addition! When you get a chance please do address the comments in this review and also about adding tests in test/cases/testdata as suggested here. Please let us know if can help. Thanks for contribution!

Signed-off-by: Yuri Kulagin <jkulvichi@gmail.com>

jkulvich · 2023-01-27T14:00:02Z

I believe we've avoided implementing any such feature so far, because doing so could expose a lot of unnecessary implementation details, and returning error messages does not mesh with the existing style of error-handling/validation logic for policies. 🙁

@philipaconrad, indeed, I have not seen previously implemented functions return error information.

However, in our case, it is required to provide the user not only a binary decision of the rule, but also technical info about why the error occurred.

For this reason, simplified is_ functions are proposed. However, they can be replaced with snippets, as @srenatus mentioned

Signed-off-by: Yuri Kulagin <jkulvichi@gmail.com>

ashutosh-narkar · 2023-01-31T08:49:44Z

topdown/jsonschema.go

+		asJSON, err = ast.JSON(value)
+		if err != nil {
+			return nil, err
+		}


We can probably use gojsonschema.NewGoLoader

ashutosh-narkar · 2023-01-31T08:59:55Z

topdown/jsonschema.go

+// Returns an array of errors or empty array.
+// Returns an empty array if no errors are found.
+// In case of internal error returns empty array.
+func builtinJSONMatchSchema(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error {


We need to specify in the docs that the first argument is the document to match with the schema and second is the schema itself.

ashutosh-narkar · 2023-01-31T09:02:42Z

topdown/jsonschema.go

+	// Make new schema instance to provide validations.
+	schema, err := gojsonschema.NewSchema(schemaLoader)
+	if err != nil {
+		return err
+	}
+
+	// Use the schema instance to validate the document.
+	result, err := schema.Validate(documentLoader)
+	if err != nil {
+		return err
+	}


We could also do if you like:

gojsonschema.Validate(schemaLoader, documentLoader)

ashutosh-narkar · 2023-01-31T09:17:42Z

However, in our case, it is required to provide the user not only a binary decision of the rule, but also technical info about why the error occurred.

Directly returning an error string from the builtin does not seem the right approach. How about returning an object from the builtin and one of the fields could be an error field whose value will be an object.

ashutosh-narkar · 2023-02-09T16:42:43Z

@jkulvich if you like we are happy to carry forward your great work and add tests etc. These are useful builtins and if you need our help to move this along we're happy to do it. Let us know what you think. Thanks!

johanfylling

I've taken the liberty to update this PR according to review comments.

johanfylling · 2023-02-24T16:25:48Z

builtin_metadata.json

@@ -8726,6 +8753,26 @@
    },
    "wasm": true
  },
+  "jsonschema.verify": {


Should this perhaps be json.verify_schema?

Yes json.match_schema and json.verify_schema would be better.

mjungsbluth · 2023-02-26T23:47:29Z

To link this to the actual issue, this PR implements issue #1449 .

* Merging `jsonschema.is_valid` into `jsonschema.verify`, and `json.is_match_schema` into `json.match_schema` * Adding yaml test cases in testdata Signed-off-by: Johan Fylling <johan.dev@fylling.se>

ashutosh-narkar

This is looking good. Few comments.

ashutosh-narkar · 2023-02-27T22:21:03Z

ast/builtins.go

+				),
+			),
+		}, nil)).
+			Description("`output` is of the form `[match, errors]`. If the document is valid given the schema, then `match` is `true`, and `errors` is an empty array. Otherwise, `match` is `true` and `errors` is an array of objects describing the error(s)."),


Do you mean "Otherwise, match is false" ?

Ah, yes, of course 🙄 .

ashutosh-narkar · 2023-02-27T22:25:56Z

builtin_metadata.json

@@ -8726,6 +8753,26 @@
    },
    "wasm": true
  },
+  "jsonschema.verify": {


Yes json.match_schema and json.verify_schema would be better.

ashutosh-narkar · 2023-02-27T22:32:40Z

topdown/jsonschema.go

+
+// builtinJSONSchemaValidate accepts 1 argument which can be string or object and checks if it is valid JSON schema.
+// Returns array [false, <string>] with error string at index 1, or [true, ""] with empty string at index 1 otherwise.
+func builtinJSONSchemaValidate(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error {


Nit: lets update the function name to builtinJSONSchemaVerify

ashutosh-narkar · 2023-02-27T22:40:56Z

topdown/jsonschema.go

+			[...]*ast.Term{ast.StringTerm("error"), ast.StringTerm(re.String())},
+			[...]*ast.Term{ast.StringTerm("type"), ast.StringTerm(re.Type())},
+			[...]*ast.Term{ast.StringTerm("field"), ast.StringTerm(re.Field())},
+			[...]*ast.Term{ast.StringTerm("desc"), ast.StringTerm(re.Description())},


Do we need both re.String() and re.Description()? I mean do they contain different enough that can help in debug etc. or is one them enough ? https://sourcegraph.com/github.com/open-policy-agent/opa@7b8fa09ff8fb3d447abeefa5d257fbc00e4a2ee6/-/blob/internal/gojsonschema/result.go?L159

Looks like the string representation contains field and description, which we include in the error object; but it also includes context and value. Unless the formatting of the error string makes it much more readable, perhaps we could skip including it, and instead include context and value(?).
Although, I don't see harm in including it, as the formatting of the string might make the data more human readable.

Sounds good 👍

ashutosh-narkar · 2023-02-27T22:51:48Z

topdown/jsonschema.go

+
+	// Check that schema is correct and parses without errors.
+	if _, err = gojsonschema.NewSchema(loader); err != nil {
+		return iter(newResultTerm(false, ast.StringTerm("jsonschema: "+err.Error())))


Instead of returning a string here, what if we return an object with an error field describing the error similar to what http.send does?

Now, we're returning an array with a boolean and an error string, similar (but kinda reversed) to what graphql.parse_and_verify does. E.g.

deny[err] { [false, err] = jsonschema.verify(input.schema) }

We could return an object instead, if that is preferable.

This seems fine. My reasoning behind using an object was it gives us flexibility to add more fields if needed.

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

ashutosh-narkar · 2023-02-28T17:21:32Z

topdown/jsonschema.go

+
+	// Check that schema is correct and parses without errors.
+	if _, err = gojsonschema.NewSchema(loader); err != nil {
+		return iter(newResultTerm(false, ast.StringTerm("jsonschema: "+err.Error())))


This seems fine. My reasoning behind using an object was it gives us flexibility to add more fields if needed.

rbuckland · 2023-03-06T14:25:16Z

Yes well done. This will be super helpful.

hajdukda · 2023-08-18T11:51:37Z

Is there any official documentation in OPA about using this feature ?

jkulvich · 2023-08-19T07:42:52Z

Is there any official documentation in OPA about using this feature ?

@hajdukda, please, follow next OPA docs sections json.match_schema, and json.verify_schema

jkulvich force-pushed the 1449-jsonschema branch from f2fab75 to 4be3a6d Compare December 16, 2022 08:04

srenatus reviewed Dec 16, 2022

View reviewed changes

topdown/jsonschema.go Outdated Show resolved Hide resolved

topdown/jsonschema.go Outdated Show resolved Hide resolved

jkulvich force-pushed the 1449-jsonschema branch from 0f50b95 to f26c589 Compare December 23, 2022 14:56

jsonschema validation

5d64604

Signed-off-by: Yuri Kulagin <jkulvichi@gmail.com>

jkulvich closed this Dec 23, 2022

jkulvich force-pushed the 1449-jsonschema branch from f26c589 to 9bd1bfb Compare December 23, 2022 15:07

jkulvich reopened this Dec 23, 2022

srenatus reviewed Jan 2, 2023

View reviewed changes

ashutosh-narkar added the waiting-for-response label Jan 23, 2023

Better objects construction, and error handling

1900c45

Signed-off-by: Yuri Kulagin <jkulvichi@gmail.com>

rename: jsonschema.validate -> jsonschema.verify

deb6690

Signed-off-by: Yuri Kulagin <jkulvichi@gmail.com>

ashutosh-narkar removed the waiting-for-response label Jan 27, 2023

ashutosh-narkar reviewed Jan 31, 2023

View reviewed changes

ashutosh-narkar mentioned this pull request Feb 6, 2023

Support for schema verification on input/data in server mode #5572

Closed

Merge branch 'main' into 1449-jsonschema

8f4d04c

johanfylling reviewed Feb 24, 2023

View reviewed changes

johanfylling force-pushed the 1449-jsonschema branch from 586d605 to 0501498 Compare February 25, 2023 21:42

johanfylling mentioned this pull request Feb 27, 2023

Add JSON Schema validation built-ins #5702

Closed

Addressing review comments

85685e3

* Merging `jsonschema.is_valid` into `jsonschema.verify`, and `json.is_match_schema` into `json.match_schema` * Adding yaml test cases in testdata Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling force-pushed the 1449-jsonschema branch from 0501498 to 85685e3 Compare February 27, 2023 19:55

ashutosh-narkar reviewed Feb 27, 2023

View reviewed changes

johanfylling force-pushed the 1449-jsonschema branch from 1bbc4f2 to ee7e293 Compare February 28, 2023 13:17

Making changes suggested by @ashutosh-narkar

9e8e10a

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling force-pushed the 1449-jsonschema branch from ee7e293 to 9e8e10a Compare February 28, 2023 13:34

ashutosh-narkar approved these changes Feb 28, 2023

View reviewed changes

Merge branch 'main' into 1449-jsonschema

1330cdf

johanfylling merged commit 334f27e into open-policy-agent:main Mar 2, 2023

anderseknert mentioned this pull request Mar 6, 2023

Add built-in funtion to support JSON Schema Validation #1449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1449: JSON Schema validation and verification #5486

#1449: JSON Schema validation and verification #5486

jkulvich commented Dec 15, 2022 •

edited

Loading

netlify bot commented Dec 15, 2022 •

edited

Loading

srenatus left a comment

jkulvich commented Dec 23, 2022

srenatus left a comment

srenatus Jan 2, 2023

jkulvich Jan 27, 2023

philipaconrad commented Jan 17, 2023 •

edited

Loading

ashutosh-narkar commented Jan 23, 2023

jkulvich commented Jan 27, 2023 •

edited

Loading

ashutosh-narkar Jan 31, 2023

ashutosh-narkar Jan 31, 2023

ashutosh-narkar Jan 31, 2023

ashutosh-narkar commented Jan 31, 2023

ashutosh-narkar commented Feb 9, 2023

johanfylling left a comment

johanfylling Feb 24, 2023

ashutosh-narkar Feb 27, 2023

mjungsbluth commented Feb 26, 2023

ashutosh-narkar left a comment

ashutosh-narkar Feb 27, 2023

johanfylling Feb 28, 2023

ashutosh-narkar Feb 27, 2023

ashutosh-narkar Feb 27, 2023

ashutosh-narkar Feb 27, 2023

johanfylling Feb 28, 2023

ashutosh-narkar Feb 28, 2023

ashutosh-narkar Feb 27, 2023

johanfylling Feb 28, 2023

ashutosh-narkar Feb 28, 2023

ashutosh-narkar Feb 28, 2023

rbuckland commented Mar 6, 2023

hajdukda commented Aug 18, 2023

jkulvich commented Aug 19, 2023 •

edited

Loading

#1449: JSON Schema validation and verification #5486

#1449: JSON Schema validation and verification #5486

Conversation

jkulvich commented Dec 15, 2022 • edited Loading

netlify bot commented Dec 15, 2022 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

srenatus left a comment

Choose a reason for hiding this comment

jkulvich commented Dec 23, 2022

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipaconrad commented Jan 17, 2023 • edited Loading

ashutosh-narkar commented Jan 23, 2023

jkulvich commented Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar commented Jan 31, 2023

ashutosh-narkar commented Feb 9, 2023

johanfylling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjungsbluth commented Feb 26, 2023

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckland commented Mar 6, 2023

hajdukda commented Aug 18, 2023

jkulvich commented Aug 19, 2023 • edited Loading

jkulvich commented Dec 15, 2022 •

edited

Loading

netlify bot commented Dec 15, 2022 •

edited

Loading

philipaconrad commented Jan 17, 2023 •

edited

Loading

jkulvich commented Jan 27, 2023 •

edited

Loading

jkulvich commented Aug 19, 2023 •

edited

Loading