Skip to content

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

Open
phacops wants to merge 1 commit into
mainfrom
claude/object-additional-properties-e6l5yu
Open

feat(cli): guard object-valued map shapes in schema evolution#155
phacops wants to merge 1 commit into
mainfrom
claude/object-additional-properties-e6l5yu

Conversation

@phacops

@phacops phacops commented Jul 9, 2026

Copy link
Copy Markdown

What

Objects as additionalProperties — maps whose values are structured objects with embedded keys — are already expressible: the validation core (sentry-options-validation), the meta-schema, and the Python stub generator all handle them (see test_object_valued_map_validates_nested_values, test_nested_fixed_object_enforces_required_and_rejects_unknown, test_generate_object_valued_map, etc.).

The one place that hadn't caught up was the schema-evolution breaking-change guard (sentry-options-cli/src/schema_evolution.rs). compare_schemas only inspected properties (for object options) and items.properties (for array options). Because a map declares its value type under additionalProperties rather than properties, a breaking change to it went undetected:

  • changing a scalar map's value type ({"additionalProperties": {"type": "string"}}{"type": "integer"}), and
  • changing the embedded object shape of an object-valued map (additionalProperties: {"type": "object", "properties": {"limit": {"type": "integer"}}}limit becomes a string)

both kept the top-level option type "object" and default {}, so nothing flagged them.

Changes

  • Extend the object shape check to also compare additionalProperties, and the array item shape check to also compare items.additionalProperties. Deep equality on the subtree catches arbitrarily nested changes, and these are reported as ShapeChanged — consistent with how struct and array-of-object shape changes are already treated.
  • Add tests: scalar-map value-type change, object-valued map embedded shape change, array-of-maps shape change, and an identical-map no-change case (must still pass).
  • Update the schema-evolution rules table in docs/architecture.md to mention map value types.

Testing

  • cargo test -p sentry-options-cli -p sentry-options-validation — all green (64 + 91).
  • cargo fmt --check and cargo clippy -p sentry-options-cli clean.
  • Pre-existing flaky failures in the sentry-options client testing module reproduce on a clean tree and are unrelated to this change.

🤖 Generated with Claude Code


Generated by Claude Code

Object-valued `additionalProperties` (maps with embedded keys) are
already supported by the validation core and stub generator, but the
schema-evolution breaking-change guard only inspected `properties` and
`items.properties`. A breaking change to a map's value type — or to the
embedded object shape of an object-valued map ("embedded keys") — slipped
through undetected.

Extend `compare_schemas` to also compare `additionalProperties` for
object options and `items.additionalProperties` for array options, so
these changes are flagged as `ShapeChanged` just like struct and
array-of-object shape changes. Deep equality on the subtree catches
arbitrarily nested changes.

Add tests for scalar-map value-type changes, object-valued map embedded
shape changes, array-of-maps shape changes, and the identical-map
no-change case, and update the evolution rules table in the docs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0116iNAHJNzoX9ynxTnfBz8H
Comment on lines +150 to +153
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 {

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants