voxpupuli / json-schema

Ruby JSON Schema Validator
MIT License
1.54k stars 243 forks source link

Incorrect behavior of strict vs. default modes #103

Open pdlug opened 10 years ago

pdlug commented 10 years ago

The behavior of strict mode is a bit misleading (and incorrect IMHO). JSON schema provides required already but in the default execution of the json-schema validator it ignores this property. The expected behavior is that default validation checks that any keys present in the document being validated match the definition in the schema and that all keys listed as required in the schema are present. Strict mode would then make a bit more sense to change all keys to required though I'd argue that's a bit misleading as well and strict mode should really just imply that there are no keys present in the validation target that are not defined in the schema as well. Specifying that all keys present in the schema are required really should be a separate option since this is a different validation case (perhaps require_all: true?).

In summary I propose:

Default validation:

Strict validation:

Require all:

I'm happy to take a stab at a patch if I can get some consensus that this is the right thing to do.

pd commented 10 years ago

It's unclear to me exactly why strict mode exists.

Since at least draft-02, there has been an additionalProperties schema property that:

As for required, this test seems to suggest it works without strict mode: https://github.com/hoxworth/json-schema/blob/756ec68/test/test_jsonschema_draft4.rb#L195-L220

(There's a similar test for draft3)

Given additionalProperties and required properties, I think you should be able to get everything you want without an additional mode -- even without strict mode!

hoxworth commented 10 years ago

additionalProperties and required definitely allows one to write really strict schemas. The desire for the strict option, however, was initially to allow easy re-use of lenient pre-existing schemas in a strict way without requiring extending the schema. The side effect is that the library begins to intrude slightly on the domain of the schema language itself, possibly encouraging users to rely on validator features rather than schema language features.

I wouldn't mind adding a new require_all option as a complement to strict, but this issue brings up a good point that we should figure out a way to be slightly careful about how we present options that validate schemas in non-spec ways.

ninkibah commented 9 years ago

I hope this pull request is useful. I implemented pdlug's suggestion of a require_all option. It defaults to true, so keeping the current strict behavior. I turned it to false in my project, and now only fields that are listed in required are actually required.

My pull request only implements this for the draft4 validator. I was not sure if it made sense to do it for the other validators.

iainbeeston commented 9 years ago

I'm going to throw in a crazy idea - maybe we should try to limit json-schema to validation, as it appears in the spec. Keep it lean and keep to the spec. It seems like inserting defaults, optional formatters, and even strict mode are all non-standard extensions and all have lead to headaches (in varying degrees). So far as I can tell, both require_all and strict mode basically override the way that json-schema validation works (ie. according to the spec). So far as I can tell, you can achieve all of this just by adding additionalProperties: false, and by doing that your schema will work identically in other validators as well.

I don't mean to oppose this feature in particular, but I feel like there's a broad pattern emerging between a number of features like this.

ninkibah commented 9 years ago

Iain, do you mean to tell me that if I had just added additionalProperties: false to my schemas, that I could have validated my data without using strict mode?

But, yes, I would be for the default validation following the spec, without all these bells and whistles.

iainbeeston commented 9 years ago

So far as I can tell, strict mode is equivalent to setting additionalProperties: false on every property in your schema, and require all is equivalent to required: true and additionalProperties: false on every property in your schema (and my argument is that using required and additionalProperties is better because it works in every json schema validator, not just this one).

The way the json-schema gem works right now (without strict mode) is correct - the json schema spec only validates properties in the data that have the same key as properties defined in your schema (unless you use additionalProperties or required).


Correction: you specify additionalProperties on objects (not on individual properties)

RST-J commented 9 years ago

As I understood, require_all would only require each property in the schema to be present but not forbid additional ones (as strict mode would).

However, I'm very with @iainbeeston here. The problem is not that I am against fancy features, but first rule should be if json-schema is given a schema and some data it should behave according to the spec. For any additional things I'd propose two rules:

If we decide to add a require_all option then it should not be turned on by default in my opinion.

To repeat and emphasise my opinion: People that just want to do JSON-Schema validation should add json-schema as dependency throw in schemas and data and get what they would get in any other language/library adhering to the spec.

Actually this could affect $ref resolution/fetching (#152). If the spec says, validators should load refs by default, then json-schema maybe should do that by default and security bonuses are opt-in!?

iainbeeston commented 9 years ago

Yeah, if that's what's in the spec, then I think we should adhere to it... Even if we disagree (but in that case definitely have an opt-in flag)

tristil commented 9 years ago

FWIW there's a thoughtbot post where they use strict: true so if people copy that matcher they may lose some time trying to figure out why for example the parser isn't respecting required lists, etc. I don't really get what strict is for either, it ends up just distorting the schema you've specified.

freiden commented 9 years ago

Sorry for being late to the party but is strict option is still needed since behaviors are defined in schemas?

RST-J commented 9 years ago

@freiden What exactly do you mean? Strict is a makro so to say which prohibits any properties not covered by the schema (default in JSON schema is to accept any properties about which nothing is stated in a schema) and at the same time make every property required which is listed in a schema (properties not explicitly stated as required can be missing by default).

It is not necessary to have this as one can achieve this by hand. However, it was introduced as shortcut so one has not to do this tedious steps to convert relaxed schemas to strict ones, iirc.

freiden commented 9 years ago

If I understand well the purpose is to validate schemas based on the specfication, so validations of fields should be managed by the schema to prevent unwanted behavior in context where you doesn't use this gem for validations. For example, I'm using the gem to validate my schema during tests but in my API I don't need the validation when sending my data the rest of the time.

cec commented 9 years ago

@iainbeeston correct me if I'm wrong, but as stated here under the base schema section, additionalProperties is specified at the object level, not for every property; it would be a nightmare otherwise. The same is stated in the Understanding JSON Schema "web book" (I know there's a better name for this...)

iainbeeston commented 9 years ago

@cec That's correct. If you're referring to my comment above - that's a typo (should have said for every object, or maybe "for every property")

cec commented 9 years ago

@iainbeeston I'd say for every object. It's a very nice property anyway :)

cec commented 8 years ago

@iainbeeston @RST-J I recently began a new project where an extensive use of combining schemas is required and this made me realise that @pdlug proposal is definitely necessary. Allow me to explain.

We use JSON Schema to describe responses of our APIs endpoints, similar to specifying a protocol. When doing so, it is of utmost importance to be able to programmatically ensure that API clients do not receive unexpected data.

"additionalProperties" : false guarantees that the protocol is not violated unintentionally when changed the API.

However, specifying this prevents from combining/extending schemas using allOf (see this issue on the json-schema repo).

The "ban unknown props mode" discussed on the referenced issue and in the proposal for draft 5 represents the change proposed by @pdlug and by @pkarman in #295 .

I think efforts should be made to implement and merge this because of the following reasons:

I apologize for the long post and look forward to your comments.

Haelle commented 4 years ago

I was thinking of a slightly different idea ; When strict: true offer the possibility to the users to "override" the default behavior enforced by the strict mode

Example : with strict: true all property are by default "required": true ; but if the user set "optional": true or "required": false if override the behavior. The same if the user set "additionalProperties": true

I could try and dig in to the code to see if I can make a PR out of this idea

WDYT ?

AlexandreBernard commented 1 year ago

I need exactly the same behavior as you do @Haelle

I want strict validations because I want to validate that a json matches the schema and I don't trust the ones who are going to be adding fields to that JSON. So strict validation is great as it enforces everything to be defined in the schema

However, an optional attribute must be possible even in strict mode if defined in the schema. So if in the schema we have a required array that does not contain the attribute it should not be required even in strict mode.

optional: true does make sense but in non json schema standard.

required: true/false leads to problems as required should be a list of properties, not a boolean

elik-ru commented 3 months ago

Hey people! Do i get it correctly that 10 year later there is sill no solution? Is there any objection against respecting "required": false in strict mode?

elik-ru commented 3 months ago

There is was a PR that was doing exactly this, but it did not receive any comments: https://github.com/voxpupuli/json-schema/pull/406 and closed by author because of silence.

Drowze commented 2 months ago

I've been following this issue for far too long... Hope this gets fixed someday. As of now it is simply not possible to define optional properties when using the strict mode.

For a change, this is what I'd expect under strict mode:

  1. When the schema defines required, the validator should respect it.
    
    // the validator should respect the "required" definition and allow "last_name" to be optional
    {
    "type": "object",
    "properties": {
    "name": { "type": "string" },
    "last_name": { "type": "string" }
    },
    "required": ["name"]
    }

// the validator should respect the "required" definition and allow any defined property to be optional: { "type": "object", "properties": { "name": { "type": "string" }, "last_name": { "type": "string" } }, "required": [] }


2. When the schema does not define `required`, the validator should treat all defined properties as required.
```jsonc
// the validator should treat all defined properties as required
{
  "type": "object",
  "properties": {
    "name": { "type": "string" },
    "last_name": { "type": "string" }
  }
}
braindeaf commented 2 months ago

I encountered a problem this week. It was using RSwag / RSpec to test API responses against out swagger schema. We use strict mode, and this is handy I hadn't written the schema properly but I knew the response was correct. I worked my way through every error to get the swagger schema I wanted.

Next step, we use openapi-typescript to generate Typescript from our swagger.json. Problem is in JSON-Schema we use strict and there is no equivalent in openapi-typescript and every single property is marked as optional by default. So then I have to go in and marked every property as required anyway just so the Typescript is correct. All of this because as I understand it the standard does not support a strict mode. So really is the answer to this not to have a strict mode at all and adhere to the JSON Schema standard and be done with it.

In order to be happy that the schema I am testing in RSwag is going to eventually generate the Typescript we need then we should write our specs without strict mode.

Just my two cents after two days looking into this (not 10 years 😸 )