Skip to content

TSC Vote - The future of format #19

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

Closed
gregsdennis opened this issue Nov 2, 2024 · 19 comments
Closed

TSC Vote - The future of format #19

gregsdennis opened this issue Nov 2, 2024 · 19 comments

Comments

@gregsdennis
Copy link
Member

gregsdennis commented Nov 2, 2024

json-schema-org/json-schema-spec#1520 describes the current state of the format keyword and proposes a change that will technically be breaking but should also align the keyword's behavior with user expectation.

In summary:

  • format is an assertion, always.
  • Implementations SHOULD support format with the defined values.
  • Implementations MAY support others, but only by explicit config.
  • Implementations MUST refuse to process a schema that contains an unsupported format.

I have created PR json-schema-org/json-schema-spec#1553 to reflect these changes, but the TSC should vote on this change.

  • 👍 In favor of this change as-is
  • 😕 In favor but with a modification (please elaborate in a comment)
  • 👎 Not in favor at all
@Julian
Copy link
Member

Julian commented Nov 2, 2024

I am -1 personally on all backwards incompatible changes, but curious to see what others' opinions are.

@mwadams
Copy link

mwadams commented Nov 2, 2024

I'm really struggling to choose between strict backwards compatibility (ie 👎), an explicit acknowledgement of the opt-in-to-backwards compatibility (😕 and something like Implementations MUST support annotations for unknown formats by explicit config only) or the devil-may-care 👍

Thinking about a user base which is already splintered between the "format is asserted" and "format is annotated" drafts vocabularies, I am tending towards 😕 or 👍 and will tentatively vote 😕 - implementations MUST have a back-compat mode - with no real conviction!

(This leads me to some thoughts about the difference between implementation config and vocabs and whether this should all be revisited when vocabs are nailed down).

@gregsdennis
Copy link
Member Author

Thinking about a user base which is already splintered between the "format is asserted" and "format is annotated" drafts... - @mwadams

There are no "format is asserted" drafts. format has always been an annotation.

@mwadams
Copy link

mwadams commented Nov 2, 2024

Thinking about a user base which is already splintered between the "format is asserted" and "format is annotated" drafts... - @mwadams

There are no "format is asserted" drafts. format has always been an annotation.

Apologies, I'm confusing the optional tests with the specs again! [Edited above to refer to vocabularies for posterity]

@jviotti
Copy link
Member

jviotti commented Nov 2, 2024

I don't like breaking changes either, but I agree with @gregsdennis that users do expect this. It is such a common source of confusion that I'm 👍 for fixing it

@jdesrosiers
Copy link
Member

Considering the compatibility guarantees that we're going to make going forward from the next release, we can no longer allow a "best effort" validation for format. It has to be strict validation or there isn't compatibility between implementations let alone versions of the spec. Then in order to allow for forward compatibility of the spec, we need to raise an error on unknown formats. If unknown formats are ignored, we would never be able to add a new format because it would be incompatible with the previous spec behavior of being ignored.

So, a backward incompatible change is required no matter what we do. The validation behavior of format either needs to change as proposed or needs to be removed altogether. I think it's safe to assume no one wants to completely remove validation behavior, so what remains is whether we want to retain annotation-only behavior and what we want to call the keyword(s).

My preference is that we introduce a new keyword (format has too much baggage) and make it strict validation only. I don't think we need an annotation-only keyword. People can use x- keywords for formats they want to declare, but not validate.

So, I think this change needs to happen, but my vote is 😕 for two reasons. First, I would like us to consider a rename. It would not be a blocker for me if everyone wants to keep the name and we can always make the change and rename in a later step, so it doesn't have to hold up this change. The second change I'd like to see is that the PR still has the "best effort" language and I think that needs to be more strict.

@gregsdennis
Copy link
Member Author

I don't think we need an annotation-only keyword.

If we introduce a new keyword, I think we still need to keep format, even if it's included as deprecated. I know that Henry in particular relies on the annotation output of this keyword, and where there's one...

@jdesrosiers
Copy link
Member

If we introduce a new keyword, I think we still need to keep format, even if it's included as deprecated

Then we wouldn't be addressing the user expectation problem. I think that's something worth addressing, which means our only options are change format or replace it. Replacing format would make the upgrade path a little more complicated, but I think jettisoning format's baggage is worth it. A simple rename shouldn't be too difficult for users to deal with.

I know that Henry in particular relies on the annotation output of this keyword, and where there's one...

I probably shouldn't have said "validation-only". I expect the keyword will still emit annotations as well as validate. I'm not sure if there was a miscommunication there or not, so I wanted to clarify. The annotation output wouldn't go away, it would just be under a different name. Migrating to the new name is just part of updating for the stable release. Everyone will need to do it regardless of whether they use annotation output or not.

@karenetheridge
Copy link
Member

+1 on this proposal as it is (barring any minor wording tweaking), using the existing keyword name.

My reading of this is that this is exactly the behaviour when using the format-assertion vocabulary in draft2020-12 with a value of true: there are a list of formats to be supported, and if any of them are not supported then the schema cannot be processed (this is different from generating a validation error, which might be unseen or turn into a true validation if below an anyOf, if/then/else or not keyword, etc).

@gregsdennis
Copy link
Member Author

The second change I'd like to see is that the PR still has the "best effort" language and I think that needs to be more strict. - @jdesrosiers

I've removed the permissive language from the PR. Every format does list a spec, so that should be defined enough. (I wonder if we can borrow from any test suites those specs may have.)

@Relequestual
Copy link
Member

I'd usually not be in favour of backwards incompatible changes, however, this change would be inline with user expectations. So I'm in favour with similar reservations as Jason.

My key principal of thinking is reducing surprises.

@benjagm
Copy link
Contributor

benjagm commented Nov 9, 2024

If that is okay, I would prefer not to vote in this case.

@gregsdennis
Copy link
Member Author

@Relequestual @mwadams @jdesrosiers

I prefer to keep the existing keyword.

  • People are already familiar with it
  • People expect it to validate
  • It's a good, descriptive name for what it does

I have, however, removed the permissive language from the PR (linked just below the opening comment). Please let me know if that changes your vote, or if further change is needed.

@mwadams
Copy link

mwadams commented Nov 9, 2024

I like that change

@jdesrosiers
Copy link
Member

I have, however, removed the permissive language from the PR (linked just below the opening comment). Please let me know if that changes your vote, or if further change is needed.

I haven't reviewed the change yet, but yes, that was the blocker for me. I'm changing my vote.

@Relequestual
Copy link
Member

I have, however, removed the permissive language from the PR (linked just below the opening comment). Please let me know if that changes your vote, or if further change is needed.

I can see some changes, but I feel like this is an opportunity to be less lax.

I'm veering into PR review territory here, but for example...

SHOULD provide validation for each format attribute defined in this
document;

https://github.com/json-schema-org/json-schema-spec/pull/1553/files#diff-a2feaccf762dd55c4d3dd625aa1b57c48df6f782b8088cd4210fc9b93f8aea17R330

Still says SHOULD. If we think about strictness and tests, that still makes the tests optional as opposed to required.

As such, I don't feel the spec changes currently match up with what's in the ADR...

Due to this consistency in user expectations have decided to:

  1. make format an assertion keyword and strictly,
  2. enforce it by moving the appropriate tests into the required section of the Test Suite.

https://github.com/json-schema-org/json-schema-spec/pull/1553/files#diff-5ecc58afa33e0f51d32d6d09c4d548b82ae251b1f31887264d5559cb845bbdacR26-R29

I appreciate that this may be in relation to Regex and ECMA-262 compliance. If that's the reason for SHOULD as opposed to MUST, I feel like we should strengthen requirements as much as possible.

@gregsdennis
Copy link
Member Author

gregsdennis commented Nov 11, 2024

@Relequestual I'm happy for these comments to be in the PR. I'm just looking for general vibe here.

Just quickly, though, I still have that the implementations SHOULD implement the all of the formats in the spec, but they MUST refuse to process a schema that contains a format they don't fully support. This requires full support or no support for a given format, but they get to choose which formats to support.

Regex still needs work, and I'd like to revisit the possibility of moving from ECMA-262 to i-regexp, but I'll raise that separately.

@gregsdennis
Copy link
Member Author

gregsdennis commented Nov 13, 2024

I'm calling this vote. Thanks everyone for your thoughts. We will move forward with json-schema-org/json-schema-spec#1553.

👍 - 5
😕 - 1
👎 - 1

@Relequestual would you please transfer your thoughts there?

@Relequestual
Copy link
Member

The updates made since I last viewed the RP remove the concerns I had, but I'll comment over on the PR all the same, even though it's closed, as you asked for it =]

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

9 participants