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

1163 - Json validation #1343

Merged
merged 2 commits into from Feb 14, 2018

Conversation

Projects
None yet
7 participants
@asoorm
Copy link
Member

asoorm commented Dec 5, 2017

Fixes #1163
Based on #1180

Adds a new JSON Validation middleware that can be configured as follows:

"version_data": {
  "not_versioned": true,
  "versions": {
    "default": {
      "name": "default",
      "use_extended_paths": true,
      "extended_paths": {
        "validate_json": [{
          "method": "POST",
          "path": "me",
          "schema": schema,
          "error_response_code": 422 // 422 default however can override.
        }]
      }
    }
  }
},

The schema must be a draft v4 JSON Schema spec. The gateway will attempt
to validate the inbound request against it, if fields are failing the
validation process, a detailed error response is provided for the user
to fix their payload.

example schema:

{
    "title": "Person",
    "type": "object",
    "properties": {
        "firstName": {
            "type": "string"
        },
        "lastName": {
            "type": "string"
        },
        "age": {
            "description": "Age in years",
            "type": "integer",
            "minimum": 0
        }
    },
    "required": ["firstName", "lastName"]
}

This will require a new Dashboard UI to handle input.

@asoorm asoorm force-pushed the json-validation-ahm branch 7 times, most recently from b645d4c to aca8584 Dec 5, 2017

@asoorm asoorm changed the title [WIP] Json validation ahm 1163 - Json validation Dec 11, 2017

@asoorm asoorm requested review from buger and dencoded Dec 11, 2017

@@ -699,6 +702,20 @@ func (a APIDefinitionLoader) compileTrackedEndpointPathspathSpec(paths []apidef.
return urlSpec
}

func (a APIDefinitionLoader) compileValidateJSONPathspathSpec(paths []apidef.ValidatePathMeta, stat URLStatus) []URLSpec {
urlSpec := []URLSpec{}

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

To avoid slice re-allocation I think we could use urlSpec := make([]URLSpec, len(paths)) and then do for index, pathVal .. urlSpec[index] = newSpec

This comment has been minimized.

@asoorm

asoorm Dec 13, 2017

Author Member

I like this idea - but I think for consistency, we keep same as rest of api_definition.go. Will create separate GH refactor issue for avoiding slice re-allocation.

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

why introduce new code which will be subject of refactoring later? I mean we can refactor existing code as separated task for sure, but new changes - why not to do them better at the very beginning (if we believe they are better)

return nil, 200
}

mmeta := meta.(*apidef.ValidatePathMeta)

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

mmeta and meta - it is probably better to use variables with more different names (than one-letter difference - source of potential bugs)

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

also, might worth to do safe type assert meta, ok := meta.(*apidef.ValidatePathMeta) to avoid panics. I know it is unlikely but still..

rCopy := copyRequest(r)
bodyBytes, err := ioutil.ReadAll(rCopy.Body)
if err != nil {
return err, 400

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

I would count it as 500 reply as something might happen on communication level and retry of the same request seems to be a valid situation


mmeta := meta.(*apidef.ValidatePathMeta)
if mmeta.ValidateWith64 == "" {
return errors.New("no schemas to validate against"), 400

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

this seems to be not a problem with incoming requests as something is not right with API spec setup on our end

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

I mean it is probably not a 400


schemaBytes, err := base64.StdEncoding.DecodeString(mmeta.ValidateWith64)
if err != nil {
return errors.New("unable to base64 decode schema"), 400

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

again, I think it could be not a 400 as incoming request seems to be fine


result, err := k.validate(bodyBytes, schemaBytes)
if err != nil {
return err, 400

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

might worth to log err so we will be able to explain to customer why it was 400

This comment has been minimized.

@asoorm

asoorm Dec 13, 2017

Author Member

@dencoded - Agree with status code changes and have implemented these.
The reason I have not logged before returning error is that IMO it's bad practice to log the error before returning. It's up to the caller of ProcessRequest to do the logging if it needs to. Open to thoughts on this.

if !result.Valid() {
errStr := "payload validation failed"
for _, desc := range result.Errors() {
errStr = fmt.Sprintf("%s: %s", errStr, desc)

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

I think errStr will contain in this case the latest element from result.Errors(), if we want to show all errors we should provide something like comma-separated list

This comment has been minimized.

@asoorm

asoorm Dec 13, 2017

Author Member

Check tests. Provides a , separated list.

"validate_json": [{
"method": "POST",
"path": "me",
"validate_with_64": "ew0KICAgICJ0aXRsZSI6ICJQZXJzb24iLA0KICAgICJ0eXBlIjogIm9iamVjdCIsDQogICAgInByb3BlcnRpZXMiOiB7DQogICAgICAgICJmaXJzdE5hbWUiOiB7DQogICAgICAgICAgICAidHlwZSI6ICJzdHJpbmciDQogICAgICAgIH0sDQogICAgICAgICJsYXN0TmFtZSI6IHsNCiAgICAgICAgICAgICJ0eXBlIjogInN0cmluZyINCiAgICAgICAgfSwNCiAgICAgICAgImFnZSI6IHsNCiAgICAgICAgICAgICJkZXNjcmlwdGlvbiI6ICJBZ2UgaW4geWVhcnMiLA0KICAgICAgICAgICAgInR5cGUiOiAiaW50ZWdlciIsDQogICAgICAgICAgICAibWluaW11bSI6IDANCiAgICAgICAgfQ0KICAgIH0sDQogICAgInJlcXVpcmVkIjogWyJmaXJzdE5hbWUiLCAibGFzdE5hbWUiXQ0KfQ=="

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

I'd prefer to see in test the actual schema instead of base64, you can encode it when put value here

}
defer rCopy.Body.Close()

schemaBytes, err := base64.StdEncoding.DecodeString(mmeta.ValidateWith64)

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

might worth to cache schemaBytes as this might be too expensive to decode the same bas64 encoded string with every incoming through this middleware request

This comment has been minimized.

@asoorm

asoorm Dec 13, 2017

Author Member

@dencoded nice - suggestions on implementation / examples of where done before please?

E.g. create internal cache within application? Use Redis?

Finally - would you classify as significant for added complexity vs limited perf gain possibly only when schema is massive?
https://gist.github.com/asoorm/5ed9996cda838a7a05ee557b4ca077af

$ go test --bench . -benchmem
BenchmarkBase64DecodeString-4            1000000              1781 ns/op             992 B/op          2 allocs/op
--- BENCH: BenchmarkBase64DecodeString-4
        b64_test.go:19: decode len 516
        b64_test.go:19: decode len 516
        b64_test.go:19: decode len 516
        b64_test.go:19: decode len 516
BenchmarkBase64DecodeString2-4              5000            250250 ns/op          147456 B/op          2 allocs/op
--- BENCH: BenchmarkBase64DecodeString2-4
        b64_test.go:27: decode len 81376
        b64_test.go:27: decode len 81376
        b64_test.go:27: decode len 81376
PASS
ok      github.com/TykTechnologies/tyk/perftest 3.088s

This comment has been minimized.

@dencoded

dencoded Dec 14, 2017

Contributor

I feel like decoding/loading json-schema is an operation which needs to be done when we load/reload API spec definition, so we can put loaded/ready to use json-schema in some additional field and use it

@dencoded
Copy link
Contributor

dencoded left a comment

I left some comments, I open for discussion about any of them though

@asoorm asoorm force-pushed the json-validation-ahm branch 4 times, most recently from 18f8934 to 403fcc3 Dec 13, 2017

@asoorm asoorm changed the title 1163 - Json validation [WIP] 1163 - Json validation Dec 13, 2017


func (k *ValidateJSON) validate(input []byte, schema map[string]interface{}) (*gojsonschema.Result, error) {
inputLoader := gojsonschema.NewBytesLoader(input)
rulesLoader := gojsonschema.NewGoLoader(schema)

This comment has been minimized.

@dencoded

dencoded Dec 13, 2017

Contributor

this is related to loading/decoding of json schema. I feel like validating/parsing and loading of schema is the task for schema-loading process, we don't need to do this when processing every request inside a middleware

This comment has been minimized.

@buger

buger Dec 14, 2017

Member

Since middleware created per API, I think it should be ok to add some field to ValidateJSON middleware and cache this schema.

@asoorm asoorm force-pushed the json-validation-ahm branch 3 times, most recently from 32291e4 to f9fc223 Jan 14, 2018

@asoorm asoorm force-pushed the json-validation-ahm branch 2 times, most recently from 73a5d95 to c4bf4da Jan 15, 2018

@asoorm asoorm added this to the Release 2.5 milestone Jan 15, 2018

@asoorm asoorm changed the title [WIP] 1163 - Json validation 1163 - Json validation Jan 15, 2018

@asoorm asoorm force-pushed the json-validation-ahm branch from c4bf4da to 4ede633 Jan 21, 2018

@asoorm

This comment has been minimized.

Copy link
Member Author

asoorm commented Jan 21, 2018

@buger @dencoded - Are we ok to merge / send to @ilijabojanovic for test? I am struggling with dashboard component testing given that it relies on api definition within this repo.

@asoorm asoorm force-pushed the json-validation-ahm branch from 4ede633 to 3dc8691 Jan 22, 2018

@buger buger force-pushed the json-validation-ahm branch from 792e346 to 2dcfe9c Jan 22, 2018

@asoorm asoorm force-pushed the json-validation-ahm branch from 2dcfe9c to 80c9ac8 Jan 31, 2018

@buger buger removed this from the Release 2.5.1 milestone Feb 1, 2018

Adds JSON Schema validation to the gateway
Fixes #1163
Based on #1180

Adds a new JSON Validation middleware that can be configured as follows:

```
"version_data": {
  "not_versioned": true,
  "versions": {
    "default": {
      "name": "default",
      "use_extended_paths": true,
      "extended_paths": {
        "validate_json": [{
          "method": "POST",
          "path": "me",
          "validate_with": {
            "title": "Person",
            "type": "object",
            "properties": {
              "firstName": {
                "type": "string"
              },
              "lastName": {
                "type": "string"
              },
              "age": {
                "description": "Age in years",
                "type": "integer",
                "minimum": 0
              }
            },
            "required": ["firstName", "lastName"]
          }
        }]
      }
    }
  }
},
```

The schema must be a draft v4 JSON Schema spec. The gateway will attempt
to validate the inbound request against it, if fields are failing the
validation process, a detailed error response is provided for the user
to fix their payload.

This will require a new Dashboard UI to handle input.

make Base64 Schema readable in tests

removing base64 as not necessary

using make for known length slice

@asoorm asoorm force-pushed the json-validation-ahm branch 4 times, most recently from de884c4 to d69f119 Feb 11, 2018

return errors.New("no schemas to validate against"), http.StatusInternalServerError
}

if vPathMeta.SchemaVersion != "" && vPathMeta.SchemaVersion != "draft-v4" {

This comment has been minimized.

@asoorm

asoorm Feb 11, 2018

Author Member

Only checks that is Empty string or not draft-v4

@asoorm asoorm force-pushed the json-validation-ahm branch from d69f119 to aa6e459 Feb 11, 2018

Refactor tests to use new framework
In addition updates error code and message for not valid json payloads

Require schema_version

@asoorm asoorm force-pushed the json-validation-ahm branch from aa6e459 to 4e062f2 Feb 12, 2018

@buger buger merged commit e694b66 into master Feb 14, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 52.861%
Details

@buger buger deleted the json-validation-ahm branch Feb 14, 2018

@philsturgeon

This comment has been minimized.

Copy link

philsturgeon commented Feb 21, 2018

Hey there! I’m really excited about this, but Draft v04 is fairly outdated. Is there a plan to add support for more recent versions of the spec? Seeing as they declare the Schema in the file, it should be easy enough.

@asoorm

This comment has been minimized.

Copy link
Member Author

asoorm commented Feb 21, 2018

We are a bit limited with golang libs http://json-schema.org/implementations.html#validator-go .

I see that there is now a new lib https://github.com/santhosh-tekuri/jsonschema (untested by us) which supports draft6 but nothing yet for draft7. Any thoughts @buger @lonelycode ?

@matiasinsaurralde

This comment has been minimized.

Copy link
Member

matiasinsaurralde commented Feb 21, 2018

There's an ongoing PR that adds support for newer versions: xeipuuv/gojsonschema#180

@asoorm

This comment has been minimized.

Copy link
Member Author

asoorm commented Feb 21, 2018

And to track progress: xeipuuv/gojsonschema#178

@lonelycode

This comment has been minimized.

Copy link
Member

lonelycode commented Feb 21, 2018

I would suggest we go live with what we do have support for so as not to delay the feature and then update the library for a later release in a different issue, since it is also used elsewhere it will have a wider testing surface so will need more time to integrate.

@philsturgeon

This comment has been minimized.

Copy link

philsturgeon commented Feb 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.