Skip to content
This repository was archived by the owner on Nov 2, 2023. It is now read-only.

Determine if default non-compliant behaviour of implementations should be noted #390

Closed
Relequestual opened this issue May 4, 2021 · 25 comments · Fixed by #471
Closed

Comments

@Relequestual
Copy link
Member

A PR to update the draft compliance of opis/json-schema was created: #386

The implementation has a number of non-compliant behaviours by default.
After discussion, feature flags were added to allow users to, when correctly configured, have spec compliant behaviour.

For exmple, the default keyword, by default, would modify the instance BEFORE validation, should a value not be included in the instance at a location. A feature flag was added, but I still feel this is undesierable, significantly enough to warrant recomended we did not update the librareis compliance listing.

So, I now raise the question, should we note implementations that claim compliance, but which are not compliant as far as we are aware?

I'm not suggesting we audit every implementation, but I usually check if a library is using the test suite, and ask questions if they do not.

I'm not suggesting we re-visit every implementation we currently list, but that we do checks when updating compliance.

I'm not suggesting we specifically list the ways in which a library is non-compliant, just that it is non-compliant in some way.

@handrews
Copy link
Contributor

handrews commented May 4, 2021

Integrating the list with test suite results would also be nice, and a more objective way of reporting compliance. Do we have a test that verifies that the instance is unchanged? (EDIT: Is that even possible in our testing framework, and if not should we try to add it somehow?)

@Relequestual
Copy link
Member Author

I don't believe we do, but we COULD by proxy of making the default not valid in lieu of modifying the test suite structures.

@Relequestual
Copy link
Member Author

We do: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/master/tests/draft2020-12/default.json

@Relequestual
Copy link
Member Author

We would have to devise a way to make sure the tests being used are correct, and determine an output of test compliance that we'd look for (if we can even reasonably do that... I don't know what sort of test reporting is possible across multiple languages)

@handrews
Copy link
Contributor

handrews commented May 4, 2021

@Relequestual yeah it's probably a non-trivial undertaking.

@msarca
Copy link

msarca commented May 4, 2021

My 2 cents on this issue: the fact that someone knows how to write a validation schema doesn't mean they know how to use opis/json-schema. If you want to use opis/json-schem you must read the documentation first and learn how to setup the library. You can setup the library in such a way that is fully compliant with the specification. The default setup is not fully compliant, but this was our choice and nobody has the right to dictate to us which features to be enabled by default and which not.

So, to wrap up: opis/json-schema can be setup to be fully compliant, therefore is fully compliant.

@handrews
Copy link
Contributor

handrews commented May 4, 2021

@msarca

nobody has the right to dictate to us which features to be enabled by default and which not.

🤨

This is about "rights" now? We're talking about a spec, not a moral obligation. The spec defines what is and isn't compliant. It is in fact the definition of a specification to dictate what is and is not compliant. That's what it means to have a spec.

If this is ambiguous, it can and should be clarified. If the purpose of a page listing implementations is ambiguous, that can and should be clarified.

At that point, you can choose to meet the criteria or not. Your "rights" are not being impeded regardless of what you choose to do or not do.

@jdesrosiers
Copy link
Member

Generally I'm not too concerned about default configuration. As long as it can be easily configured to be compatible. Although, lately ajv's strict mode is making me think twice about that. I've seen a few times now where people get confused thinking a schema they are trying to use is not valid because ajv's strict mode is throwing errors.

For now, specially for opis/json-schema, I'm not worried about default as long as it can be turned of (I see that it can). The part that was more concerning to me was the use of Relative JSON Pointer. Last I checked, there was no way to turn that off. If that's changed, then I wouldn't be opposed to putting it on the website. However, what I really think opis/json-schema should do is declare their own dialect and do whatever they like. I'd be happy to put it on the website with a custom dialect declared and a note that it's mostly compatible with 2020-12.

@gregsdennis
Copy link
Member

gregsdennis commented May 4, 2021

This is closely related to json-schema-org/JSON-Schema-Test-Suite#314

In this issue I mention a site that someone put together for JSON Path comparisons. I think something like this that covers our test suite would go a long way.

@sorinsarca
Copy link
Contributor

The part that was more concerning to me was the use of Relative JSON Pointer. Last I checked, there was no way to turn that off.

@jdesrosiers that can be turned on/off using the allowRelativeJsonPointerInRef option https://opis.io/json-schema/2.x/php-loader.html#parser-options

@msarca
Copy link

msarca commented May 4, 2021

This is about "rights" now?
At that point, you can choose to meet the criteria or not.

No doubt this is about rights! Your personal opinion is not a valid criterion because is subjective. You believe our default configuration is wrong, but we believe otherwise. In order for you to be able to judge if something is compliant or not you must have a set of objective criteria that can be applied to all. Do you have one?

@handrews
Copy link
Contributor

handrews commented May 4, 2021

@jdesrosiers

Generally I'm not too concerned about default configuration.

I tend to be concerned about this, and in fact the spec has had notes about default configuration for a long time (regarding format, so... meh... but default configuration has long been in the scope of the spec).

The problem is when you have a very large-scale system with different people/companies/products using different implementations. You expect that if everyone installs a nominally-conforming implementation then things will be interoperable to the extent governed by the specification.

Many (most?) people do not read the docs. I don't even mean that they decide not to. I mean that they install X which installs Y which installs Z which installs JSON Schema implementation A, and Y also installs W which installs implementation B, and going in and reconfiguring implementations that you may or may not realize are even present is pretty unlikely.

For the JSON Schema project, we do not want to get a reputation for being unreliably interoperable, or for having an inconsistently implemented ecosystem (that's part of the problem with format- as much as the keyword vexes me, if it was reliably and consistently implemented it would be OK. But it never has been).

However, what I really think opis/json-schema should do is declare their own dialect and do whatever they like. I'd be happy to put it on the website with a custom dialect declared and a note that it's mostly compatible with 2020-12.

Not everything is within the scope of a dialect. Mutating the instance is just not within the scope of regular JSON Schema behavior. It's not a part of any of the keyword classifications. So adding that is going outside the spec more than just adding a vocabulary of keywords that are within the regular scope of JSON Schema functionality would be.


Regardless, we do need to clarify what is required to be on the page, and what has to get called out on the page if we want some leeway in that but also want to give clarity to users.

@handrews
Copy link
Contributor

handrews commented May 4, 2021

@msarca this issue is about resolving that. Please read what I stated about clarifying ambiguity.

I am puzzled by you treating this as some sort of philosophical debate about subjectivity or some sort of attack on your implementation. JSON Schema is a volunteer project. There is far more to do than people to do it. When someone runs up against something that needs to be done, then we look into it. That process is going on right here, with your participation. That's all that this is.

We don't have enough time or people to have everything perfectly sorted, but we are doing our best to work it out visibly, where people can participate.

@handrews
Copy link
Contributor

handrews commented May 4, 2021

Having thought on this more, here are some options that I see, which are not necessarily mutually exclusive:

  • Nail down more implementation behavior and expectations as requirements in the spec. This has the advantage of being clear to everyone (if you aren't looking in the spec, what are you even doing?) but the more we get into implementation behavior the more we have to anticipate the conditions of a range of implementation contexts. We might want to clarify a few things, but this is a big hammer that is often not needed.
  • Require test results and display them on the page. I would be happy with just statically putting the results up- of course someone might then diverge from it, but this is all somewhat best-effort. Requiring live-updated test results is a lot and I don't see much point in requiring it.
  • Allow specifying configuration information on the page, along with test results. This could allow for showing both the default results and the most-compatible-configuration results.

Some of this would require significant redesigning of the page, but TBH we should do that anyway. None of this applies to the non-validation functionality, the vast majority of which is not standardized anyway. We should clarify that distinction.

I personally like the idea of displaying test results alongside configuration information. That offers both clarity and flexibility.

I'm not bothered by putting partially conformant implementations on the page as long as it's very clear what you get if you just go grab the thing and use it as-is. I'm fairly sure we have some partially compliant implementations there now, some with comments, some probably without. I don't remember when I last audited the whole mess, but I tended to err on the side of keeping listings.

Both short-term and long-term, whatever we do should be consistent, so if we have other implementations with similar situations as the implementation in question the we should either take them all off or put this one on. If we have any, they should all be noted clearly while we figure out something better.

I will note that auditing the implementations is a giant pain- I used to do it every draft, mostly because I went around and contacted each project to let them know a new draft was out. We should not really be in the business of doing that as a general rule. Requiring test results in a PR seems reasonable. Of course people could falsify results, but then they're undercutting their own reputation and a.) that's not our problem, and b.) I'm not that worried about people doing it.

@msarca
Copy link

msarca commented May 5, 2021

You are opening issues regarding our library and then you declare I am puzzled by you treating this as some sort of philosophical debate about subjectivity or some sort of attack on your implementation.

Look, guys, if you don't believe that Opis can bring some value to the JSON Schema project, feel free to remove it from your official page.

@gregsdennis
Copy link
Member

@msarca, I understand your frustration with this conversation. It's kind of gone off the rails a bit.

We need to ascertain (internally, but publicly) whether we think it's acceptable (and if so, how) to list on the site an implementation which deviates from the spec by default. There are arguments both ways, but either way, we should be consistent. Much of this is driven from an assumption (which may be incorrect) that most implementations which claim compliance would be set up to have that behavior by default. Your implementation challenges that assumption.

Thank you for your implementation and work toward supporting the ecosystem surrounding this project.

@handrews
Copy link
Contributor

handrews commented May 5, 2021

@msarca I am afraid I was not aware of issues opened against your library, as I did not see a link to one. If I missed it I do apologize - it seems there is backstory here of which I'm not aware. All I'm trying to do here is figure out how to get to a clear set of criteria. It's not a judgement on your library, and this issue is not (to me, anyway) specific to your implementation.

We are always thrilled to see another implementation, particularly one addressing the most recent drafts.

@msarca
Copy link

msarca commented May 5, 2021

Given the fact that our library passes all JSON Schema tests, the only issue left is the default configuration of the validator. We can not change that because of semantic versioning, but we can create a new validator configured by default to be fully compliant.

class CompliantValidator extends Validator
{
    public function __construct(?SchemaLoader $loader = null, int $max_errors = 1)
    {
        parent::__construct($loader, $max_errors);

        $this->parser()
            ->setOption("allowDefaults", false)
            ->setOption("allowRelativeJsonPointerInRef", false)
            // ... and so on
            ;
    }
}

Those who want to be fully compliant with the JSON Schema standard can use that validator without extra configuration. This will ensure good interoperability between different implementations that comply with the standard.

$validator = new CompliantValidator();
$validator->validate($data, $schema);

I believe this is a good solution for settling this issue. Let me know your thoughts on this.

@Relequestual
Copy link
Member Author

You are opening issues regarding our library and then you declare I am puzzled by you treating this as some sort of philosophical debate about subjectivity or some sort of attack on your implementation.

Look, guys, if you don't believe that Opis can bring some value to the JSON Schema project, feel free to remove it from your official page.
@msarca

@msarca @sorinsarca I'm really sorry if you've perceived this issue as an attack against your implementation specifically.

This issue, is not specifically about your library, but the linked PR (#386) gives this issue context.

I want to make a few things really clear here to avoid confusion or further misunderstandings, including that we really appreciate you sticking with us here. I'm going to write as literally as possible, and not imply anything other than what is written. Words are hard.

We are more reactive than proactive

I am puzzled by you treating this as some sort of philosophical debate about subjectivity or some sort of attack on your implementation. JSON Schema is a volunteer project. There is far more to do than people to do it. When someone runs up against something that needs to be done, then we look into it. That process is going on right here, with your participation. That's all that this is.

We don't have enough time or people to have everything perfectly sorted, but we are doing our best to work it out visibly, where people can participate.
@handrews

The issues around the situation that has sparked is a new situation for us. It's a new unique situation we're having to work our way through in the best way we feel we can. I'm sorry that has fallen short of your expectations.

We have no formal policy when it comes to adding implementations to the site. Personally, when adding or updating implementations on the site, I always look to evaluate the use of the test suite. That's my primary indicator as to compliance.

When the test suite isn't used, I suggest it be used before adding the implementation to the site. Usually one of two things happens as a result. Either the response is "Great, we'll add the test suite!" followed later by fixing compliance issues and reporting back, OR we get no response. In this case, the response has been "[We use a modified version of the test suite, and we don't want to change this approach. We are by default partially compliant for various reasons and we don't want to change that right now, but we've made full compliance possible by using the correct config options]"

As this was a new resonse, I felt I needed to consult with the team to guage others feelings on how to proceed. Reactive, rather than proactive. I SHOULD have been clearer about what was happening and why in #386, and for that I apologise. My thought at the time was I didn't want to publically call out a load of issues if no one else agreed with me that there was some.

I'm going to link to here in an issue when we're formalising our Code of Conduct to do our best to avoid that sort of situation again by making expectations clear for all involved.

I do think it's worth mentioning that my invitiation to chat via DM was taken up, and we talked for several hours. I wanted to work through the issues we had without causing any undue concern publically. I think I was worried that calling out several issues publically by one of the editors of the JSON Schema spec could be seen as an attack, and that's the last thing I want.

We greatly value your contribution to JSON Schema and the wider community

I want to reiterate...

...JSON Schema is a volunteer project. There is far more to do than people to do it. When someone runs up against something that needs to be done, then we look into it. That process is going on right here, with your participation. That's all that this is. - @handrews

...

All I'm trying to do here is figure out how to get to a clear set of criteria. It's not a judgement on your library, and this issue is not (to me, anyway) specific to your implementation.
We are always thrilled to see another implementation, particularly one addressing the most recent drafts.
- @handrews

We really value your work with JSON Schema. For many of the team, it's a passion project (and I've been fortunate enough to now go full time), and we get excited to see another project implement new versions of the specification. We've spent countless hours debating, agnonising, over every single slightest change. It's no exaduration to say updated implementations are a thrill to see.

This issue honestly aims to work out how WE as JSON Schema can do better for our community, and by extension your community too.

I really appreciate you both sticking around to discuss this with us, in the open, and although it's clearly a bumpy road, I truly hope we can arrive at a conclusion you feel is fair.

I remember the first time I saw your implementation. I was thrilled to see the lovely site and great docs to boot. Even the most popular implementations can have (what I'd consider) pretty terrible docs. It was refreshing to see such a well looked after implementation.

What does compliance mean?

The default setup is not fully compliant, but this was our choice and nobody has the right to dictate to us which features to be enabled by default and which not.
So, to wrap up: opis/json-schema can be setup to be fully compliant, therefore is fully compliant. - @msarca

We have to be objective here. The default behaviour of an implementation is totally within the remit of the JSON Schema specification. We lay this out as part of the expectations around how we define Dialects and Vocabularies.

We by no means dictate anything to people writing code. Anyone can write and publish whatever code they like.

However, what happens by default is compliance issue, because it's defined in the spec.

... Your personal opinion is not a valid criterion because is subjective. You believe our default configuration is wrong, but we believe otherwise. In order for you to be able to judge if something is compliant or not you must have a set of objective criteria that can be applied to all. Do you have one? - @msarca

I don't believe Henry was expressing a personal opinion here. The test suite is one of the tools we use to judge compliance. That is our objective criteria. (Please let me know if I'm missing something here).

For example, as I mentioned in an above comment, we have tests for checking that the default keyword isn't used to fill in missing fields before validation. Your implementation, under default configuration, will not pass this test.

Please correct me if I'm wrong or you dissagree with anything here (I see you've posted a comment about tests while I was writing this response).

We want to do what we believe is best for the community at large

Your implementation is the first, and only as far as I can see, PHP library which has added support for 2019-09 and 2020-12. This is monumentally important for the PHP community wanting to work with OpenAPI 3.1.

The implementations page on our website serves as the first place many look for libraries to use with JSON Schema, and we often direct people there from various places.

It's clear that we can do more and make it better. We made it better previously by making it easier to jump to specific languages, and draft version support could be easily marked and seen.

As I've said, this is new waters for us, and we can do more to make it as transparant and fair as possible.

What are we going to do about it then?

Right now, we can work on the listing PR.
I feel it would be fair, if you would agree, to add in brackets something like "Fully compliant when correctly configured".
It could link to a page in your docs that explains this.
I'm happy to commit my time to help write the majority of that page (If you'd like me to).

What we can do long term is create a new issue to talk about how we can re-work the implementations listing.
Henry has laid out some ideas above which were roughly the same ideas as I had over breakfast and while getting ready for work this morning (it's hard to mentally put down sometimes).
This longer term work will require non-trivial effort.

I invite you both to help define, oversee, and review this work, should you wish. You're a welcome part of our community!

I know there was a concern raised that someone had personal reasons or some sort of personal vendetta against the specific library or individuals. I want to be clear, I don't believe anyone has any personal interest in denying updates to your library listing. I'm not aware of anyone in our community who has commit access to have any financial interest in any implementations.

We are forming our Code of Conduct, and we welcome input from the whole community. Should we join the OpenJS Foundation (which I feel is likely), then should anyone feel the Code of Conduct review comittee hasn't acted fairly, they can escalate issues to the OpenJS Foundation.

I hope the above aleviates any concerns, but in the absense of our formalised CoC, please reach out to me with any further concerns.

I really hope we can work together on a soultion we all feel is fair given our constraints.

@Relequestual
Copy link
Member Author

Given the fact that our library passes all JSON Schema tests, the only issue left is the default configuration of the validator. We can not change that because of semantic versioning, but we can create a new validator configured by default to be fully compliant.

class CompliantValidator extends Validator
{
    public function __construct(?SchemaLoader $loader = null, int $max_errors = 1)
    {
        parent::__construct($loader, $max_errors);

        $this->parser()
            ->setOption("allowDefaults", false)
            ->setOption("allowRelativeJsonPointerInRef", false)
            // ... and so on
            ;
    }
}

Those who want to be fully compliant with the JSON Schema standard can use that validator without extra configuration. This will ensure good interoperability between different implementations that comply with the standard.

$validator = new CompliantValidator();
$validator->validate($data, $schema);

I believe this is a good solution for settling this issue. Let me know your thoughts on this.

I actually really love this solution.

If you're OK to add it to your quickstart docks page, I think it would negate the need to say anything about compliance on the JSON Schema site listing.

@karenetheridge
Copy link
Member

karenetheridge commented May 6, 2021

Do we have a test that verifies that the instance is unchanged?

I test for such a thing in https://metacpan.org/pod/Test::JSON::Schema::Acceptance (thanks, Relequestual), which is a wrapper around the test suite -- a checksum is taken of the schema and data instance before evaluation, and compared to the same taken afterward.

It might be worth mentioning this in JSON-Schema-Test-Suite (not that anyone reads any docs anyway..) that this is one of the invariants that consumers should be testing, that cannot be expressed in the data files themselves.

@handrews
Copy link
Contributor

PR #464 (about AJV, not opis) is related. If it is accepted I will update any other implementations known to require configuration in order to be compliant, as measured by the test suite (AJV definitely fails the test suite in strict mode).

It might be a good idea to just add a compliant_configuration field to the YAML that just lists the necessary configurations or links to the documentation for the configuration without comment. That way we can avoid weighted language in the notes.

@gregsdennis
Copy link
Member

I think the compliant_configuration field suggestion is fine until we can get some kind of compliance report going (it's planned).

@handrews
Copy link
Contributor

@gregsdennis are you aware of any implementations other than AJV and Opis that require configuration to pass the suite?

@gregsdennis
Copy link
Member

I am not aware of any, but my mental catalog is just starting.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants