provide defaults for all keywords and subschemas that lacked them #1006

karenetheridge · 2020-10-22T22:35:50Z

For vocabulary metaschemas, the default is just 'true'; for
the 'dependentRequired' and 'dependentSchemas' keywords, the default is the
empty object.

awwright · 2020-10-22T22:41:41Z

What's a concrete example of functionality this provides?

My interpretation of "default" is a suitable initial value for the instance when it's created; so at the very least, I think {} may be a preferable value over true. See also #867

karenetheridge · 2020-10-23T17:54:48Z

This is to restore consistency to the metaschemas. In most other places, a default keyword is provided to indicate the intended behaviour when the keyword is absent from a subschema -- this just fills in the places where the default was missing. true is used in other places where the current position is a subschema (which can be a boolean or an object).

karenetheridge · 2020-11-16T21:19:05Z

Is this good to include in draft2020-xx?

karenetheridge · 2020-11-25T22:00:09Z

rebased

gregsdennis · 2020-12-13T04:00:12Z

Hm.. I didn't see this when I did #1057. May have duplication.

Relequestual · 2021-01-20T13:19:07Z

Given that we don't provide defaults in the same schema object which uses a ref or dynamic ref, we should remove that found in the applicator vocab schema on line 17, right?

json-schema-spec/meta/applicator.json

Lines 15 to 18 in 77686de

    
           "additionalProperties": { 
        
               "$dynamicRef": "#meta", 
        
               "default": {} 
        
           },

And for propertyNames?

I think I'll merge this as best I can, then remove the defaults where $dynamicRef is used.

Relequestual · 2021-01-20T13:38:25Z

Can this be rebased one more time please?

For vocabulary metaschemas, the default is just 'true'; for the 'dependentRequired' and 'dependentSchemas' keywords, the default is the empty object.

karenetheridge · 2021-01-20T17:17:45Z

rebased!

meta/applicator.json

gregsdennis · 2021-01-20T20:05:36Z

meta/core.json

@@ -48,5 +48,6 @@
            "type": "string",
            "format": "uri-reference"
        }
-    }
+    },
+    "default": true


I don't agree with having defaults in the roots of the meta-schemas. What's the motivation for this?

I get that the motivation is to avoid putting "default": true everywhere we reference a schema, but the default isn't always "true" for all references.

Traditionally, in our meta-schemas, default has meant something like: if this doesn't exist, the validation result would be the same as if it did exist and had this default value. For example, the default of properties is {}. Therefore, {} has the same result as { "properties": {} }.

So, now consider the not keyword. It references the top-level meta-schema. If the top-level meta-schema has "default": true that says the default of not is true. Therefore, {} has the same result as { "not": true }, which is not correct. That means each time something references the top-level meta-schema it needs to define a default that makes sense in its context.

... or just get rid of default in meta-schemas altogether because it doesn't provide any real value.

@jdesrosiers I think we're arguing the same side of the coin.

Yes, we are. Bottom line is that default doesn't belong at the root of meta-schemas.

So, now consider the not keyword. It references the top-level meta-schema. If the top-level meta-schema has "default": true that says the default of not is true. Therefore, {} has the same result as { "not": true }, which is not correct. That means each time something references the top-level meta-schema it needs to define a default that makes sense in its context.

Or, we can add a default for not -- which should be false. that is, ..., "not": { "$dynamicRef": "#meta", "default": false }, ...

So, how about we remove all uses of default where it's just the obvious thing ({} for objects, [] for arrays, or trueor {} for schemas), and keep them only when they're non-standard -- which is, I believe, just "not", "deprecated", "readOnly", "writeOnly", "uniqueItems", and "minContains".)

If that's amenable to all I can amend this PR.

karenetheridge · 2021-01-21T16:52:14Z

In the end, this is the diff I get against master. is this what we want?

diff --git a/meta/applicator.json b/meta/applicator.json
index ff2cfc2..8652779 100644
--- a/meta/applicator.json
+++ b/meta/applicator.json
@@ -13,8 +13,7 @@
         "items": { "$dynamicRef": "#meta" },
         "contains": { "$dynamicRef": "#meta" },
         "additionalProperties": {
-            "$dynamicRef": "#meta",
-            "default": {}
+            "$dynamicRef": "#meta"
         },
         "properties": {
             "type": "object",
@@ -33,8 +32,7 @@
             "default": {}
         },
         "propertyNames": { 
-            "$dynamicRef": "#meta",
-            "default": {}
+            "$dynamicRef": "#meta"
         },
         "if": { "$dynamicRef": "#meta" },
         "then": { "$dynamicRef": "#meta" },
diff --git a/meta/validation.json b/meta/validation.json
index 606b87b..1432ad8 100644
--- a/meta/validation.json
+++ b/meta/validation.json
@@ -65,7 +65,8 @@
             "type": "object",
             "additionalProperties": {
                 "$ref": "#/$defs/stringArray"
-            }
+            },
+            "default": {}
         }
     },
     "$defs": {

e.g. when evaluating the "not" keyword, the subschema inside it should *not* be true, as that would produce a false result from this keyword

karenetheridge · 2021-01-21T19:36:31Z

Given that we have been using the default keyword wrong anyway (missing keywords cannot produce annotations), maybe something like this is more correct: https://gist.github.com/karenetheridge/1239b063611b937c38713f6ac6a6eaa6

jdesrosiers · 2021-01-21T21:43:17Z

@karenetheridge

missing keywords cannot produce annotations

That has never occurred to me. We would only know the default value if the value exists, but the only reason to care about the default value is if the value doesn't exist.

   "default": {
       "items": true,
       "contains": true,
       "additionalProperties": true,
       "properties": {},
       "patternProperties": {},
       "dependentSchemas": {},
       "propertyNames": true,
       "if": true,
       "then": true,
       "else": true,
       "not": false
   }

If an entire schema is missing, I would expect this default to apply. However, if the schema does exist with some of these values missing, I wouldn't expect this default to those values. The default would be applied all or nothing. So, I don't think this makes sense either.

It seems default is completely useless, even as an annotation.

karenetheridge · 2021-01-21T21:52:07Z

If an entire schema is missing, I would expect this default to apply. However, if the schema does exist with some of these values missing, I wouldn't expect this default to those values. The default would be applied all or nothing. So, I don't think this makes sense either.

Well, the user can do whatever he likes with the annotations he gets back -- it doesn't have to be all or nothing. In some of my code (that is, the caller of the json schema evaluator - not the evaluator itself!), I retrieve that default annotation and add its values into the data instance for every property that is missing.

jdesrosiers · 2021-01-21T21:59:48Z

Here's another approach we might take with where we use default in meta-schemas.

If a keyword isn't present it has no effect. There's no reason to declare a default value that also has no effect. The only reason to have a default value is for keywords that depend on other keywords for their results. In order to process additionalProperties, we need to know the value of properties and patternProperties. Therefore, properties and patternProperties should have a default value. Things like allOf/anyOf/oneOf/not don't need defaults because nothing depends on their presence.

if would be a weird one tho. I'd have to look it up, but I'm pretty sure then and else don't apply if if is not present. So, when you're looking at then, if's default is false. If you're looking a else, if's default true.

karenetheridge · 2021-01-21T22:06:58Z

I think we can leave out properties and patternProperties too -- as the spec says what to do if those keywords are missing (no keyword -> no annotation -> nothing to consume when considering the result of additionalProperties, unevaluatedProperties).

We can leave out if/then/else and not as well -- no keyword -> nothing to do -> no false value injected into the result.

The only outliers really are "minContains", which defaults to 1 when absent, and the meta-data keywords which default to false. We should be able to drop all the rest.

edit: and '$schema', which defaults to the main metaschema $id.

jdesrosiers · 2021-01-21T22:18:09Z

I think properties and patternProperties might be nice to have as a way of documenting what the spec says, but I agree they aren't necessary.

I agree if/then/else don't need defaults and minContains should have one.

Others to consider are

addtionalProperties - true - depended on by unevaluatedProperties
items - true - depended on by unevaluatedItems
prefixItem - [] - depended on by items

I think it would be a nice-to-have to include these defaults, but not necessary.

karenetheridge · 2021-01-21T22:21:45Z

I pushed another commit for consideration, which would be squashed with the other two if acceptable.

…only include those that insert different behaviour than would exist if they were absent

…duce no annotations

jdesrosiers · 2021-01-21T22:34:34Z

meta/core.json

        }
    },
+    "default": {
+        "$schema": "https://json-schema.org/draft/2020-12/schema"


Wouldn't this mean that every schema, including sub-schemas, have a $schema keyword. $schema has no effect in sub-schemas, so this doesn't make sense to me.

Yes, good point. So we'll have to leave it out.. as we have no way currently of specifying in the metaschema "this should apply for the top level schema but not for anything dynamically referenced lower down".

jdesrosiers · 2021-01-21T22:47:03Z

@karenetheridge

the user can do whatever he likes with the annotations he gets back -- it doesn't have to be all or nothing

I agree that default can be used however we want, but I'm not a fan of this approach. It feels awkward and wrong to me. I'd rather use it the way we have been.

Not a hard objection, just my preference. I won't argue if others like it.

awwright · 2021-01-22T01:52:03Z

It seems default is completely useless, even as an annotation.

This is close to my conclusion too, see #867

gregsdennis · 2021-01-22T02:01:03Z

meta/validation.json

+    "default": {
+        "minContains": 1
+    },


I'm not a fan of this format, and I don't think we should encourage it. I think the default value should stay with the property declaration.

gregsdennis · 2021-01-22T02:01:21Z

meta/validation.json

        },
        "maxContains": { "$ref": "#/$defs/nonNegativeInteger" },
        "minContains": {
-            "$ref": "#/$defs/nonNegativeInteger",
-            "default": 1


this should stay

we're okay with using "default" totally contradictory to the spec? If this is here purely for humans, it should be in a $comment.

I don't think this use is contradictory to the spec, because the spec defines default so broadly that there is no wrong way to use it. Since there is no right or wrong way to use it and it doesn't do anything anyway, I'd rather use it in the spirit of how it was intended to be used.

If this is here purely for humans, it should be in a $comment.

I had the same thought at first, but I've come to the conclusion that just because default is not useful as an annotation, doesn't mean it's not useful. Consider that a documentation generator doesn't use instances and therefore doesn't work on annotations as they are defined by the spec. The documentation generator can still make good use of default because the limitations of using it as an annotation don't apply in that context.

I'm in agreement with this. We should consider other applications beyond just being used with an instance, the most obvious, to me, being writing schemas by hand or using UI tools (in terms of adding these for the meta schema).

gregsdennis · 2021-01-22T02:11:34Z

I agree with @jdesrosiers's idea of only including defaults for keywords that have other keywords being dependent upon them.

Going forward, maybe we need to declare the default values of keywords in the context where they're depended upon. For example, the section for contains would define a default for minContains should it be absent, opposed to as it is now where the section for minContains declares its own default. This also has two side effects:

a keyword (or subschema) can have different defaults based on the context in which it's used,
custom keywords can be written (in new vocabs) that utilize keywords with different defaults.

Note that this doesn't give the default keyword any more purpose than for humans.

Relequestual · 2021-01-22T13:23:32Z

In agreement with changes requested by @gregsdennis and @jdesrosiers, including those in
#1006 (comment)

addtionalProperties - true - depended on by unevaluatedProperties
items - true - depended on by unevaluatedItems
prefixItem - [] - depended on by items

karenetheridge · 2021-01-22T17:40:43Z

For example, the section for contains would define a default for minContains should it be absent

I don't see how that can be done with current syntax, because anything we put here would be defined at the .../contains/ position, not .../.

…s that are useful to note for humans.

karenetheridge · 2021-01-22T18:56:00Z

I have pushed one more commit for consideration.

…n sugar.

gregsdennis · 2021-01-23T02:44:53Z

I don't see how that can be done with current syntax...

Yeah, it can't. Was just thinking out loud.

Relequestual · 2021-01-25T12:48:49Z

So the net result of this PR is dependencies gets a default? Was that the intent?

Do we want to handle defaults for prefixItem and items in a separate PR?

karenetheridge · 2021-01-25T19:11:43Z

The net result is that defaults have been removed for all subschemas that recursively-reference #meta (e.g. additionalProperties, propertyNames), and dependencies gets a default. borrrring!

prefixItems already has a default, via the schemaArray definition, and items is another #meta reference.

karenetheridge · 2021-01-25T23:06:04Z

I can squash with a better commit message if we're good with this (or let Relequestual do it).

I trust the discussion

- Remove `default` from all subschemas that recursively-reference #meta - Add `default to `dependencies` Remove, e.g. when evaluating the "not" keyword, the subschema inside it should *not* be true, as that would produce a false result from this keyword.

Relequestual modified the milestone: draft-08-patch1 (draft 2020-NN) Nov 25, 2020

karenetheridge force-pushed the ether/schema-defaults branch from 35325a5 to 8d766a4 Compare November 25, 2020 22:00

Relequestual approved these changes Dec 8, 2020

View reviewed changes

karenetheridge force-pushed the ether/schema-defaults branch from 8d766a4 to 5d21304 Compare December 8, 2020 20:09

Relequestual approved these changes Dec 8, 2020

View reviewed changes

provide defaults for all keywords and subschemas that lacked them

9eef7bf

For vocabulary metaschemas, the default is just 'true'; for the 'dependentRequired' and 'dependentSchemas' keywords, the default is the empty object.

karenetheridge force-pushed the ether/schema-defaults branch from 5d21304 to 9eef7bf Compare January 20, 2021 17:17

remove redundant defaults adjacent to $dynamicRef

7060428

gregsdennis previously requested changes Jan 20, 2021

View reviewed changes

meta/applicator.json Show resolved Hide resolved

gregsdennis reviewed Jan 20, 2021

View reviewed changes

remove misleading/incorrect defaults at top schema level

3ea4c02

e.g. when evaluating the "not" keyword, the subschema inside it should *not* be true, as that would produce a false result from this keyword

squash - final (?) attempt - move defaults to where they belong, and …

a33ef8f

…only include those that insert different behaviour than would exist if they were absent

karenetheridge force-pushed the ether/schema-defaults branch from dc338ab to a33ef8f Compare January 21, 2021 22:22

squash - these defaults do not make sense here - missing keywords pro…

b5ab09f

…duce no annotations

jdesrosiers reviewed Jan 21, 2021

View reviewed changes

gregsdennis reviewed Jan 22, 2021

View reviewed changes

Relequestual self-requested a review January 22, 2021 13:19

Relequestual added this to the draft-08-patch1 (draft 2020-NN) milestone Jan 22, 2021

squash. remove defaults in the wrong place; add $comments for default…

5553c05

…s that are useful to note for humans.

squash - okay, one last time. keep defaults at property level as huma…

677f20a

…n sugar.

Relequestual approved these changes Jan 25, 2021

View reviewed changes

Relequestual requested a review from gregsdennis January 25, 2021 22:53

Relequestual merged commit 237c078 into json-schema-org:master Jan 27, 2021

karenetheridge deleted the ether/schema-defaults branch February 20, 2021 20:54

provide defaults for all keywords and subschemas that lacked them #1006

provide defaults for all keywords and subschemas that lacked them #1006

Conversation

karenetheridge commented Oct 22, 2020

awwright commented Oct 22, 2020

karenetheridge commented Oct 23, 2020 • edited Loading

karenetheridge commented Nov 16, 2020

karenetheridge commented Nov 25, 2020

gregsdennis commented Dec 13, 2020

Relequestual commented Jan 20, 2021 • edited Loading

Relequestual commented Jan 20, 2021

karenetheridge commented Jan 20, 2021

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

karenetheridge Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

karenetheridge commented Jan 21, 2021

karenetheridge commented Jan 21, 2021

jdesrosiers commented Jan 21, 2021

karenetheridge commented Jan 21, 2021

jdesrosiers commented Jan 21, 2021

karenetheridge commented Jan 21, 2021 • edited Loading

jdesrosiers commented Jan 21, 2021

karenetheridge commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdesrosiers commented Jan 21, 2021

awwright commented Jan 22, 2021

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

gregsdennis commented Jan 22, 2021

Relequestual commented Jan 22, 2021

karenetheridge commented Jan 22, 2021 • edited Loading

karenetheridge commented Jan 22, 2021

gregsdennis commented Jan 23, 2021

Relequestual commented Jan 25, 2021

karenetheridge commented Jan 25, 2021

karenetheridge commented Jan 25, 2021 • edited Loading

karenetheridge commented Oct 23, 2020 •

edited

Loading

Relequestual commented Jan 20, 2021 •

edited

Loading

karenetheridge Jan 21, 2021 •

edited

Loading

karenetheridge commented Jan 21, 2021 •

edited

Loading

karenetheridge commented Jan 22, 2021 •

edited

Loading

karenetheridge commented Jan 25, 2021 •

edited

Loading