-
Notifications
You must be signed in to change notification settings - Fork 96
schema: candidate plugin for json schema validation #1861
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
Plugins/Schema/CMakeLists.txt
Outdated
| add_custom_command( | ||
| OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/ajv_bundle_data.h" | ||
| COMMAND xxd -i "ajv_runtime.min.js" "${CMAKE_CURRENT_BINARY_DIR}/ajv_bundle_data.h" | ||
| WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/external/ajv/dist" | ||
| DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/external/ajv/dist/ajv_runtime.min.js" | ||
| VERBATIM | ||
| ) |
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.
Could we not run this locally and just bundle ajv_bundle_data.h in the tree? We'd get rid of the xxd dependency and wouldn't have cmake doing this each compile
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.
Yes, we can. However, I included this here to make updating the ajv bundle easier if and when there is an ajv or quickjs update as it didn't seem too heavy a load to run during the target build process. A quickjs update isn't all that necessary, but ajv (and its associated bundle) could be updated relatively often to support the latest schema (both of the support systems have scripts for updating them). If the static header is compiled and included, then the xxd requirement in the builder would go away, but it would levy an additional local environmental requirement (xxd or similar) on the updater's machine.
The options as I see them are:
-
Remove this compilation step and levy the local environmental requirement for compilation on the updater's machine before commit.
-
Remove this compilation step and add the compilation as a GHA step if/when updated ajv files are committed.
-
Leave as is.
I'm fine with any of them. I'm going to assume (1) is your preferred choice, so I'll move in that direction today, removing the xxd requirement from the builder, modifying the ajv update script and editing the cmakelists and readme. Let me know if that's an incorrect assumption and I'll correct course. Thanks!
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.
Looking at the ajv repo on github and npm it doesn't look like it's updated that often, last time was in 2024ish. So just having the ajv_bundle_data.h in the tree seems best to me, if there happens to be an update someone can just PR it
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.
Resolved in 6c4f00f.
builder.Dockerfile
Outdated
| @@ -23,7 +23,7 @@ RUN buildDeps="build-essential \ | |||
| autoconf \ | |||
| automake \ | |||
| bison \ | |||
| ccache" \ | |||
| ccache \ | |||
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.
Please readd the double quote or everything'll explode at some point :D
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.
Resolved in b35d3f6.
| /// @return A json object containing validation results. | ||
| json NWNX_Schema_ValidateInstanceByID(json jInstance, string sSchemaId, int nVerbosity = NWNX_SCHEMA_OUTPUT_VERBOSITY_NORMAL); | ||
|
|
||
| int NWNX_Schema_GetIsRegistered(string sSchemaId) { |
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.
Style nit: braces should be on their own line
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.
Resolved in 19211fc.
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.
You missed nwnx_schema.nss I think!
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.
Wow, I think that's the first time I've ever written an .nss file with inline braces. Fixed in 6fc9498.
Plugins/Schema/Schema.cpp
Outdated
| static json JSValueToJSON(JSValue val); | ||
|
|
||
| void SchemaUnload() __attribute__((destructor)); | ||
| void SchemaUnload() { |
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.
Style nit: braces should be on their own line
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.
Resolved in 19211fc.
Plugins/Schema/CMakeLists.txt
Outdated
| @@ -0,0 +1,25 @@ | |||
| add_shared_plugin(Schema | |||
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.
add_plugin() didn't work here?
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.
Resolved in de0cef6.
Plugins/Schema/CMakeLists.txt
Outdated
| @@ -0,0 +1,25 @@ | |||
| add_shared_plugin(Schema | |||
| "Schema.cpp" | |||
| "${CMAKE_CURRENT_SOURCE_DIR}/external/ajv/dist/ajv_bundle_data.h" | |||
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.
${CMAKE_CURRENT_SOURCE_DIR}/ is probably not needed?
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.
Resolved in de0cef6.
Plugins/Schema/CMakeLists.txt
Outdated
| ${CMAKE_SOURCE_DIR} | ||
| ${CMAKE_CURRENT_SOURCE_DIR} | ||
| ${CMAKE_CURRENT_BINARY_DIR} |
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.
Are these needed?
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.
Resolved in de0cef6.
Plugins/Schema/CMakeLists.txt
Outdated
| CONFIG_VERSION="2026" | ||
| ) | ||
|
|
||
| set_target_properties(Schema PROPERTIES LINK_FLAGS "-rdynamic") |
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.
Is this needed?
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.
Resolved in de0cef6.
Plugins/Schema/Schema.cpp
Outdated
| js_std_init_handlers(global_rt); | ||
| js_std_add_helpers(global_ctx, 0, NULL); | ||
|
|
||
| const char* js = R"( |
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 think this, and the others like it, can be static constexpr const char* so they're basically compile time constants
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.
Resolved in afa8a9a.
|
Before I forget, don't forget to add your plugin under the |
This plugin candidate implements a javascript-based json schema validator (Another Json Validator). JS was selected because it's the language of the web and generally kept up to date better than pure-cpp versions (the latest stable cpp version was still at draft 7, js versions are up to date to the soon-to-be-released v1), making updating to current json-schema.org releases much easier.
Info, documentation and examples are in the readme.
This PR requires nwnx-builder to be republished.