Integrate official Draft 2020-12 Basic output tests and preserve annotation value shapes - #978
Conversation
🤖 Augment PR SummarySummary: This PR integrates the Draft 2020-12 Basic-output conformance fixtures and aligns annotation values with the JSON Schema output model. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
bd88e32 to
63a88ab
Compare
|
Hi @jviotti, All CI checks (Linux, macOS, Windows, ASan, DCO, CLA) are now passing green. Summary of Changes:
Whenever you have a moment, could you please take a look? Thanks! |
| bin | ||
| output-tests | ||
| output-tests/README.md | ||
| output-tests/draft2019-09 |
There was a problem hiding this comment.
I think we should do this one too? Or is it different?
There was a problem hiding this comment.
Unmasked output-tests/draft2019-09 as well! Both 2019-09 and 2020-12 follow the same Basic output format specification. Both official suites are now unmasked and passing in output_official_suite.
There was a problem hiding this comment.
I would keep the JS port outside of this to begin with, as we are not running those in the suite? i.e. we can focus on pinning this down for C++ first, and then doing the JS changes?
There was a problem hiding this comment.
We noticed that CI executes node --test "ports/javascript/*.test.mjs" on Linux and macOS, where ports/javascript/output.test.mjs directly runs test/output/output_standard_basic.json.
Because output_standard_basic.json was updated to reflect standard scalar annotations, keeping ports/javascript/index.mjs in sync with the C++ engine's annotation format is required for output.test.mjs and the CI matrix to pass.
There was a problem hiding this comment.
I'm a bit confused by these changes. Does the schema say annotation cannot be an array?
There was a problem hiding this comment.
According to the JSON Schema Specification (Section 12.3 Basic format & official output-schema.json), annotation is defined without restricting its value type (it can be any valid JSON value, depending on what the keyword produces):
- Collection keywords that aggregate multiple evaluated items (e.g.
properties,patternProperties,additionalProperties,unevaluatedProperties,contains) produce a JSON array of values (e.g. array of evaluated property names or item indices). - Scalar / leaf keywords with a single value (such as
title,description,default,readOnly: true, etc.) produce their raw scalar value directly (true,"foo",42), not wrapped in a single-element array (["foo"],[true]).
Previously, Blaze was wrapping all scalar annotations into single-element arrays [annotation], which caused official tests such as readOnly.json to fail schema validation (since the test expects "annotation": true, not "annotation": [true]). The fix preserves raw scalar values for non-collection keywords while retaining array aggregation for keywords that produce collections.
276fc1c to
40f1409
Compare
|
Hey @jviotti , |
| resolve(PROJECT_ROOT, 'build/bin/Release', BINARY_NAME + '.exe'), | ||
| resolve(PROJECT_ROOT, 'build/bin/Debug', BINARY_NAME + '.exe') | ||
| resolve(PROJECT_ROOT, 'build/bin/Debug', BINARY_NAME + '.exe'), | ||
| resolve(PROJECT_ROOT, 'build-ucrt/bin', BINARY_NAME + '.exe') |
| return false; | ||
| } | ||
| const auto &keyword{evaluate_path.back().to_property()}; | ||
| return keyword == "properties" || keyword == "patternProperties" || |
There was a problem hiding this comment.
I read your explanation on https://github.com/sourcemeta/blaze/pull/978/changes#r3845621857. Interesting finding....
The main problem I'm considering now is that:
- Doing these string checks for EVERY annotation can be catastrophic on some larger validation pipelines in terms of performance
- Checking against names is also not 100% correct. For example, what if you have a custom 2020-12 dialect without the unevaluated vocabulary? An annotation would be emitted for i.e.
unevaluatedPropertieseven though it would be an "unknown" annotation and not the legit one from this keyword. Essentially, vocabularies can make this check wrong
Here is what I propose we try:
Can you try to modify this file so that it ALWAYS stores whatever value directly. If we try to store another one to the SAME place, then it gets converted into an array? Would that be a way out of this?
There was a problem hiding this comment.
Also the vocabulary cases I just talked about could make good new tests for the standard basic tests. I bet we are not covering those edge cases, though we should
40f1409 to
e8f8e30
Compare
There was a problem hiding this comment.
2 issues found across 19 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/output/output_standard_basic_test.cc">
<violation number="1" location="test/output/output_standard_basic_test.cc:39">
P1: This test encodes a shape that violates the official Draft 2020-12 Basic output format. The `properties` keyword's annotation is defined as an array of matching property names, so a single matched property must emit `["foo"]`, not the scalar `"foo"` asserted here. The actual `handle_standard` in src/output/output_standard.cc uses a size heuristic (`size() == 1 ? front() : array`), and the `is_array_aggregated_keyword()` function described in the PR was never added, so aggregated keywords with one value unwrap to a scalar and the output no longer matches the spec (and single scalar keywords such as readOnly with repeated values would inflate into arrays). Correct the implementation to emit the aggregation-array shape for properties/patternProperties/additionalProperties/unevaluatedProperties/contains and update these fixture expectations to match the official suite.</violation>
</file>
<file name="test/output/output_standard_basic.json">
<violation number="1" location="test/output/output_standard_basic.json:18">
P2: This fixture now expects the `/properties` annotation to be the scalar `"foo"` instead of `[ "foo" ]`, which contradicts the PR's stated goal that aggregation keywords such as `properties` continue emitting array annotations, and the 2020-12 spec where the `properties` annotation is a set (array) of matched property names. A single matching property must produce `[ "foo" ]`, not `"foo"`; only scalar/metadata keywords (title, readOnly, default, etc.) should be flattened. Please confirm the intended shape — if `properties` should remain an array, this expectation (and the size==1 flattening in output_standard.cc that drives it) is wrong.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "absoluteKeywordLocation": "#/properties", | ||
| "instanceLocation": "", | ||
| "annotation": [ "foo" ] | ||
| "annotation": "foo" |
There was a problem hiding this comment.
P1: This test encodes a shape that violates the official Draft 2020-12 Basic output format. The properties keyword's annotation is defined as an array of matching property names, so a single matched property must emit ["foo"], not the scalar "foo" asserted here. The actual handle_standard in src/output/output_standard.cc uses a size heuristic (size() == 1 ? front() : array), and the is_array_aggregated_keyword() function described in the PR was never added, so aggregated keywords with one value unwrap to a scalar and the output no longer matches the spec (and single scalar keywords such as readOnly with repeated values would inflate into arrays). Correct the implementation to emit the aggregation-array shape for properties/patternProperties/additionalProperties/unevaluatedProperties/contains and update these fixture expectations to match the official suite.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/output/output_standard_basic_test.cc, line 39:
<comment>This test encodes a shape that violates the official Draft 2020-12 Basic output format. The `properties` keyword's annotation is defined as an array of matching property names, so a single matched property must emit `["foo"]`, not the scalar `"foo"` asserted here. The actual `handle_standard` in src/output/output_standard.cc uses a size heuristic (`size() == 1 ? front() : array`), and the `is_array_aggregated_keyword()` function described in the PR was never added, so aggregated keywords with one value unwrap to a scalar and the output no longer matches the spec (and single scalar keywords such as readOnly with repeated values would inflate into arrays). Correct the implementation to emit the aggregation-array shape for properties/patternProperties/additionalProperties/unevaluatedProperties/contains and update these fixture expectations to match the official suite.</comment>
<file context>
@@ -36,7 +36,50 @@ TEST(prettify_annotations) {
"absoluteKeywordLocation": "#/properties",
"instanceLocation": "",
- "annotation": [ "foo" ]
+ "annotation": "foo"
+ }
+ ]
</file context>
| "absoluteKeywordLocation": "#/properties", | ||
| "instanceLocation": "", | ||
| "annotation": [ "foo" ] | ||
| "annotation": "foo" |
There was a problem hiding this comment.
P2: This fixture now expects the /properties annotation to be the scalar "foo" instead of [ "foo" ], which contradicts the PR's stated goal that aggregation keywords such as properties continue emitting array annotations, and the 2020-12 spec where the properties annotation is a set (array) of matched property names. A single matching property must produce [ "foo" ], not "foo"; only scalar/metadata keywords (title, readOnly, default, etc.) should be flattened. Please confirm the intended shape — if properties should remain an array, this expectation (and the size==1 flattening in output_standard.cc that drives it) is wrong.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/output/output_standard_basic.json, line 18:
<comment>This fixture now expects the `/properties` annotation to be the scalar `"foo"` instead of `[ "foo" ]`, which contradicts the PR's stated goal that aggregation keywords such as `properties` continue emitting array annotations, and the 2020-12 spec where the `properties` annotation is a set (array) of matched property names. A single matching property must produce `[ "foo" ]`, not `"foo"`; only scalar/metadata keywords (title, readOnly, default, etc.) should be flattened. Please confirm the intended shape — if `properties` should remain an array, this expectation (and the size==1 flattening in output_standard.cc that drives it) is wrong.</comment>
<file context>
@@ -15,7 +15,31 @@
"absoluteKeywordLocation": "#/properties",
"instanceLocation": "",
- "annotation": [ "foo" ]
+ "annotation": "foo"
+ }
+ ]
</file context>
| "annotation": "foo" | |
| "annotation": [ "foo" ] |
Signed-off-by: HarshPopat23 <musichk61@gmail.com>
e8f8e30 to
481dc7a
Compare
|
Hi @jviotti, Here is a summary of the updates in the latest commit:
All test suites (Official Draft 2019-09 & 2020-12 output suites, standard basic/flag suites, all 674 C++ output unit tests, and the JS test suite) are passing green. PTAL and i was busy with meets since 2 days sorry for delay and guide me if i have missed somthing. |
2 participants
Summary
This PR integrates the official JSON Schema Draft 2020-12 Basic output test suite into Blaze and fixes an annotation value shaping issue in Standard Basic output.
Changes
vendor/jsonschema-test-suite.maskto unmaskoutput-tests/draft2020-12/(escape.json,general.json,readOnly.json,type.json, andoutput-schema.json), while keeping other drafts and formats masked.is_array_aggregated_keyword()helper insrc/output/output_standard.ccandports/javascript/index.mjs.properties,patternProperties,additionalProperties,unevaluatedProperties,contains) continue to emit array annotations (unknown[]).readOnly,title,description,default, etc.) now emit their scalar / JSON value shape (annotation.second.back()) rather than wrapping them into arrays.test/output/output_official_draft2020_12_suite.ccregistered intest/output/CMakeLists.txtto execute all official Draft 2020-12 content output tests againstoutput-schema.json.StandardOutputAnnotationEntry.annotationtype fromunknown[]tounknowninports/javascript/index.d.mts.SimpleOutput.toBasicinports/javascript/index.mjsto match the C++ output shaping.test/output/output_standard_basic.jsonfor scalartitleannotations.src/output/include/sourcemeta/blaze/output_standard.hwhile keeping Detailed/Verbose TODOs.Verification