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.

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

26 changes: 25 additions & 1 deletion src/v/pandaproxy/schema_registry/json.cc
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,31 @@ bool is_array_superset(json::Value const& older, json::Value const& newer) {
return false;
}

// uniqueItems makes sense mostly for arrays, but it's also allowed for
// tuples, so the validation is done here
auto get_unique_items = [](json::Value const& v) {
auto it = v.FindMember("uniqueItems");
if (it == v.MemberEnd()) {
// default value
return false;
}
return it->value.GetBool();
};

auto older_value = get_unique_items(older);
auto newer_value = get_unique_items(newer);
// the only failure mode is if we removed the "uniqueItems" requirement
// older ui | newer ui | compatible
// -------- | -------- | ---------
// false | ___ | yes
// true | true | yes
// true | false | no

if (older_value == true && newer_value == false) {
// removed unique items requirement
return false;
}

throw as_exception(invalid_schema(fmt::format(
"{} not implemented. input: older: '{}', newer: '{}'",
__FUNCTION__,
Expand Down Expand Up @@ -1097,7 +1122,6 @@ bool is_superset(json::Value const& older, json::Value const& newer) {
"$schema",
"additionalItems",
"items",
"uniqueItems",
"definitions",
"dependencies",
"allOf",
Expand Down
6 changes: 6 additions & 0 deletions src/v/pandaproxy/schema_registry/test/test_json_schema.cc
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,12 @@ static constexpr auto compatibility_test_cases = std::to_array<
.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

// array checks: uniqueItems must be compatible
{
.reader_schema = R"({"type": "array", "uniqueItems": true})",
.writer_schema = R"({"type": "array"})",
.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 adding a test here for the default value handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i'll followup

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