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.

Sign up for GitHub

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

[CORE-3186] schema registry json schema: array compatibility checks #20137

Merged
merged 7 commits into from
Jun 28, 2024
Merged

[CORE-3186] schema registry json schema: array compatibility checks #20137

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
701189e
schema_registry/json: move is_object_additional_properties_superset
andijcr Jun 27, 2024
6b3df9c
schema_registry/json: refactor is_object_additional_properties_superset
andijcr Jun 27, 2024
83962e5
schema_registry/json: is_array_superset size checks
andijcr Jun 27, 2024
4c9789c
schema_registry/json: is_array_superset uniqueItems
andijcr Jun 27, 2024
04edb4f
schema_registry/json: is_array_superset array schema
andijcr Jun 27, 2024
8f0aadd
schema_registry/json: is_array_schema tuple schema
andijcr Jun 27, 2024
3c9596f
schema_registry/json: fix is_numeric_property_value_superset
andijcr Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions src/v/pandaproxy/schema_registry/json.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,9 +731,28 @@ bool is_numeric_superset(json::Value const& older, json::Value const& newer) {
return true;
}

bool is_array_superset(
[[maybe_unused]] json::Value const& older,
[[maybe_unused]] json::Value const& newer) {
bool is_array_superset(json::Value const& older, json::Value const& newer) {
// "type": "array" is used to model an array or a tuple.
// for array, "items" is a schema that validates all the elements.
// for tuple in Draft4, "items" is an array of schemas to validate the
// tuple, and "additionaItems" a schema to validate extra elements.
// TODO in later drafts, tuple validation has "prefixItems" as array of
// schemas, "items" is for validation of extra elements, "additionalItems"
// is not used.
// This superset function has a common section for tuples and array, and
// then is split based on array/tuple.

// size checks are common to both types
if (!is_numeric_property_value_superset(
older, newer, "minItems", std::less_equal<>{})) {
return false;
}

if (!is_numeric_property_value_superset(
older, newer, "maxItems", std::greater_equal<>{})) {
return false;
}

throw as_exception(invalid_schema(fmt::format(
"{} not implemented. input: older: '{}', newer: '{}'",
__FUNCTION__,
Expand Down Expand Up @@ -1078,8 +1097,6 @@ bool is_superset(json::Value const& older, json::Value const& newer) {
"$schema",
"additionalItems",
"items",
"maxItems",
"minItems",
"uniqueItems",
"definitions",
"dependencies",
Expand Down
6 changes: 6 additions & 0 deletions src/v/pandaproxy/schema_registry/test/test_json_schema.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,12 @@ static constexpr auto compatibility_test_cases = std::to_array<
= R"({"type": "object", "properties": {"a": {"type": "integer", "default": 10}}, "required": ["a"]})",
.reader_is_compatible_with_writer = false,
},
// array checks: size increase is not allowed
{
.reader_schema = R"({"type": "array", "minItems": 2, "maxItems": 10})",
.writer_schema = R"({"type": "array", "maxItems": 11})",
.reader_is_compatible_with_writer = false,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is it worth splitting this into 2 tests, one that changes minItems and another that changes maxItems to show that neither change is allowed (independently of one another)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, i'll follow up

// combinators: "not" is required on both schema
{
.reader_schema
Expand Down