-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add ELM validation #688
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still some leftover println! statements in this file :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
| let errors: Vec<ValidationError> = schema.iter_errors(data).collect(); | ||
| if !errors.is_empty() { | ||
| println!("Validation errors: {errors:#?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed? Or should this be error! instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
| #[test] | ||
| fn credential_schema_validation_elm_edc_ok() { | ||
| let result = validate_credential_types(&EXAMPLE_BASIC_ELM_EDC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the value that is being passed to validate_credential_types is a Value::Object, however the code introduced here passes a Value::String which will then always result in debug!("No credential type found, skipping validation");.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
| // Select correct draft version for JSON Schema Validator | ||
| // Define the relative path to our jsonschema folder needed for the LocalRetriever | ||
| let jsonschema_dir = std::env::current_dir().unwrap().join("resources/jsonschemas"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will panic on mobile (or in any environment where the application runs without the source code) because resources/* is not part of the binary. I think there are at least two ways to deal with this:
- Take a look at
initialize_storageand how we make use of Tauri'sdata_dirmethod. - Easier option (and therefore probably preferred): since we're dealing with static data here (in contrast to
STATE_FILE,STRONGHOLD, etc.. --> Embed the JSON Values directly in to the code the same way we do for Ferris's test Credentials inferris_static_profile.rs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you go with option 2 I think you should still be able to keep your json files the way they are if you just 'include' them into the code through include_str!.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
took a slightly different approach but i think we are still aligned
Description of change
I've added support for ELM validation in the
validate_credential_types()function.To avoid confusion: the European Publication Office has converted the VC DM 1.1 into a JSON Schema as well but enhanced it to match the EDC with fields such as
validFromandissued, which are not in the original VC DM 1.1.Therefore, we now have 2 distinct VC DM 1.1 JSON Schemas. One is created by ourselves, true to the spec. The other is provided by European Publication Office as it is referenced in their EDC JSON Schema.
Links to any relevant issues
How the change has been tested
The old tests pass.
I've added a unit test for a valid ELM validation.
Run
cargo test --allin the repo root to test.Definition of Done checklist
Add an
xto the boxes that are relevant to your changes.