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

BG: EXPECTEDSCHEMAVALIDATION #298

Open
bhavin-qryptal opened this issue Jun 8, 2021 · 14 comments
Open

BG: EXPECTEDSCHEMAVALIDATION #298

bhavin-qryptal opened this issue Jun 8, 2021 · 14 comments

Comments

@bhavin-qryptal
Copy link
Collaborator

Affected Country: BG

@stamo , @SchulzeStTSI and @daniel-eder - FYI.

Issue Description

Payload does not comply with Schema.

Ref:
BG/2DCode/raw/1.json
BG/2DCode/raw/2.json

>           raise error
E           jsonschema.exceptions.ValidationError: None is not of type 'array'
E           
E           Failed validating 'type' in schema['properties']['v']:
E               {'description': 'Vaccination Group',
E                'items': {'description': 'Vaccination Entry',
E                          'properties': {'ci': {'description': 'Certificate '
E                                                               'Identifier, format as '

image

Proposed Solution

Removing these NULL values should resolve the issue.

    "r": null,
    "t": null,

@stamo
Copy link
Contributor

stamo commented Jun 8, 2021

Thanks a lot we are aware of that, that's why we issued 3 and 4 which are correct. The problem is that we have issued approximately 1000000 certificates in production with these null objects and we are asking everyone to make schema validation a little bit softer, to accept these QR codes

@SchulzeStTSI
Copy link
Collaborator

@bhavin-qryptal Sorry for the confusion. There is an exception currently for BG which should ignore nulls for arrays and a different format for DOB. Can you relax the schema f.e. like if(countryCode=="BG") then the type of r is not array, but ["null","array"]. DOB should be as well allowed for T00:00:00. We wait for the final decision tomorrow.

@bhavin-qryptal
Copy link
Collaborator Author

@SchulzeStTSI and @stamo , thank you for your inputs.

I believe, in order to relax this, schema change would be required. Validation script has NO specific logic to deal with individual attributes of the payload.

@SchulzeStTSI
Copy link
Collaborator

@bhavin-qryptal Let's see how we handle this. I would like to copy the schema into the script(folder) instead of loading it live. Then we can relax it easily. The official schema should not be touched.

@bhavin-qryptal
Copy link
Collaborator Author

@SchulzeStTSI , Are you suggesting to modify the schema before it is used for validation by test script? In that case, wouldn't such code fail validation when real life apps and services try to validate such codes?

@SchulzeStTSI
Copy link
Collaborator

@bhavin-qryptal Currently is the discussion about, if the verifier should do a mandatory validation of the schema. The most of the people cite Postal's Law, which means that the issuer should be very strict, but the verifier must be so flexible as possible. Under this assumptions the verifier schema can be relaxed, but the issuer must still following the strict one. Currently we have some apps outside which do a very strict validation, and some which are validate nothing. I think the golden way is somewhere in the middle. Let's see what the todays discussion outcome is.

@bhavin-qryptal
Copy link
Collaborator Author

@SchulzeStTSI , Thank you for explanation, it is useful. For now, validation script is marking such tests as XFAIL. Test script could be updated after today's discussion. It MIGHT be a good idea to have two sets of schema, One for issuer and other for validator.

This was referenced Jun 9, 2021
@SchulzeStTSI
Copy link
Collaborator

@bhavin-qryptal Let's relax the schema here for the codes. What do you think where should place in the best case the schema copy?

@bhavin-qryptal
Copy link
Collaborator Author

@SchulzeStTSI , Please help me with few basic queries.

  1. Who would author the schema to use for validation?
  2. How would this new set of validation only schema be controlled and how would it remain in sync with schema use for generation?
  3. Likewise https://id.uvci.eu/DGC.schema.json, could we have new set of schema available at some public URL (e.g. https://id.uvci.eu/DGC.validation.schema.json)? Also this new validation schema set should be maintained under GitHub repository.

Looking forward to have your thoughts and comments.

@vanlooverenkoen
Copy link

vanlooverenkoen commented Jun 10, 2021

In the future, this should be discussed first. Before we merge such a pull request. Right now all pull requests are merged even if the checks are failing. If the schema should be relaxed, the github checks should be relaxed as well so we don't have problems when other countries create a new pull request.

@SchulzeStTSI
Copy link
Collaborator

@vanlooverenkoen Yes you are absolutley right. This was an quick merge, just to highlight the current situation with the productive codes.

@bhavin-qryptal Regarding your questions:

  1. The schema is created within this repo: https://github.com/ehn-dcc-development/ehn-dcc-schema in multiple versions. We should create under "tests" a folder with the different version 1.0,1.1,1.2 etc and copy them into it. This schemas must be relaxed regarding the array handling for r,t,v (["null","array"]) and the dob(https://github.com/eu-digital-green-certificates/dgca-app-core-android/blob/main/decoder/src/main/java/dgca/verifier/app/decoder/JsonSchema.kt#L54 )
  2. The sync should be manually. All issuers and verifiers use the schema which are currently offically released. But this is for issuing. Verifiers can have a different behaviour (or no validation). This is still under discussion.
  3. We are working on the public URL. Still under discussion. Currently all verifier apps which are doing a validation integrate it hardcoded.

@vaubaehn
Copy link

3. We are working on the public URL. Still under discussion. Currently all verifier apps which are doing a validation integrate it hardcoded.

Verifier apps and wallet apps .
Adjustments to the schemas made in the past two weeks and problems with implementations on issuer's side are already causing problems with wallet apps and verifier apps, where (previous) schemas and validation rules are hardcoded, see issue mentioned above @SchulzeStTSI 's reply.

Are developers of wallet and verifier apps are informed pro-actively from your side to take account of changes? Or are they forced to regularly track changes here by themselves?

In this view, does it even make sense that wallet apps and verifier apps already have gone live, when they can't handle all adjustments from the past weeks?

Thank you in advance for your reply.

@bhavin-qryptal
Copy link
Collaborator Author

  • The sync should be manually. All issuers and verifiers use the schema which are currently offically released. But this is for issuing. Verifiers can have a different behaviour (or no validation). This is still under discussion.

Purpose if the validation script is to ensure that all the codes issues / submitted by member state complies with the standards. Actual validator app could have liberal implementation but the validation script should have stricter checks to ensure good compliance (e.g. schema compliance). Otherwise, we would miss capturing important compatibility issues upfront.

@SchulzeStTSI
Copy link
Collaborator

@vanlooverenkoen We announce all changes in our regular meetings. This kind of changes were announced last week, after the report from bulgaria at monday.

@bhavin-qryptal OK agreed. We should keep this repo as it is to ensure more compliance. I will contact stamo and then we move the codes to another repo for "in field" compatibility/versioning checks. Thank you. I let you know when we have setup it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants