json-schema-org / json-schema-spec

The JSON Schema specification
http://json-schema.org/
Other
3.77k stars 266 forks source link

Move "definitions" to core (as "$defs"?) #512

Closed handrews closed 6 years ago

handrews commented 6 years ago

In #505 I commented that definitions fits better into core than validation (and @Anthropic agreed).

The point of definitions is simply to reserve a keyword under which you can safely collect named schemas, without worrying that they will get accidentally interpreted by an extended vocabulary.

definitions also allows packaging multiple schemas in a file for re-use as $ref targets without having to explicitly assign $ids. In that sense, it is most closely associated with $ref and $id, both of which are part of core.

Furthermore, having a designated re-use location has nothing to do with validation or any other particular JSON Schema use case. Reading through the validation spec it seems quite out of place. For the last several drafts, there has even been a specific note in its description that it does not affect validation.

When you have to explain how the keyword has nothing to do with the purpose of the specification, that's probably a sign that the keyword is in the wrong place.

Anyone seriously object to moving this?

erayd commented 6 years ago

Moving this into core sounds like a good idea. Core makes much more sense as a logical home for the definitions keyword, as the keyword itself doesn't actually imply any action - it's simply a well-known location where schema definitions may be stored.

In order to maintain consistency with other core keywords, it seems sensible to rename it to $definitions. Doing so will improve obviousness going forward, and seems unlikely to adversely impact users.

handrews commented 6 years ago

@erayd I suppose there wouldn't be any harm in officially reserving $definitions and perhaps deprecating definitions but noting that extension vocabularies MUST NOT give it different behavior. It's not like references to #/definitions/... would stop working :-P

erayd commented 6 years ago

@handrews That sounds like a good idea.

erayd commented 6 years ago

On a related note, would it also be possible to specify that arbitrary schema fragments SHOULD NOT appear outside of [$]definitions (other than locations already defined by the spec obviously, e.g. under properties), and that implementations MAY treat any schema that is defined in a nonstandard location as equivalent to {}?

The particular use-case I'm thinking of is a validation implementation which compiles a schema definition into procedural logic for direct execution against a document. Any circumstance which results in an inbound remote reference to a nonstandard location would necessarily imply lazy compilation, which adds significant implementation complexity and may be highly undesirable - in such cases, this would allow the implementation to ignore inbound references to a nonstandard location.

handrews commented 6 years ago

@erayd I don't think we should forbid it (although the thought has actually crossed my mind before, so I get your point here). But there's nothing preventing an implementation from saying "if you only put $ref targets under $definitions, your schema will be pre-compiled and fast, and otherwise it may be slow."

I definitely think that reaching into sub-pieces of other schemas (like referencing a particular property schema) is bad form- factor the subschema out to definitions and have both locations reference it. But that's a linter/best practices thing, not a specification thing.

Also, someone writing an extension vocabulary may come up with a good reason to have another location for schemas.

awwright commented 6 years ago

"definitions" is basically the same thing as a comment. I don't see a problem with this.

Anthropic commented 6 years ago

@handrews funny how similar this thread is to what I was discussing with you earlier and I hadn't seen this at that point :)

handrews commented 6 years ago

Sounds like the main question is whether to add the $ prefix or not. @json-schema-org/spec-team ?

I had thought not, but am warming to the idea. It would place $definitions, $id, $ref, and $schema clearly in the same category (while #513 which would move keywords like properties over almost certainly does not need the $ prefix as they are a different sort of keyword and, while important, slightly less fundamental)

epoberezkin commented 6 years ago

Having it in core makes sense. Not so sure about renaming to $definitions. That means that the references should become "#/$definitions/whatever. To be honest, "#/definitions/whatever was quite long already in many cases I used local IDs:

{
  "properties": {
    "foo": {"$ref": "#foo"}
  },
  "definitions": {
    "foo": {
      "$id": "#foo"
    }
  }
}

In any case I don't like renaming to $definitions because refs will become uglier... It does make sense from the consistency point of view though.

Maybe let's move to core to begin with and think longer about renaming. Unlike other keywords with $, definitions does not have any direct effect on the schema processing, it just provides the location for other schemas. It's the argument not to rename.

gregsdennis commented 6 years ago

@handrews: I don't think we should forbid it

I agree with this. Technically someone could have a reference like #/else/not. Why someone would so this is beyond me, but it's still a valid reference to a location within the document.

handrews commented 6 years ago

Further thoughts on naming: As @epoberezkin definitions is already quite long. Would $defs be acceptable? I'm not sure if there is a stylistic / readability concern over a shortened word. We do have const and enum but in the context of programming those are practically normal words anyway.

Then again, Python uses def for defining functions. Not as long of a pedigree as const or enum but not without precedent.

$defs would shorten references for most people and bring it into the $ naming scheme.

I'm not dead set on the $ still but I do lean that way.

erayd commented 6 years ago

I like the idea of $defs. It's both concise and obvious.

erayd commented 6 years ago

If you're worried about obviousness of an abbreviated word, $lib could also be a workable option.

epoberezkin commented 6 years ago

@handrews to be honest, I don't like the look of "#/$", it looks very bash-like. Given that definitions on their own have no impact on validation, they just mark the location in the schema, I'm not sure why we need to rename anything. Users are free to use whatever they choose, really.

semanino commented 6 years ago

I'd like to repeat my comment on issue #505 here:

IMHO, as far as definitions and $ref are concerned, the discussion on how it should be modelled comes down to the point that they try to meet two different aspects:

With current $ref and $schema there's no way do differentiate between these two aspects.

Yet, they are somewhat different:

Basically I agree with @handrews that one should not cross-reference from one schema into others - this results IMHO in an ugly spaghetti thing. Yet, factoring out any and all schema parts, that are used more than once, into a separate file definitely rips apart closely coupled things. I think it's fine to put such building blocks into a "global" location whitin the same schema file - this is what definitions implements. This is the 3rd aspect modelling should consider.

As a result I'd like to suggest the following for consideration (applicable aspect in square brackets):

Pros:

Cons:

Effects on implementation I can't assess because I'm not familiar with it.

Relequestual commented 6 years ago

@semanino Please don't copy paste walls of text which are asking for things not about this issue. Please keep on topic where possible, and reference your comment using a link rather than pasting. Please edit your comment to reflect this. I can see it's slightly different.

Relequestual commented 6 years ago

Decision:

Renaming to shorthand fits with $refs.

awwright commented 6 years ago

I'm a little wary of renaming things if we don't have to. We could keep "definitions" in validation, so that it still exists in that form.

I think this is more impactful than "$id" since this seems to explect people to change their URIs. And of course, Cool URIs Don't Change.

handrews commented 6 years ago

@awwright it's more of a recommendation than a requirement. I think it would go something like this:

Or something like that. We definitely want to avoid the appearance of telling people to reorganize everything Just Because.

I must confess, I hate typing definitions all the time. It makes lines too long and I always misspell it (definitOIns) :-P

handrews commented 6 years ago

This was done as PR #570, but I forgot to close the issue at the time.