Skip to content

Commit d7c5dd5

Browse files
committed
schema_registry: enforce strict validation for missing required attributes
The `is_object_required_superset` function in the JSON schema validator contained a bug where `std::views::filter` was incorrectly dropping missing required fields during compatibility checks. This allowed schemas with missing required attributes to falsely pass validation. This commit removes the faulty filter to strictly enforce superset validation. If a reader schema demands a required attribute that the writer schema lacks (and does not provide a default for), the validator now correctly identifies the incompatibility. Additionally, updated the `test_json_compat_messages` test case to expect the newly exposed `required_attribute_added` error, which was previously masked by this bug.
1 parent 8ad2288 commit d7c5dd5

2 files changed

Lines changed: 39 additions & 25 deletions

File tree

src/v/pandaproxy/schema_registry/json.cc

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1670,41 +1670,39 @@ json_compatibility_result is_object_required_superset(
16701670
const json::Value& newer,
16711671
const std::filesystem::path& p) {
16721672
json_compatibility_result res;
1673-
// to pass the check, a required property from newer has to be present in
1674-
// older, or if new it needs to have a default value.
1675-
// note that:
1676-
// 1. we check only required properties that are in both newer["properties"]
1677-
// and older["properties"]
1678-
// 2. there is no explicit check that older has an open content model
1679-
// there might be a property name outside of (1) that could be rejected
1680-
// by update, if update["additionalProperties"] is false
1673+
// older is a superset of newer iff every required property in older is
1674+
// also required in newer, with one exception: if older provides a
1675+
// "default" for the property, the consumer can fill it in when a writer
1676+
// (newer) omits it, so it doesn't have to be in newer.required.
1677+
//
1678+
// Note: this does not check for new required properties added on the
1679+
// newer side when older is closed (additionalProperties: false). That
1680+
// case is covered separately by
1681+
// required_property_added_to_unopen_content_model (TODO).
16811682

16821683
auto older_req = get_array_or_empty(older, "required");
16831684
auto newer_req = get_array_or_empty(newer, "required");
16841685
auto older_props = get_object_or_empty(older, "properties");
1685-
auto newer_props = get_object_or_empty(newer, "properties");
1686-
1687-
// TODO O(n^2) lookup that can be a set_intersection.
1688-
auto older_req_in_both_properties
1689-
= older_req | std::views::filter([&](const json::Value& o) {
1690-
return newer_props.HasMember(o) && older_props.HasMember(o);
1691-
});
16921686

16931687
// for each element:
16941688
// in older.required? | in newer.required? | result
16951689
// yes | yes | yes
16961690
// yes | no | if it has "default" in older
16971691
// no | yes | yes
1698-
std::ranges::for_each(
1699-
older_req_in_both_properties, [&](const json::Value& o) {
1700-
if (
1701-
std::ranges::find(newer_req, o) == newer_req.End()
1702-
&& !older_props.FindMember(o)->value.HasMember("default")) {
1703-
res.emplace<json_incompatibility>(
1704-
p / "required" / as_string_view(o),
1705-
json_incompatibility_type::required_attribute_added);
1706-
}
1707-
});
1692+
1693+
// TODO O(n^2) lookup that can be optimized using a flat_hash_set of
1694+
// newer_req.
1695+
std::ranges::for_each(older_req, [&](const json::Value& o) {
1696+
auto it = older_props.FindMember(o);
1697+
bool has_default = it != older_props.MemberEnd()
1698+
&& it->value.HasMember("default");
1699+
if (
1700+
std::ranges::find(newer_req, o) == newer_req.End() && !has_default) {
1701+
res.emplace<json_incompatibility>(
1702+
p / "required" / as_string_view(o),
1703+
json_incompatibility_type::required_attribute_added);
1704+
}
1705+
});
17081706
return res;
17091707
}
17101708

src/v/pandaproxy/schema_registry/test/test_json_schema.cc

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -812,6 +812,21 @@ static const auto compatibility_test_cases = std::to_array<compatibility_test_ca
812812
// Note: this is reported as combined_type_changed by the reference impl
813813
.compat_result = {{"#/", incompat_t::combined_type_subschemas_changed}},
814814
},
815+
// #30398: adding a required field whose name is absent from the writer's
816+
// properties must be flagged. Previously masked by a faulty filter.
817+
{
818+
.reader_schema = R"({
819+
"type": "object",
820+
"properties": {"name": {"type": "string"}, "id": {"type": "integer"}},
821+
"required": ["name", "id"]
822+
})",
823+
.writer_schema = R"({
824+
"type": "object",
825+
"properties": {"name": {"type": "string"}},
826+
"required": ["name"]
827+
})",
828+
.compat_result = {{"#/required/id", incompat_t::required_attribute_added}},
829+
},
815830
// object checks: removing a required property not ok if didn't have a default value
816831
{
817832
.reader_schema = R"(
@@ -2298,6 +2313,7 @@ const absl::flat_hash_set<incompatibility> forward_expected{
22982313
// for this schema update.
22992314
// {"#/properties/cccc",
23002315
// incompat_t::required_property_added_to_unopen_content_model},
2316+
{"#/required/cccc", incompat_t::required_attribute_added},
23012317
{"#/dependencies/a", incompat_t::dependency_array_extended},
23022318
{"#/dependencies/d", incompat_t::dependency_array_added},
23032319
{"#/dependencies/b", incompat_t::type_narrowed},

0 commit comments

Comments
 (0)