Closed gabywh closed 3 years ago
The examples are not correct. UVCI:s are in lowercase letters...
Also, the rather big change to have the schema preventing several v, r or t entries will probably lead to to too big changes in code generation (haven't tested yet though).
I just tested generating code and it worked fine and nothing was really affected. So, I'll back from my comment.
But the examples still need fixing.
What about the dob being just an empty string? I don't see that here. Was that changed?
The examples are not correct. UVCI:s are in lowercase letters...
ok, that needs fixing (unless meant as negative example / test?) - @ryanbnl ?
The examples are not correct. UVCI:s are in lowercase letters... Also, the rather big change to have the schema preventing several v, r or t entries will probably lead to to too big changes in code generation (haven't tested yet though).
I just tested generating code and it worked fine and nothing was really affected.
Indeed - but always nice to get independent confirmation of this - thanks for taking the time to verify
But the examples still need fixing.
Agreed.
The examples are not correct. UVCI:s are in lowercase letters...
ok, that needs fixing (unless meant as negative example / test?) - @ryanbnl ?
No, I missed it. Copied from our NL examples in the test repository, turns out they were wrong (as they are for about 40% of countries from a random sampling - so Verifiers are accepting them) 🤦
Thanks for the catch Martin! :-)
I've uppercased everything now (the whole string should be upper case).
What about the dob being just an empty string? I don't see that here. Was that changed?
Good catch. Not changed, is definitely in the spec now (I just re-checked). I didn't add it as an example as people will just be tempted to throw in "" all the time, but indeed it needs to be supported.
Means the regex needs to change to include (^$) | (the rest)
and a test added for valid (empty string)
The examples are not correct. UVCI:s are in lowercase letters...
I've uppercased everything now (the whole string should be upper case).
Actually... I fell victim to the (seemingly endless) discussions over this and what was finally captured in the FAQ re UVCI since it was missing from the eHealth Network spec.
I shall leave you with a short summary of the status regarding UVCI format according to the currently approved eHealth Network interop guidelines for you to digest during the course of the evening:
01/LU/3U4KSX4DK7JDR#48
is valid and correctThe examples are not correct. UVCI:s are in lowercase letters...
I've uppercased everything now (the whole string should be upper case).
Actually... I fell victim to the (seemingly endless) discussions over this and what was finally captured in the FAQ re UVCI since it was missing from the eHealth Network spec.
I shall leave you with a short summary of the status regarding UVCI format according to the currently approved eHealth Network interop guidelines for you to digest during the course of the evening:
01/LU/3U4KSX4DK7JDR#48
is valid and correct- there is no requirement for a urn prefix
Indeed, I have a list of extra examples that I want to push by the end of the week, and the short form of the CI is fairly high on that list. Other things such as examples with/without optional fields, different date formats (both DOBs and the dates with timezones) are also on that list.
I would like to contribute my feedback after having tried the rc 1.3.0 in my DCC JSON validator experiment:
Result:
The proposed schema didn't compile.
Possible reason:
http://json-schema.org/draft/2020-12/json-schema-core.html#rfc.section.10.2.1.3
The array elements of "oneOf" must be valid JSON schemas. E.g. the following doesn't compile either:
{ "required": [ "ver", "nam", "dob", "v" ] }
Possible solution:
I found the following solution that compiles and seems to do what it should:
However I could imagine some code generators getting a serious hiccup 🤢 with this.
Validation results:
At a first sight the validation results look good. The DCCs with more than one main element (v,t,r) were discovered with rc1.3.0:
-> logs for 1.2.1 and dcc test data 20210607:
-> logs for rc1.3.0 modififed proposal and dcc test data 20210607:
Empty "dob":
For curiosity I also included the "flex-dob" with the following regex that should do the job:
^((19|20)\d\d(-\d\d){0,2}){0,1}$
Test: http://regexr.com/5v6tg
Can you clarify what you mean with “doesn't compile”?
The combined schema is validated fine using ajv
, and also e.g. IntelliJ/IDEA doesn't see a problem.
Having said that, I agree that the "oneOf": [ { "required": [ "ver", "name", "dob", "v" ] }, ... ]
formulation looks a bit “magical”, and could be reformulated in a less magical way.
We validated the schema with both ajv and jschon.
The "oneOf" is formulated as it is for a reason - if you follow the most obvious composition (as I did) you end up validating a schema that has both a "v" and a "t" array.
With this formulation, you can still have multiple "v" arrays - that's a limitation of json (inherited from javascript). Luckily we have the Business Rules to paste over the inadequacies of web tech đź‘Ť
I would like to contribute my feedback after having tried the rc 1.3.0 in my DCC JSON validator experiment:
Result:
The proposed schema didn't compile.
Do bear in mind that this is an internal release candidate up for review so there will be several commits coming in once the internal reviewers are done - IOW not stable, but thanks anyway for sharing your experience, all testing helps make a better product for all of us.
Can you say which tooling you were using. We're running on ajv and that's working fine.
Possible reason:
http://json-schema.org/draft/2020-12/json-schema-core.html#rfc.section.10.2.1.3
The array elements of "oneOf" must be valid JSON schemas. E.g. the following doesn't compile either:
{ "required": [ "ver", "nam", "dob", "v" ] }
Possible solution:
I found the following solution that compiles and seems to do what it should:
+1
I'll take a look - but difficult to verify it as a fix as we're not seeing the problem.
However I could imagine some code generators getting a serious hiccup with this.
Validation results:
At a first sight the validation results look good. The DCCs with more than one main element (v,t,r) were discovered with rc1.3.0:
that's good.
-> logs for 1.2.1 and dcc test data 20210607:
-> logs for rc1.3.0 modififed proposal and dcc test data 20210607:
Empty "dob":
For curiosity I also included the "flex-dob" with the following regex that should do the job:
^((19|20)\d\d(-\d\d){0,2}){0,1}$
yeah ^$ should be added - already known and delegated to be fixed, but thanks for confirming this.
Test: http://regexr.com/5v6tg
Having said that, I agree that the
"oneOf": [ { "required": [ "ver", "name", "dob", "v" ] }, ... ]
formulation looks a bit “magical”, and could be reformulated in a less magical way.
Could you please be more specific and stop using vague terms like magical? What precisely is the issue?
This is standard JSON Schema formulation, so anyone familiar with JSON Schema should be able to read this. In fact, as one other developer involved here could also tell you, we actually dumbed it down to make it easier to read for end-users who may be not so technical...
Can you clarify what you mean with “doesn't compile”?
The combined schema is validated fine using
ajv
, and also e.g. IntelliJ/IDEA doesn't see a problem. Having said that, I agree that the"oneOf": [ { "required": [ "ver", "name", "dob", "v" ] }, ... ]
formulation looks a bit “magical”, and could be reformulated in a less magical way.
Well your're right. I forgot an important detail. I'm using Ajv in strict mode (whenever possible). Then I get the following error during compile:
Error: strict mode: required property "ver" is not defined at "https://id.uvci.eu/DCC.schema.json#/oneOf/0" (strictRequired)
The oneOf [required clauses] indeed looks a bit strange. But when it's JSON schema spec compliant and just the "price" for working code generators - why not.
Well your're right. I forgot an important detail. I'm using Ajv in strict mode (whenever possible). Then I get the following error during compile:
Error: strict mode: required property "ver" is not defined at "https://id.uvci.eu/DCC.schema.json#/oneOf/0" (strictRequired)
ok thx, so you explicitly set strictRequired: true
in the ajv ctor, or something else?
Having said that, I agree that the
"oneOf": [ { "required": [ "ver", "name", "dob", "v" ] }, ... ]
formulation looks a bit “magical”, and could be reformulated in a less magical way.Could you please be more specific and stop using vague terms like magical? What precisely is the issue?
This is standard JSON Schema formulation, so anyone familiar with JSON Schema should be able to read this. In fact, as one other developer involved here could also tell you, we actually dumbed it down to make it easier to read for end-users who may be not so technical...
OK, maybe being too technical is my problem here :)
More precise formulation of the problem-as-I-perceive-it: the members of the oneOf
specify only the required
properties without explicitly specifying (or referencing specifications for) these properties (name + type + restrictions). E.g., IDEs don't resolve the required
properties at all, while in the previous version they did.
I'll see whether I can come up with a variant of Miguel's proposal that I'd be happier with.
By the way: I'd expect that code generators could have a problem with this schema for the same reason as stated above: no explicit specification of properties referenced in a required
clause.
ok thx, so you explicitly set
strictRequired: true
in the ajv ctor, or something else?
Yes, https://ajv.js.org/options.html#strict
I imagine that you had to disable strict as you're using the custom "valueset-uri" extension property.
For curiosity I also included the "flex-dob" with the following regex that should do the job:
^((19|20)\d\d(-\d\d){0,2}){0,1}$
Test: http://regexr.com/5v6tg
@miguelzapaton Nice :) Could you submit a PR from your own fork/branch onto rc/1.3.0 for this? Would be nice to have you acknowledged in the code base for your work :)
ok thx, so you explicitly set
strictRequired: true
in the ajv ctor, or something else?
+1
I imagine that you had to disable strict as you're using the custom "valueset-uri" extension property.
well yeah - but we could also add a user-defined keyword for valueset-uri
which would allow us to enforce all the strict options - it was actually something I got about halfway through typing this afternoon, before yet another call and yet another meeting...
I'll see whether I can come up with a variant of Miguel's proposal that I'd be happier with.
sure, submit for review, we'll take a look
I'll see whether I can come up with a variant of Miguel's proposal that I'd be happier with.
sure, submit for review, we'll take a look
Will do, although so far Miguel's proposal makes me happy enough - e.g. IDEs grok it fine as well.
I'll see whether I can come up with a variant of Miguel's proposal that I'd be happier with.
sure, submit for review, we'll take a look
Will do, although so far Miguel's proposal makes me happy enough - e.g. IDEs grok it fine as well.
@miguelzapaton could you please submit a PR for your work? More than happy to have you acknowledged here in some official way :)
@miguelzapaton could you please submit a PR for your work? More than happy to have you acknowledged here in some official way :)
@miguelzapaton sorry, couldn't wait for your PR, so I committed the regex and acknowledged you in the commit message, hope that's ok with you? @ryanbnl can you add some positive and negative tests for empty DOB? TIA.
Possible solution:
... which defines it all in-line as you go - also ok, I'll pick it up and try it out here locally. Thx.
Possible solution: https://github.com/miguelzapaton/ehn-dcc-schema-json-validator/blob/main/spec/structure/jsonschema/rc2-1.3.0/DCC.schema.json
... which defines it all in-line as you go - also ok, I'll pick it up and try it out here locally. Thx.
@miguelzapaton: ok tried and tested your version of DCC.schema.json (primarily restructure around allOf / oneOf) - looks good, let's take it :) Rather than watiing on your PR, I mentioned you in the commit message - hope that's ok with you?
Possible solution: https://github.com/miguelzapaton/ehn-dcc-schema-json-validator/blob/main/spec/structure/jsonschema/rc2-1.3.0/DCC.schema.json
... which defines it all in-line as you go - also ok, I'll pick it up and try it out here locally. Thx.
@miguelzapaton: ok tried and tested your version of DCC.schema.json (primarily restructure around allOf / oneOf) - looks good, let's take it :) Rather than watiing on your PR, I mentioned you in the commit message - hope that's ok with you?
286144f
@gabywh That's just perfect :)
Has anybody tried code generation with the allOf / oneOf combination?
I ran "make test" and all those passed, so that's the "smoke check" level looking good.
More extensive independent checking would need to be done by other implementors. Fingers crossed!
It would be nice to acknowledge and incorporate your contribution, particularly as you kindly took the trouble to not only identify a potential issue but also supply a proposed solution. That is something that should never be taken for granted and I for one very much appreciate your positive and pro-active attitude here, thx for that :)
miguelzapaton @.***> schrieb am Do., 10. Juni 2021, 09:49:
Possible solution:
... which defines it all in-line as you go - also ok, I'll pick it up and try it out here locally. Thx.
@miguelzapaton https://github.com/miguelzapaton: ok tried and tested your version of DCC.schema.json https://github.com/miguelzapaton/ehn-dcc-schema-json-validator/blob/main/spec/structure/jsonschema/rc2-1.3.0/DCC.schema.json (primarily restructure around allOf / oneOf) - looks good, let's take it :) Rather than watiing on your PR, I mentioned you in the commit message - hope that's ok with you?
286144f
@gabywh https://github.com/gabywh That's just perfect :)
Has anybody tried code generation with the allOf / oneOf combination?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ehn-dcc-development/ehn-dcc-schema/pull/100#issuecomment-858396713, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASHCPHKI5C2BY3JRV3IRJO3TSBVCLANCNFSM46MFV4JQ .
Possible solution: https://github.com/miguelzapaton/ehn-dcc-schema-json-validator/blob/main/spec/structure/jsonschema/rc2-1.3.0/DCC.schema.json
... which defines it all in-line as you go - also ok, I'll pick it up and try it out here locally. Thx.
@miguelzapaton: ok tried and tested your version of DCC.schema.json (primarily restructure around allOf / oneOf) - looks good, let's take it :) Rather than watiing on your PR, I mentioned you in the commit message - hope that's ok with you?
286144f
@gabywh That's just perfect :)
Has anybody tried code generation with the allOf / oneOf combination?
Doesn't work. At least not in Java using the jsonschema2pojo-maven-plugin. I suggest that we revert to the previous version.
Thanks for doing an independent check.
Ok so two options:
Op do 10 jun. 2021 11:33 schreef Martin Lindström @.***
:
Possible solution:
... which defines it all in-line as you go - also ok, I'll pick it up and try it out here locally. Thx.
@miguelzapaton https://github.com/miguelzapaton: ok tried and tested your version of DCC.schema.json https://github.com/miguelzapaton/ehn-dcc-schema-json-validator/blob/main/spec/structure/jsonschema/rc2-1.3.0/DCC.schema.json (primarily restructure around allOf / oneOf) - looks good, let's take it :) Rather than watiing on your PR, I mentioned you in the commit message - hope that's ok with you?
286144f
@gabywh https://github.com/gabywh That's just perfect :)
Has anybody tried code generation with the allOf / oneOf combination?
Doesn't work. At least not in Java using the jsonschema2pojo-maven-plugin.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ehn-dcc-development/ehn-dcc-schema/pull/100#issuecomment-858468751, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASHCPHPGAJDPQFN5KMP4EDTTSCBFBANCNFSM46MFV4JQ .
@martin-lindstrom I feared that. There's an interesting proposal in the code generator project:
https://github.com/joelittlejohn/jsonschema2pojo/wiki/Proposal-for-allOf,-anyOf-and-oneOf
Just for completeness: This was my first try yesterday - but I was not sure if everybody would be happy with the introduced "supertype property" "vtr" - even if code generation would work with it:
{
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "https://id.uvci.eu/DCC.schema.json",
"title": "EU DCC",
"description": "EU Digital Covid Certificate",
"$comment": "Schema version 1.3.0 exp code generation supertype no allOf",
"type": "object",
"required": [
"ver",
"nam",
"dob",
"vtr"
],
"properties": {
"ver": {
"title": "Schema version",
"description": "Version of the schema, according to Semantic versioning (ISO, https://semver.org/ version 2.0.0 or newer)",
"type": "string",
"pattern": "^\\d+.\\d+.\\d+$",
"examples": [
"1.3.0"
]
},
"nam": {
"description": "Surname(s), given name(s) - in that order",
"$ref": "https://id.uvci.eu/DCC.Core.Types.schema.json#/$defs/person_name"
},
"dob": {
"title": "Date of birth",
"description": "Date of Birth of the person addressed in the DCC. ISO 8601 date format restricted to range 1900-2099",
"type": "string",
"pattern": "^((19|20)\\d\\d(-\\d\\d){0,2}){0,1}$",
"examples": [
"1979-04-14",
"1950",
"1901-08"
]
},
"vtr": {
"type": "array",
"minItems": 1,
"maxItems": 1,
"oneOf": [
{
"description": "Vaccination Group",
"items": {
"$ref": "https://id.uvci.eu/DCC.Types.schema.json#/$defs/vaccination_entry"
}
},
{
"description": "Test Group",
"items": {
"$ref": "https://id.uvci.eu/DCC.Types.schema.json#/$defs/test_entry"
}
},
{
"description": "Recovery Group",
"items": {
"$ref": "https://id.uvci.eu/DCC.Types.schema.json#/$defs/recovery_entry"
}
}
]
}
}
}
I would expect your generated v1.2.1 POJOs to currently have three Collections (v,t,r) and not one supertype Collection as the above one would probably produce.
@martin-lindstrom I feared that. There's an interesting proposal in the code generator project:
https://github.com/joelittlejohn/jsonschema2pojo/wiki/Proposal-for-allOf,-anyOf-and-oneOf
Just for completeness: This was my first try yesterday - but I was not sure if everybody would be happy with the introduced "supertype property" "vtr" - even if code generation would work with it:
Supertype property not an option - breaks existing schema structure.
Shame, but nothing for it but to revert #286144f
restructure allOf/oneOf reverted by #58ec1c6
One last comment on this, even if it's already reverted:
A third possible solution
Conclusion
When you're convinced (-> FAQ) that Postel's Law should be applied in this project the logical consequence would be to have two schema invariants for each schema version:
e.g. "DGC.schema.json" (e.g. like v1.2.1 light)
-> used for input payload validation -> used for code generation
e.g. "DGC.schema.extended.json" (e.g. like the allOf/oneOf proposal, even with stronger regex etc.)
-> used for output payload validation
Imo this would best fit the diverse reality of implementations and contribute to interoperability without limiting people.
Naming
There would be many possibilities to name the invariant files:
DCC.schema{R}.json vs DCC.schema{S}.json
$R / $S
e.g.
<
One last comment on this, even if it's already reverted:
A third possible solution
- Apply Postel's Law to JSON Schema
Full marks for at least mentioning / considering Postel's Law - that already puts you steps ahead of some others :)
Conclusion
When you're convinced (-> FAQ) that Postel's Law should be applied in this project the logical consequence would be to have two schema invariants for each schema version:
... but you're conclusion is (somewhat) incorrect, even if at first sight it seems logical. Let me walk you through the reasoning by way of example.
First of all, let's consider the extreme case:
strict in what you generate: - issue in full conformance to all business rules (highest priority) and then to the DCC physical structure (schema). Since Issuance business rules may vary by Member State then the underlying schema needs to be flexible enough to accomodate them all.
tolerant in what you accept: in the extreme case, no schema validation at all, all validation by your own verification business rules and no schema validation at all - after all, the schema defines "only" the physical payload structure
Issues with this extreme scenario:
Verification - if you don't use schema validation then there will be lots of boiler plate checks in code that could easily be handled by schema validation. The mistake here would be to apply schema validation and immediately reject the payload.
Applying Postel's Law:
So let's take a small example: I generate my QR payload with a date-time field e.g. for a test certificate I specify date-time of sample collection (t/sc
) which is given in the v1.3.0 spec as e.g. YYYY-MM-DDThh:mm:ssZ. However, I generate with more precision than is specifed (you could argue this is a violation of Postel's Law, or you could argue that it is not - you are being very specific with what you generate) YYYY-MM-DDThh:mm:ss.sssZ.
Applying a very strict schema here on the verification side would cause this to fail, which is perhaps not an altogether unreasonable option provided you don't immediately fail the payload. You see, the information that you require (YYYY-MM-DDThh:mm:ss) is actually present as a subset of the complete value. So what you could do is try validation, let it fail but then handle the schema validation error(s) and see if there is something you can rescue - here the case would be you can definitely "rescue" the original required information to the specified resolution and so you can continue with the verification process.
Where having some schema structure and some typing of entries comes in useful is to easily catch "obvious" errors like someone putting "abcde" in a date field (e.g. hacker trying to look for weaknesses in the system). This too would fail the standard schema validation check then your code to handle this failed schema check can take a look at the value and see if you can generate a date out of the data. With "abcde" no you can't so this would fail verification.
Similar case actually for a string without the CBOR string tag: it may fail initial validation, but you try to see if you can make something sensible out of the contents anyway. If so, then continue. If not, then fail.
A third and final rather topical example: supposing you were verifying with v 1.0.x of the schema, and someone sends you a partial date with e.g. v1.2.x of the schema. This will fail initial schema validation. So you handle the error case and take a look at what you have. Well, you have just the year. Do your business rules allow just a year here? If so, then take the partial date and continue. If not, then you can reject as invalid -> but only after you have a first schema validation error and then seeing if you can use whatever is in the field anyway, according to your verification business rules.
That's Postel's law applied in practice.
Out of all of the above:
HTH, Gaby.
All examples use year-only for the birth of date. This is not how a typical DCC should look like, so they need to be updated.
Agreed. We are talking something around 99.5% of all cases with full YYYY-MM-DD and thus 0.5% without a complete date. Out of that remaining 0.5%, by far the most common from I have seen in practice is YYYY only. Leaving just a fraction of a fraction (no hard figures to hand here, sorry, just my own experience) who would have YYYY-MM or YYYY--DD or even no birth date at all
It is enough if one example uses the year-only format.
One positive test of each and a couple of negative tests would be sufficient here, I would think.
We can't express everything using the schema anyway.
Indeed, nor should we try to - see other text on Postel's Law and the predominance of the business rules (https://github.com/ehn-dcc-development/ehn-dcc-schema/pull/100#issuecomment-858845610)
In accordance with the eHealth Network JSON Schema v1 3.0 which has just been formally approved, please review