Skip to content
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

Unhelpful validation error message #64

Closed
kd-ods opened this issue Nov 24, 2020 · 11 comments
Closed

Unhelpful validation error message #64

kd-ods opened this issue Nov 24, 2020 · 11 comments

Comments

@kd-ods
Copy link
Collaborator

kd-ods commented Nov 24, 2020

(I hope this is the right repo for this issue.) Try running this json through the data review tool:

[ { "annotations": [ { "motivation": "correcting" } ], "addresses": [ { "country": "EE", "type": "residence" } ], "birthDate": "1985-02", "identifiers": [ { "id": "280871-*****", "schemeName": "GTR-PASSPORT" } ], "isComponent": false, "names": [ { "familyName": "Nonymou", "fullName": "A Nonymous", "givenName": "A", "type": "individual" } ], "nationalities": [ { "code": "GB" } ], "personType": "knownPerson", "publicationDetails": { "bodsVersion": "0.2", "license": "http://creativecommons.org/publicdomain/zero/1.0/", "publicationDate": "2020-11-10", "publisher": { "name": "Test Register", "url": "http://www.test.com" } }, "source": { "assertedBy": [ { "name": "Me" } ], "type": [ "selfDeclaration", "officialRegister" ] }, "statementDate": "2020-10-30", "statementID": "d5764567474-d2a5-4e89-be41-614crgdtgsdgsgsfg", "statementType": "personStatement" } ]

It throws the error: '{'motivation': 'correcting'} is not valid under any of the given schemas'. This does not help with troubleshooting. A better message would be:

'{'motivation': 'correcting'} is not valid according to the BODS schema. See https://github.com/openownership/data-standard/tree/master/schema.

Or if we could link to the relevant place in the BODS schema that would be even better. (In this case:

https://github.com/openownership/data-standard/blob/master/schema/components.json#L394, OR

https://github.com/openownership/data-standard/blob/7693b43c7d506106196bd83c7cd440e57236341d/schema/components.json#L329)

@kd-ods
Copy link
Collaborator Author

kd-ods commented Aug 17, 2021

To clarify the above. Because of the json schema logic, the user is told "{'motivation': 'correcting'} is not valid under any of the given schemas", rather than the more useful information: statementPointerTarget is a required property of an annotation.

So, if this isn't fixable with a change to the json schema logic, then pointing to the relevant point in the schema is the next best option.

@kd-ods
Copy link
Collaborator Author

kd-ods commented Sep 20, 2021

Just noticed that the existing schema logic also causes/relates to this issue: openownership/data-standard#333

@ghost
Copy link

ghost commented Sep 21, 2021

This is caused by use of "anyOf" - if the data does not match the JSON Schema validator doesn't know which one of the onyOf clauses to use, hence the unhelpful error message. If we could remove the "anyOf" that would be a simple fix, but having just had a quick look I'm not sure we can. We fixed a similar problem with regards to better errors with the 3 statement types, but this might be harder to solve as for that one we could use the "type" key to choose the correct schema and this time there is no such key.

@ghost
Copy link

ghost commented Oct 26, 2021

I'm looking into our custom validator. I've managed to extend it to provide better errors - it will try to select the subschema that is the right one to use based on the selector field then show the user errors, so the user sees "field is required" errors.

Working in isolation at https://gist.github.com/odscjames/3e510bebdaa736b53ad2b80f96d6e43d but when I try and put it into Cove I get problems - I'll keep looking.

Also noting that we could remove the anyOf totally here if we instead added a simple Python check - if an annotation is of type linking then url is a required field.

ghost pushed a commit to OpenDataServices/lib-cove that referenced this issue Oct 27, 2021
ghost pushed a commit to OpenDataServices/lib-cove that referenced this issue Oct 27, 2021
ghost pushed a commit to OpenDataServices/lib-cove that referenced this issue Oct 27, 2021
ghost referenced this issue in openownership/data-standard Oct 27, 2021
@ghost
Copy link

ghost commented Oct 27, 2021

this time there is no such key.

So I was wrong about that. There is such a key - the motivation key. I was able to reuse the same mechanism to pick which sub schema to use, and then we get the direct errors from the subschema like:

oneOfErrors

OpenDataServices/lib-cove#96 does this, however I've tried to do it in a general way so that we can activate this feature in schemas in the future, and not have to change libraries.

This involves some changes to the BODS schema that I'm hoping we can release as patch releases to existing 0.2 and maybe 0.1.

  • It's currently an anyOf - change it to a oneOf. This is pedantically correct - it can only be one of the subschemas. Also, if we leave it as anyOf there will be more code changes needed - the subschemas around statementType currently use oneOf.
  • Add a new key - "oneOfEnumSelectorField": "motivation", - This tells our new library code which field to use.

(If this isn't possible there are other ways possible)

So if that is fine, the process is:

  • Release new version of lib-cove with the above pull request
  • Release new version(s) of data standard with above changes
  • Release new version of lib-cove-bods with new data standard
  • Update Cove with new versions of lib-cove and lib-cove-bods and release

So I'm waiting for feedback on whether changes to standard are ok? @kd-ods

@kd-ods
Copy link
Collaborator Author

kd-ods commented Oct 28, 2021

My overall question is: can we focus on making these changes in the main branch of the schema first, and - for the moment - leave consideration of whether to issue a 0.21 patch?

I'm also not clear on what 'release' means above and how lib-cove-bods changes need to be timed with an actual BODS release.

Also - the new key "oneOfEnumSelectorField": "motivation": is there anything like this in the latest version of JSON schema, or are we going out on a limb?

@kd-ods kd-ods assigned ghost Oct 28, 2021
@ghost
Copy link

ghost commented Oct 28, 2021

can we focus on making these changes in the main branch of the schema first

Yes

leave consideration of whether to issue a 0.21 patch?

(Aside: Does this mean 0.2.1?)

I'm also not clear on what 'release' means above

Release a new version of the software library / standard with a new version number. In the case of the software libraries we do that all the time so it's not a big deal, but I want to talk to you about the options for the standard.

how lib-cove-bods changes need to be timed with an actual BODS release.

All the parts need to be done before this starts working but they don't need to be released together in any way - we can release in any order at any time and that shouldn't cause any issues.

the new key "oneOfEnumSelectorField": "motivation": is there anything like this in the latest version of JSON schema, or are we going out on a limb?

This is new and not JSON Schema standard. But we already have non-standard things like codelists, titles in $ref's, ... - that's part of what https://compiletojsonschema.readthedocs.io/en/latest/ does, turn it back into a standard JSON Schema.

@kd-ods
Copy link
Collaborator Author

kd-ods commented Oct 28, 2021

Thanks for all that, James....

(Aside: Does this mean 0.2.1?)

Yes.

So, I think what I'd say is: check with @rhiaro tomorrow about making those changes on the main branch of the schema. You might want to work on @rhiaro's branch to coordinate the changes?

ghost pushed a commit to OpenDataServices/lib-cove that referenced this issue Oct 28, 2021
ghost pushed a commit that referenced this issue Oct 28, 2021
ghost pushed a commit to OpenDataServices/lib-cove that referenced this issue Oct 28, 2021
ghost pushed a commit that referenced this issue Oct 29, 2021
@rhiaro
Copy link
Collaborator

rhiaro commented Oct 29, 2021

I don't like using schema properties to provide config options to the validator, which oneOfEnumSelectorField is doing. It's not Data, as in it isn't something someone publishing BODS would fill in a value for. It would be out of place to show up in the schema reference or schema browser documentation. It's not something an end user of the data - either publisher or consumer - should have to think about.

If there's a better way to pass this information to the validator (specifically cove, as any other hypothetical BODS validator may do things a different way) we should look at that instead of adding to the schema in this case.

@ghost
Copy link

ghost commented Nov 2, 2021

Discussed with @kd-ods this morning - we'll put in schema for now.

ghost pushed a commit to openownership/data-standard that referenced this issue Nov 2, 2021
ghost pushed a commit to openownership/data-standard that referenced this issue Nov 2, 2021
ghost pushed a commit to openownership/data-standard that referenced this issue Nov 2, 2021
ghost pushed a commit to openownership/data-standard that referenced this issue Nov 2, 2021
openownership/lib-cove-bods#64

This involves changes to build_schemas_for_testing that are required by
the change from anyOf to oneOf.
But actually, this code should have been required before but the compile
tool was not processing anyOf properly.
OpenDataServices/compile-to-json-schema#28
ghost pushed a commit to openownership/data-standard that referenced this issue Nov 4, 2021
openownership/lib-cove-bods#64

This involves changes to build_schemas_for_testing that are required by
the change from anyOf to oneOf.
But actually, this code should have been required before but the compile
tool was not processing anyOf properly.
OpenDataServices/compile-to-json-schema#28
ghost pushed a commit to openownership/data-standard that referenced this issue Nov 4, 2021
openownership/lib-cove-bods#64

This involves changes to build_schemas_for_testing that are required by
the change from anyOf to oneOf.
But actually, this code should have been required before but the compile
tool was not processing anyOf properly.
OpenDataServices/compile-to-json-schema#28

#376
@ghost
Copy link

ghost commented Nov 10, 2021

This is not released yet. 2 things need to happen for this to be released:

  • Cove needs to start using 0.27.0 of Libcove - this will happen at some point anyway.
  • The next version of BODS schema needs to be released with the change.

Discussed with @kd-ods this morning: given the size of problem we are happy to wait for these and thus I am now closing this.

@ghost ghost closed this as completed Nov 10, 2021
This issue was closed.
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

2 participants