Skip to content

feat(cli): guard object-valued map shapes in schema evolution #155

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

Open
phacops wants to merge 1 commit into
base: main
Choose a base branch
from
+199 −12
Open

feat(cli): guard object-valued map shapes in schema evolution #155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion docs/architecture.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ The `validate-schema` reusable workflow runs on schema PRs and enforces (`sentry
| Remove a namespace | ❌ |
| Change an option's type | ❌ |
| Change an option's default | ❌ |
| Change an object / array-of-object shape | ❌ (`ShapeChanged`) |
| Change an object / array-of-object shape, or a map's value type | ❌ (`ShapeChanged`) |

New namespaces must be named `{repo}` (exact) or `{repo}-*` (prefixed with `{repo}-`).

Expand Down
209 changes: 198 additions & 11 deletions sentry-options-cli/src/schema_evolution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,15 @@ fn compare_schemas(
let old_props = old_meta.property_schema.get("properties");
let new_props = new_meta.property_schema.get("properties");

if old_props != new_props {
// Maps declare their value type under `additionalProperties`. Changing
// it — the value type of a scalar map, or the embedded shape of an
// object-valued map ("embedded keys") — is as breaking as changing a
// fixed struct's fields, so guard it the same way. Deep equality on the
// subtree catches arbitrarily nested changes.
let old_additional = old_meta.property_schema.get("additionalProperties");
let new_additional = new_meta.property_schema.get("additionalProperties");

if old_props != new_props || old_additional != new_additional {
Comment on lines +150 to +153

@sentry sentry Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The check for additionalProperties changes in object types is stricter than for array items, incorrectly flagging the addition of additionalProperties as a breaking change.
Severity: MEDIUM

Suggested Fix

Align the object-type schema evolution check with the array-item check. Add an is_some() guard to the condition for old_additional to ensure that adding additionalProperties to a previously fixed object is not considered a breaking change. The logic should be similar to old_additional.is_some() && old_additional != new_additional.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: sentry-options-cli/src/schema_evolution.rs#L150-L153

Potential issue: The schema evolution logic for object types treats the addition of
`additionalProperties` to a previously fixed struct as a breaking change. The condition
`old_additional != new_additional` triggers when `old_additional` is `None` and
`new_additional` is `Some(...)`. However, the logic for array items handles this
differently, using an `is_some()` guard to prevent this scenario from being flagged as a
breaking change. This asymmetry suggests the object-type check is unintentionally
restrictive and should align with the more permissive array-item behavior.

Did we get this right? 👍 / 👎 to inform future reviews.

@phacops phacops Jul 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not a real issue — the premise doesn't hold here.

property_schema in the evolution check is the post-injection schema (OptionMetadata::property_schema is cloned after inject_object_constraints runs in SchemaRegistry::parse_schema). For any type: "object" option, injection guarantees additionalProperties is present: a fixed struct gets additionalProperties: false injected, and a map keeps its declared value schema. I verified this directly — a struct with no declared additionalProperties yields property_schema.additionalProperties == Some(Bool(false)), never None.

So old_additional is always Some(...) for object options, and the None → Some transition the suggestion is worried about can't occur. The suggested old_additional.is_some() && old_additional != new_additional guard would be a no-op (is_some() is always true), and wouldn't change behavior even for the "add additionalProperties to a fixed struct" case — that goes from Some(false) to Some({...}), which is still !=.

That's also intentional: converting a strict struct into an open struct+map is a shape change, and the existing policy flags any object shape change as breaking (e.g. even adding a field, per test_object_shape_change_fails). The array-item is_some() guard exists only to avoid double-flagging scalar array items (which have no injected additionalProperties) that are already caught by the item-type check — there's no analogous scalar case for a top-level object option.

Generated by Claude Code

modified_options.push(SchemaChangeAction::ShapeChanged {
context: format!("{}.{}", namespace, key),
option_type: "object".to_string(),
Expand All @@ -154,18 +162,23 @@ fn compare_schemas(
}
}

// 5d. changing array-of-objects item shape
// 5d. changing array item shape (fixed struct items or object/scalar-valued
// map items)
if old_meta.option_type == "array" && new_meta.option_type == "array" {
let old_item_props = old_meta
.property_schema
.get("items")
.and_then(|i| i.get("properties"));
let new_item_props = new_meta
.property_schema
.get("items")
.and_then(|i| i.get("properties"));
let old_items = old_meta.property_schema.get("items");
let new_items = new_meta.property_schema.get("items");

let old_item_props = old_items.and_then(|i| i.get("properties"));
let new_item_props = new_items.and_then(|i| i.get("properties"));
// Array of maps: the item's value type lives under `additionalProperties`.
let old_item_additional = old_items.and_then(|i| i.get("additionalProperties"));
let new_item_additional = new_items.and_then(|i| i.get("additionalProperties"));

if old_item_props.is_some() && old_item_props != new_item_props {
let props_changed = old_item_props.is_some() && old_item_props != new_item_props;
let additional_changed =
old_item_additional.is_some() && old_item_additional != new_item_additional;

if props_changed || additional_changed {
modified_options.push(SchemaChangeAction::ShapeChanged {
context: format!("{}.{}", namespace, key),
option_type: "array<object>".to_string(),
Expand Down Expand Up @@ -603,6 +616,180 @@ mod tests {
assert!(result.is_ok());
}

#[test]
fn test_map_value_type_change_fails() {
// A scalar map whose value type changes (string -> integer) is a breaking
// shape change even though the top-level option stays type "object".
let (old_dir, new_dir) = setup_dirs();

let mut old_options = serde_json::Map::new();
old_options.insert(
"tags".to_string(),
json!({
"type": "object",
"additionalProperties": {"type": "string"},
"default": {},
"description": "String tags"
}),
);
let old_schema = build_schema(old_options);

let new_schema = modify_schema(&old_schema, |options| {
options.insert(
"tags".to_string(),
json!({
"type": "object",
"additionalProperties": {"type": "integer"},
"default": {},
"description": "String tags"
}),
);
});

create_schema(&old_dir, "test", &old_schema);
create_schema(&new_dir, "test", &new_schema);

let result = detect_changes(old_dir.path(), new_dir.path(), "test", false, true);
assert!(result.is_err());
match result {
Err(ValidationError::ValidationErrors(errors)) => {
assert_error_contains(&errors, "object shape was modified");
}
_ => panic!("Expected ValidationErrors for map value type change"),
}
}

#[test]
fn test_object_valued_map_embedded_shape_change_fails() {
// A map whose values are objects ("embedded keys"): changing an embedded
// field's type must be rejected.
let (old_dir, new_dir) = setup_dirs();

let mut old_options = serde_json::Map::new();
old_options.insert(
"quotas".to_string(),
json!({
"type": "object",
"additionalProperties": {
"type": "object",
"properties": {
"limit": {"type": "integer"}
}
},
"default": {},
"description": "Per-key quotas"
}),
);
let old_schema = build_schema(old_options);

let new_schema = modify_schema(&old_schema, |options| {
options.insert(
"quotas".to_string(),
json!({
"type": "object",
"additionalProperties": {
"type": "object",
"properties": {
"limit": {"type": "string"}
}
},
"default": {},
"description": "Per-key quotas"
}),
);
});

create_schema(&old_dir, "test", &old_schema);
create_schema(&new_dir, "test", &new_schema);

let result = detect_changes(old_dir.path(), new_dir.path(), "test", false, true);
assert!(result.is_err());
match result {
Err(ValidationError::ValidationErrors(errors)) => {
assert_error_contains(&errors, "object shape was modified");
}
_ => panic!("Expected ValidationErrors for embedded object shape change"),
}
}

#[test]
fn test_object_valued_map_identical_shape_passes() {
// An unchanged object-valued map must not be flagged as a shape change.
let (old_dir, new_dir) = setup_dirs();

let mut options = serde_json::Map::new();
options.insert(
"quotas".to_string(),
json!({
"type": "object",
"additionalProperties": {
"type": "object",
"properties": {
"limit": {"type": "integer"},
"window": {"type": "integer"}
}
},
"default": {},
"description": "Per-key quotas"
}),
);
let schema = build_schema(options);

create_schema(&old_dir, "test", &schema);
create_schema(&new_dir, "test", &schema);

let result = detect_changes(old_dir.path(), new_dir.path(), "test", false, true);
assert!(result.is_ok());
}

#[test]
fn test_array_of_maps_embedded_shape_change_fails() {
// Array whose items are maps: changing the item's value type is breaking.
let (old_dir, new_dir) = setup_dirs();

let mut old_options = serde_json::Map::new();
old_options.insert(
"rows".to_string(),
json!({
"type": "array",
"items": {
"type": "object",
"additionalProperties": {"type": "integer"}
},
"default": [],
"description": "Rows of integer maps"
}),
);
let old_schema = build_schema(old_options);

let new_schema = modify_schema(&old_schema, |options| {
options.insert(
"rows".to_string(),
json!({
"type": "array",
"items": {
"type": "object",
"additionalProperties": {"type": "string"}
},
"default": [],
"description": "Rows of integer maps"
}),
);
});

create_schema(&old_dir, "test", &old_schema);
create_schema(&new_dir, "test", &new_schema);

let result = detect_changes(old_dir.path(), new_dir.path(), "test", false, true);
assert!(result.is_err());
match result {
Err(ValidationError::ValidationErrors(errors)) => {
assert_error_contains(&errors, "array item object shape was modified");
}
_ => panic!("Expected ValidationErrors for array-of-maps shape change"),
}
}

#[test]
fn test_array_of_objects_shape_change_fails() {
let (old_dir, new_dir) = setup_dirs();
Expand Down
Loading