test: rust differ - #543
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesThe pull request adds a Rust ADC SDK and a Rust Differ V4 implementation. It adds shared fixtures, integration tests, fixture comparison tools, benchmark data, a mock Admin API server, and synchronization benchmarks. Rust Differ V4
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant FixtureRunner
participant DifferV4
participant ResourceMetadata
participant EventOutput
FixtureRunner->>DifferV4: Load local, remote, and default values
DifferV4->>ResourceMetadata: Resolve resource fields and identifiers
ResourceMetadata-->>DifferV4: Return merge and nesting metadata
DifferV4->>DifferV4: Compute resource and nested events
DifferV4->>EventOutput: Sort and serialize events
EventOutput-->>FixtureRunner: Return fixture result map
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
rust/crates/adc-differ/src/differ_v4.rs (1)
21-27: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid deep-cloning
DefaultValueon every recursive diff call.
DifferV4::diffclones the wholeDefaultValueinto the new differ instance.handle_delete,handle_create, andhandle_updatecallDifferV4::diffagain for each resource item, so the clone repeats once per item that has nested fields. For a config with many services and routes, this copies the full core and plugin default maps many times. Hold a borrow or aCowinstead.♻️ Proposed refactor using a borrowed default value
-pub struct DifferV4 { - default_value: DefaultValue, -} +pub struct DifferV4<'a> { + default_value: std::borrow::Cow<'a, DefaultValue>, +} -impl DifferV4 { +impl<'a> DifferV4<'a> { pub fn diff( local: &InternalConfiguration, remote: &InternalConfiguration, - default_value: Option<&DefaultValue>, + default_value: Option<&'a DefaultValue>, parent_name: Option<&str>, ) -> Vec<Event> { - let differ = DifferV4 { default_value: default_value.cloned().unwrap_or_default() }; + let differ = DifferV4 { + default_value: default_value + .map(std::borrow::Cow::Borrowed) + .unwrap_or_else(|| std::borrow::Cow::Owned(DefaultValue::default())), + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/adc-differ/src/differ_v4.rs` around lines 21 - 27, Update DifferV4::diff and the DifferV4 default_value field to borrow the existing DefaultValue (or use Cow) instead of calling cloned().unwrap_or_default() on every recursive invocation. Adjust handle_delete, handle_create, and handle_update recursion to reuse that borrowed/default reference while preserving the existing fallback behavior when no default value is provided.rust/crates/adc-sdk/src/differ_meta.rs (1)
50-51: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider returning static metadata instead of building it per call.
differ_metaallocates a newResourceDifferMetaon every call.strip_nested_idsinrust/crates/adc-differ/src/differ_v4.rs(lines 333-336) calls it once per candidate type for every nested field, so the cost multiplies with config size. A&'static ResourceDifferMetaper variant, or aLazyLocklookup table keyed byconfig_field, removes both the allocation and the linear scan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/crates/adc-sdk/src/differ_meta.rs` around lines 50 - 51, Update differ_meta to return reusable static ResourceDifferMeta instances per ResourceType variant instead of constructing metadata on every call. Preserve the existing variant-to-metadata mapping and adjust callers such as strip_nested_ids to use the static reference without repeated allocation or linear lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fixtures/differ/basic.update_resource.json`:
- Around line 1-4: The generic fixture basic.update_resource.json duplicates
basic.update_resource_add_plugin.json; update basic.update_resource.json to
represent the generic update scenario or remove it if no longer needed, while
leaving the distinct add-plugin fixture unchanged.
In `@libs/differ/tools/dump-fixture-results.ts`:
- Around line 20-21: Update the fixture directory initialization near
FIXTURES_DIR to remove the machine-specific absolute fallback. Resolve the
repository’s fixtures/differ directory relative to import.meta.url, or require
ADC_DIFFER_FIXTURES_DIR when it is unset, while preserving the existing
environment-variable override.
In `@rust/crates/adc-differ/src/bin/run_fixtures.rs`:
- Around line 67-72: Update fixture discovery in the entries collection to
propagate errors from directory iteration instead of silently discarding them
via filter_map(|e| e.ok()). Make the runner fail with context identifying
fixtures_dir when an entry cannot be read, while preserving the existing JSON
path filtering and collection behavior.
In `@rust/crates/adc-differ/src/differ_v4.rs`:
- Around line 206-256: Redact ConsumerCredential secret fields before Event
values are exposed or serialized: ensure config.password and config.secret are
removed from both old_value and new_value in the differ flow around event
construction. Apply the same sanitization for create, update, and delete events,
then add fixture tests in run_fixtures.rs covering all three event types and
verifying secrets are absent from serialized output.
In `@rust/crates/adc-mock-server/src/main.rs`:
- Line 54: Update the port parsing in main so the default 18899 is used only
when no argument is supplied; when an argument is present but cannot be parsed
as u16, return a clear error instead of falling back. Preserve successful
explicit port values.
In `@rust/crates/adc-sdk/src/value_diff.rs`:
- Around line 33-52: Update every path field in the ValueDiff variants New,
Deleted, Edit, and Array with serde’s empty-vector skip rule so empty root paths
are omitted during serialization, while non-empty paths remain serialized.
In `@rust/crates/adc-sync-bench/src/main.rs`:
- Around line 113-115: Validate the benchmark arguments before loading the
fixture: update the parsing around concurrency, iterations, and runtime_flavor
to report invalid numeric input instead of unwrapping, reject zero concurrency
and iterations, and accept only “current” or “multi” runtime names. Preserve the
existing defaults for omitted arguments and ensure invalid values terminate
before fixture loading.
In `@scripts/compare-differ-fixtures.mjs`:
- Around line 94-101: Update the failure-reporting loop over failures so the TS
and normalized Rust diagnostic payloads are recursively redacted before
JSON.stringify and console.log output. Ensure credentials, authentication
headers, and event values cannot appear in terminal or CI logs; omit resource
bodies entirely if no redaction helper is available.
- Around line 19-21: Update scripts/compare-differ-fixtures.mjs to create a
unique private temporary directory per parity run, pass its TypeScript result
path to Vitest, and use its Rust result path for run_fixtures instead of fixed
/tmp filenames. Update libs/differ/tools/dump-fixture-results.ts so its output
path is required or generated as a private per-process file, never defaulting to
a shared predictable /tmp path.
---
Nitpick comments:
In `@rust/crates/adc-differ/src/differ_v4.rs`:
- Around line 21-27: Update DifferV4::diff and the DifferV4 default_value field
to borrow the existing DefaultValue (or use Cow) instead of calling
cloned().unwrap_or_default() on every recursive invocation. Adjust
handle_delete, handle_create, and handle_update recursion to reuse that
borrowed/default reference while preserving the existing fallback behavior when
no default value is provided.
In `@rust/crates/adc-sdk/src/differ_meta.rs`:
- Around line 50-51: Update differ_meta to return reusable static
ResourceDifferMeta instances per ResourceType variant instead of constructing
metadata on every call. Preserve the existing variant-to-metadata mapping and
adjust callers such as strip_nested_ids to use the static reference without
repeated allocation or linear lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c94076d-e627-4696-9f7c-035c8f1f835a
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (77)
.gitignorefixtures/differ/basic.adapts_to_default_core_values.jsonfixtures/differ/basic.adapts_to_default_plugin_values.jsonfixtures/differ/basic.boolean_defaults_merged_correctly.jsonfixtures/differ/basic.create_resource.jsonfixtures/differ/basic.delete_resource.jsonfixtures/differ/basic.empty_input_yields_empty_output.jsonfixtures/differ/basic.generates_hashed_resource_id.jsonfixtures/differ/basic.keeps_plugins_when_plugins_not_changed.jsonfixtures/differ/basic.merges_array_nested_object_defaults_correctly.jsonfixtures/differ/basic.route_and_stream_route_ids_generated_correctly.jsonfixtures/differ/basic.selectively_merges_objects_in_default_values.jsonfixtures/differ/basic.sorted_by_event_type.jsonfixtures/differ/basic.update_resource.jsonfixtures/differ/basic.update_resource_add_plugin.jsonfixtures/differ/basic.update_resource_update_plugin_with_default_value.jsonfixtures/differ/basic.updates_service_and_its_nested_route.jsonfixtures/differ/basic.updates_service_nested_route.jsonfixtures/differ/consumer.creates_updates_deletes_consumer_credentials.jsonfixtures/differ/consumer.deletes_consumer_credentials_when_consumer_is_deleted.jsonfixtures/differ/custom_id.deletes_and_creates_new_resource_when_id_changes.jsonfixtures/differ/regression.does_not_apply_stream_service_default_to_http_service.jsonfixtures/differ/regression.resolves_stream_service_default_type_correctly.jsonfixtures/differ/service_upstream.creates_non_default_upstreams.jsonfixtures/differ/service_upstream.creates_service_and_upstream.jsonfixtures/differ/service_upstream.deletes_non_default_upstreams.jsonfixtures/differ/service_upstream.replaces_non_default_upstreams.jsonfixtures/differ/service_upstream.unchanged_service_with_default_and_named_upstreams.jsonfixtures/differ/service_upstream.unchanged_service_with_only_default_upstream.jsonfixtures/differ/service_upstream.updates_default_upstream.jsonfixtures/differ/service_upstream.updates_non_default_upstreams.jsonfixtures/differ/upstream.creates_and_updates_ssl_before_upstream.jsonfixtures/differ/usecase.renames_service_with_nested_routes.jsonfixtures/differ/usecase.selectively_merges_objects_in_default_values_on_a_service.jsonlibs/differ/package.jsonlibs/differ/tools/dump-fixture-results.tslibs/differ/vitest.fixtures.config.tsrust/Cargo.tomlrust/benches/fixtures/large.few.local.jsonrust/benches/fixtures/large.many.local.jsonrust/benches/fixtures/large.none.local.jsonrust/benches/fixtures/large.remote.jsonrust/benches/fixtures/medium.few.local.jsonrust/benches/fixtures/medium.many.local.jsonrust/benches/fixtures/medium.none.local.jsonrust/benches/fixtures/medium.remote.jsonrust/benches/fixtures/small.few.local.jsonrust/benches/fixtures/small.many.local.jsonrust/benches/fixtures/small.none.local.jsonrust/benches/fixtures/small.remote.jsonrust/crates/adc-differ/Cargo.tomlrust/crates/adc-differ/benches/differ_bench.rsrust/crates/adc-differ/examples/gen_fixtures.rsrust/crates/adc-differ/src/bin/run_fixtures.rsrust/crates/adc-differ/src/differ_v4.rsrust/crates/adc-differ/src/lib.rsrust/crates/adc-differ/tests/basic.rsrust/crates/adc-differ/tests/consumer.rsrust/crates/adc-differ/tests/custom_id.rsrust/crates/adc-differ/tests/fixtures_sanity.rsrust/crates/adc-differ/tests/regression.rsrust/crates/adc-differ/tests/service_upstream.rsrust/crates/adc-differ/tests/upstream.rsrust/crates/adc-differ/tests/usecase.rsrust/crates/adc-mock-server/Cargo.tomlrust/crates/adc-mock-server/src/main.rsrust/crates/adc-sdk/Cargo.tomlrust/crates/adc-sdk/src/differ_meta.rsrust/crates/adc-sdk/src/event.rsrust/crates/adc-sdk/src/field_meta.rsrust/crates/adc-sdk/src/lib.rsrust/crates/adc-sdk/src/resource.rsrust/crates/adc-sdk/src/utils.rsrust/crates/adc-sdk/src/value_diff.rsrust/crates/adc-sync-bench/Cargo.tomlrust/crates/adc-sync-bench/src/main.rsscripts/compare-differ-fixtures.mjs
This was referenced Aug 1, 2026
Draft
Merged
Merged
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
1 participant
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.
Description
Add a Rust differ test suite that matches the TS version test suite.
Checklist
Summary by CodeRabbit
New Features
Tests
Chores