Adds JSON Validation Middleware, fixes #1163

[lonelycode]

Fixes #1163

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": "BASE64 ENCODED SCHEMA"
				}]
			}
		}
	}
},

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.

[mvdan]

a few nits, otherwise LGTM

[mvdan]

Why not allow this to be a file path too, like we do with others?

If you're only going to allow a base64 string, you can use []byte as a type and encoding/json already uses base64 implicitly.

[mvdan]

?

[mvdan]

please don't say that this is a base64-encoded schema :) this should be in plaintext, and encoded as part of the test to simplify the code and help readability/maintainability.

[buger]

I understand why you decided doing it base64 (in order to avoid problems with mongo), but it is terrible UX for the users both community and, what most important, people use use API. I would say that valid solution to this trouble, is to re-architect our mongo API storage, and store whole definition as json string: there are no use-cases which needed to quering api difinition internal fields via mongo queries, except api_id, api_name, or org_id fields, which can be denormalized into separate fields. As alternative, we can add small layer to Dashboard Mongo storage, which ensures that some of the keys will be serialized to base64 on write, and deserialized on read: but all this stuff will be hidden from user and UI, and when user retrieve or put data via API, it will be simple JSON for him.

Also, I think OpenAPI (swagger), does the better job of supporting json schema, see examples here http://bigstickcarpet.com/swagger-parser/www/index.html. In addition to body validation, it also validates URL params. If we going to improve our support of OpenAPI, I think it makes sense follow similar patterns.

To recap:

• Move base64 logic to dashboard side, and keep everything in json
• Rename validate_with to smth like body_schema

[lonelycode]

understand why you decided doing it base64 (in order to avoid problems with mongo), but it is terrible UX for the users both community and, what most important, people use use API

I am following the convention that has been used for large blob objects in Tyk API definitions since forever, things that are stored as b64 objects in CE and dash:

• Virtual path functions
• Middleware code (it does not need to be a file)
• Schemas
• Version names

There is precedent.

would say that valid solution to this trouble, is to re-architect our mongo API storage, and store whole definition as json string

Putting JSON into a file reference makes sense to me, but translating that to a document store sounds like an anti-pattern to me.

And in the dashboard, the reference is an ObjectID? So now need to keep those links in place though code? Mongo is not a relational DB, take a look at the clusterf*ck that is webhook storage to see why that might be a bad idea.

Also, just to clarify, instead of adding a feature that is a no brainer you want to re-architect our mongo DB storage I'll leave that never-ending task to you.

Also, I think OpenAPI (swagger), does the better job of supporting json schema, see examples here http://bigstickcarpet.com/swagger-parser/www/index.html. In addition to body validation, it also validates URL params. If we going to improve our support of OpenAPI, I think it makes sense follow similar patterns.

So we can't add a feature and improve it incrementally? Well, better throw the whole thing in the trash.

To recap:

Move base64 logic to dashboard side, and keep everything in json

I agree with the file based approach for CE, though docker users will hate having to add more files to their setups. The dashboard side of this particular approach can be your problem because I am not writing SQL-glue code again from Mongo, it's a document store.

[lonelycode]

@buger Over to you.

[mvdan]

I think the two of you have a point. I agree that from the gateway point of view, base64 is ugly and confusing. But I have to agree with Martin that we can't change all of this now (wink wink 3.0?), and we should not halt a new feature that simply continues the current practice.

If we want to redesign this at any point like 3.0, porting 4 models will be about as easy as porting 5.

[buger]

Well, I'm not sure why this PR was closed, I definitely not meant this.

Points described by me here is a standard code review, and my responsibility is to ensure that everything will be done the right way. My point was to raise these important topics and discuss them.

I think the main difference in our thinking here is that you originally treat schema as a blob, and I as a part of API definition, similar to how Swagger does it: schemas have quite different nature then middleware and virtual path.

It is not "no brainer" feature, because it touches a lot of people in our team and a lot of developers who will use it. This feature is VERY useful, and potentially can be used A LOT. So thinking about developer user experience in this particular case is a top priority for me. Moreover, it appeared without any planning and out of milestone.

We can definitely start with something small, and continuously improve it, but we need to choose the right defaults. I think no one argues that showing base64 fields inside API definitions is a bad smell (but storing is ok), and at time it was added it was ok. But looking at the future, we can do better.

If we think about future of this feature in terms of UI, I would say that at the start it probably should be the raw text field, with JSON syntax highlighting, but in future, it would be nice to have visual editor to control each attribute and its type.

Regarding mongo data storage format, I proposed 2 different solutions. I agree that going with storing the whole API as JSON string would be too much. But writing some glue code to deal with internal complexity, is totally ok. Moreover, we already have code on the dashboard which handles json -> base64 encoding and decoding for "Versions" field. So we already do it like this.

// Clean will URL encode map[string]struct variables for saving
func (a *APIDefinition) EncodeForDB() {
	new_version := make(map[string]VersionInfo)
	for k, v := range a.VersionData.Versions {
		newK := base64.StdEncoding.EncodeToString([]byte(k))
		v.Name = newK
		new_version[newK] = v
	}

	a.VersionData.Versions = new_version
}

func (a *APIDefinition) DecodeFromDB() {
	new_version := make(map[string]VersionInfo)
	for k, v := range a.VersionData.Versions {
		newK, err := base64.StdEncoding.DecodeString(k)
		if err != nil {
			log.Error("Couldn't Decode, leaving as it may be legacy...")
			new_version[k] = v
		} else {
			v.Name = string(newK)
			new_version[string(newK)] = v
		}
	}

	a.VersionData.Versions = new_version
}

In case of this task, it will just mean addition simple additional logic for schema field.

I'm going to re-open this request.

[lonelycode]

Well, I'm not sure why this PR was closed, I definitely not meant this.

It was closed because it's now your problem, this was a tactical PR, it is now a dashboard, mongo and schema refactor ven though it's just a CE feature. It has gone from small, to enormous.

Points described by me here is a standard code review, and my responsibility is to ensure that everything will be done the right way. My point was to raise these important topics and discuss them.

Not really, a standard code review does not suggest refactoring a whole storage engine in another project. That would be done in a major release planning phase.

It is not "no brainer" feature, because it touches a lot of people in our team and a lot of developers who will use it. This feature is VERY useful, and potentially can be used A LOT. So thinking about developer user experience in this particular case is a top priority for me. Moreover, it appeared without any planning and out of milestone.

"No brainer" means we should implement it because it is obvious that it needs to be done, and that it is a powerful feature, and that we should KISS.

If we think about future of this feature in terms of UI, I would say that at the start it probably should be the raw text field, with JSON syntax highlighting, but in future, it would be nice to have visual editor to control each attribute and its type.

Why are you thinking about UI? This PR is for the core feature in the gateway. Cart before horse, again.

[buger]

Closing in favor of #1343