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

Invalid validation of allOf combined with additionalProperties: false #101

Closed
tlj opened this issue Jun 6, 2019 · 3 comments
Closed

Invalid validation of allOf combined with additionalProperties: false #101

tlj opened this issue Jun 6, 2019 · 3 comments

Comments

@tlj
Copy link

tlj commented Jun 6, 2019

When validating a value against an allOf schema the validator will do them one by one and if the value validates against all schemas, it passes. However, if additonalProperties: false is set on one of the refs in allOf, then a valid object will fail if it has properties which is defined in one of the other refs, but not the one with additionalProperties: false.

I don't believe this is according to spec.

I've created a test for this which might explain this better:

{
	Title: "ALL OF - additionalProperties: false",
	Schema: &openapi3.Schema{
		AllOf: []*openapi3.SchemaRef{
			{
				Value: &openapi3.Schema{
					Type: "object",
					AdditionalPropertiesAllowed: openapi3.BoolPtr(false),
					Properties: map[string]*openapi3.SchemaRef{
						"pet_type": {
							Value: &openapi3.Schema{
								Type: "string",
							},
						},
					},
				},
			},
			{
				Value: &openapi3.Schema{
					Type: "object",
					AdditionalPropertiesAllowed: openapi3.BoolPtr(false),
					Properties: map[string]*openapi3.SchemaRef{
						"bark": {
							Value: &openapi3.Schema{
								Type: "boolean",
							},
						},
						"breed": {
							Value: &openapi3.Schema{
								Type: "string",
							},
						},
					},
				},
			},
		},
	},
	Serialization: map[string]interface{}{
		"allOf": []interface{}{
			map[string]interface{}{
				"type":                 "object",
				"additionalProperties": false,
				"properties": map[string]interface{}{
					"pet_type": map[string]interface{}{
						"type": "string",
					},
				},
			},
			map[string]interface{}{
				"type":                 "object",
				"additionalProperties": false,
				"properties": map[string]interface{}{
					"bark": map[string]interface{}{
						"type": "boolean",
					},
					"breed": map[string]interface{}{
						"type": "string",
					},
				},
			},
		},
	},
	AllValid: []interface{}{
		map[string]interface{}{
			"pet_type": "Dog",
			"bark": true,
			"breed": "Dingo",
		},
	},
	AllInvalid: []interface{}{
		map[string]interface{}{
			"pet_type": "Dog",
			"pet_name": "Charlie",
		},
	},
},

I'm unsure how this should be solved so I haven't created an MR for this. I think the only way to solve this properly is to resolve the allOf into a schema by merging the refs.

@tlj tlj changed the title Invalid validation of allOf combined with adittionalProperties: false Invalid validation of allOf combined with additionalProperties: false Jun 6, 2019
@fenollp
Copy link
Collaborator

fenollp commented Jun 6, 2019

This sounds according to spec to me. Did you have a look at https://json-schema.org/understanding-json-schema/reference/object.html?
Schemas with additionalProperties: false are notoriously not mixable with such combinators.

json-schema/json-schema#116

crux is: you either need anyOf or if-then-else or maybe extends if that's still a thing, but most maintainable is probably to generate that schema from parts.

cc but unrelated: #82

If you agree with this please close the issue :)

@tlj
Copy link
Author

tlj commented Jun 6, 2019

Thanks, this is really interesting.

I would think that as long as Swagger UI (and others) represent the model as an independent and fully resolved mode - not one consisting of several independent parts - I was expecting validation to also happen on the fully resolved schema.

Looking at https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/ it says using allOf is the way to extend objects, but then it goes on to say "When validating the data, servers and clients will validate the combined model against each model it consists of."

So it sounds like you're right, so I'll close the issue. I don't think this should be the correct behavior though since it makes it pretty much impossible to extend objects and still use their base object while doing strict additionalProperty:false checking.

Luckily I am already generating my specs in code, so I'll just resolve any allOf instances when I render the spec to file.

Thanks for your time! :)

@tlj tlj closed this as completed Jun 6, 2019
@fenollp
Copy link
Collaborator

fenollp commented Jun 6, 2019

Agreed, the JSON Schema Spec is lacking in this area.

If your allOf.s are trying to sum properties you can work around this and generate schemas where these properties are merged and you’ll them be able to specify that schema with additionalProperties=false. I’d suggest making the values of these properties named schemas themselves and $ref.erence them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants