-
Notifications
You must be signed in to change notification settings - Fork 275
[ExternalLibraries][Core] Add JSON Schema Validation, using json-schema-validator, to Kratos Parameters
#13798
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: master
Are you sure you want to change the base?
Conversation
…d missing parameters functionality
matekelemen
left a comment
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's already a PR for this #13131. It has a reference implementation for integrating it into Kratos.
- I thought schemas were waiting to be discussed by the @KratosMultiphysics/technical-committee. Specifically, I thought we first needed to agree on where to store schemas.
- Do I really need to start using AI to summarize the extremely verbose AI-generated PR description?
kratos/sources/kratos_parameters.cpp
Outdated
| try { | ||
| // Validate the current object's underlying nlohmann::json (mpValue). | ||
| validator.validate(*mpValue); | ||
| } catch (const std::exception& e) { | ||
| // 4. On validation failure, catch the standard exception and re-throw a detailed KRATOS_ERROR. | ||
| std::stringstream msg; | ||
| msg << "JSON schema validation failed.\n"; | ||
| msg << "Validation Error: " << e.what() << "\n\n"; | ||
| msg << "Parameters object being validated:\n" << this->PrettyPrintJsonString() << "\n\n"; | ||
| msg << "Schema used for validation:\n" << rSchema.PrettyPrintJsonString() << "\n"; | ||
| KRATOS_ERROR << msg.str() << std::endl; | ||
| } | ||
|
|
||
| KRATOS_CATCH("") |
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.
What's the point of a try-catch block if you already have it enclosed in KRATOS_TRY - KRATOS_CATCH? This reeks of AI.
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.
Nope, is copy paste
| # New in version 2 | ||
|
|
||
| Although significant changes have been done for the 2nd version | ||
| (a complete rewrite) the API is compatible with the 1.0.0 release. Except for | ||
| the namespace which is now `nlohmann::json_schema`. | ||
|
|
||
| Version **2** supports JSON schema draft 7, whereas 1 was supporting draft 4 | ||
| only. Please update your schemas. | ||
|
|
||
| The primary change in 2 is the way a schema is used. While in version 1 the schema was | ||
| kept as a JSON-document and used again and again during validation, in version 2 the schema | ||
| is parsed into compiled C++ objects which are then used during validation. There are surely | ||
| still optimizations to be done, but validation speed has improved by factor 100 | ||
| or more. |
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.
Yet another compressed external lib that doesn't say anything about its actual version.
You said it was not compiling, I said it could be fixed using a header only solution. Additionally; i dind't know about this PR.
This just adds the validation. It is a minimal interface.
You can do as you like. |
The problem is that we currently apply the same compiler flags to external libs as our own sources, including warnings and treating warnings as errors. To solve this, we'd need to revamp our CMake lists to use target-based properties rather than global ones, which again would need to be discussed in the TC. |
Header only is simpler... and we already do header only with json. |
matekelemen
left a comment
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.
I'm removing my block because I'll be on holiday until mid-October, but please wait for a review from the @KratosMultiphysics/technical-committee. I disabled automerge.
📝 Description
This PR introduces a powerful new feature to the
Kratos::Parametersclass: the ability to validate its contents against a formal JSON Schema.Library considered
This is based on the
pboettch/json-schema-validatorlibrary ("pseudo-official" to nlohmann), and the validated version is JSON Schema Draft 7. The version integrated is a header only, from here pboettch/json-schema-validator#360, to avoid potential issues from the source/header version. For thenlohmann/jsonwe are using the header only too to avoid compiling issues.Version considered
You can typically identify the version a schema is written for by looking at the
$schemakeyword at the root of the schema document. For Draft 7, this is:{ "$schema": "http://json-schema.org/draft-07/schema#" }Why this is important:
$schemakeyword in all your schema files.Motivation
Currently,
Parametersvalidation relies on methods likeValidateAndAssignDefaults, which compares an input against a defaultParametersobject. While useful for setting defaults and checking for extraneous keys, this approach has limitations:minimum,maximum), string patterns (regex), enumerated values (enum), or conditional requirements.By integrating the industry-standard JSON Schema, we provide a much more expressive and robust mechanism for ensuring that user-provided parameters are correct. This leads to:
Implementation Details
New
Validate()Method:Kratos::Parametersclass:void Validate(const Parameters& rSchema) const;pboettch/json-schema-validatorlibrary to perform a full, recursive validation of theParametersobject against the provided schema.Error Handling:
KRATOS_ERROR. The error message is detailed and user-friendly, including:Parametersobject that failed validation.KRATOS_ERRORis also thrown if the provided schema itself is invalid, aiding in debugging.Dependencies:
pboettch/json-schema-validatorsingle-header library, which has been included in the project as a header only library (MIT license).Testing
To ensure the reliability of this new feature, comprehensive tests have been added at both the C++ and Python levels.
C++ Tests (
test_kratos_parameters.cpp):KratosParametersJSONSchemaValidation, has been added.Parametersobject.minimum,exclusiveMinimum).RemoveValueto improve coverage.Python Tests (
test_kratos_parameters.py):TestParametersJSONSchemaValidation, has been added to mirror the C++ tests.Validatemethod are working correctly and that exceptions are properly propagated to the Python layer.Examples
C++ Example
This code snippet shows how you would integrate the validation within a C++ function.
Python Example
This script shows the equivalent operation using the Python bindings. The workflow is identical, using a standard
try...exceptblock to handle theRuntimeErrorthat is thrown from C++.🆕 Changelog
json-schema-validatorjson-schema-validatorinclude for enhanced JSON validationParametersclassKratosParametersJSONSchemaValidationParametersclass