fix: deduplicate leaf record common types when objects_as_records is enabled#134
Open
tomjwxf wants to merge 2 commits into
Open
fix: deduplicate leaf record common types when objects_as_records is enabled#134tomjwxf wants to merge 2 commits into
tomjwxf wants to merge 2 commits into
Conversation
…enabled
When both deduplicate_entity_types and objects_as_records are true,
identical leaf record objects across tools are now consolidated into a
shared common type at the LCA namespace, instead of each tool getting
its own duplicate common type definition.
Previously, LeafRecord fingerprints were explicitly stripped from the
deduplication map when objects_as_records was enabled. This made sense
when objects_as_records implied records shouldn't participate in
entity-type dedup, but left record-type dedup unsupported.
Changes:
- Remove the blanket LeafRecord filter in deduplicate_entities()
- Place deduplicated LeafRecords as common types (via add_commontype)
when objects_as_records is true, entity types otherwise
- Update collision detection to check existing common types for matching
record shapes and reuse them when identical
- Enable dedup lookup in cedar_type_from_property_type() regardless of
the objects_as_records setting, returning EntityOrCommon references
for records
Before (bug): duplicate per-tool common types
namespace MyMcpServer::tool_a::Input {
type metadata = { author: String, version?: Long };
}
namespace MyMcpServer::tool_b::Input {
type metadata = { author: String, version?: Long };
}
After (fix): shared LCA common type
namespace MyMcpServer {
type metadata = { author: String, version?: Long };
type tool_aInput = { metadata?: MyMcpServer::metadata, ... };
type tool_bInput = { metadata?: MyMcpServer::metadata, ... };
}
Closes cedar-policy#126
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 76.47% Status: FAILED ❌ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 90.66% Status: PASSED ✅ Details
|
Add unit tests for reusing or skipping dedup when the LCA already has a common type with the same name. Also keep tool_a::Input alive in the objects_as_records dedup test so the local-namespace assertion runs.
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 98.31% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 91.77% Status: PASSED ✅ Details
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends
deduplicate_entity_typesto cover leaf records encoded as common types whenobjects_as_recordsis enabled. No new config flag — the existing dedup pipeline gains a second emission target.Closes #126
Problem
When both
deduplicate_entity_typesandobjects_as_recordsaretrue, identical leaf record objects across tools produced duplicate common type definitions instead of being consolidated:Solution
Changes
LeafRecordfilterresolved.retain()was stripping all record fingerprints whenobjects_as_records=trueadd_commontype()LeafRecords emit as common types whenobjects_as_records=trueEntityOrCommoncedar_type_from_property_type()no longer skips dedup lookup withobjects_as_recordsTests
dedup_leaf_record_objects_as_recordsdedup_leaf_record_objects_as_records_flatdedup_leaf_record_different_required_objects_as_recordsdedup_enum_record_name_conflict_objects_as_recordsAll 92 tests pass (62 unit + 30 integration), 0 warnings.