Skip to content

fix(connectors): reject ambiguous proto field mappings - #4581

Open
ParthibanRajasekaran wants to merge 12 commits into
apache:masterfrom
ParthibanRajasekaran:fix/proto-encoder-duplicate-field-mappings
Open

ParthibanRajasekaran wants to merge 12 commits into
apache:masterfrom
ParthibanRajasekaran:fix/proto-encoder-duplicate-field-mappings

Conversation

@ParthibanRajasekaran

@ParthibanRajasekaran ParthibanRajasekaran commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR address?

Relates to #4386

Rationale

ProtoEncoderConfig.field_mappings maps protobuf field names to JSON field names. When multiple protobuf fields mapped to the same JSON name, the encoder's reverse lookup depended on HashMap iteration order and could select either field nondeterministically.

The accepted scope is limited to configuration loading. Broader runtime collision behavior and the other field-mapping configurations remain unchanged.

What changed?

Deserializing ProtoEncoderConfig now rejects field_mappings containing duplicate JSON field names. Missing, null, empty, and uniquely named mappings retain their existing behavior.

Regression tests cover duplicate rejection and each preserved configuration form.

Local Execution

  • Passed cargo fmt --all
  • Passed cargo sort --no-format --workspace
  • Passed cargo clippy -p iggy_connector_sdk --all-targets --all-features -- -D warnings
  • Passed cargo test -p iggy_connector_sdk --all-features
    • 231 unit tests
    • 10 FlatBuffer integration tests
    • 21 protobuf integration tests
  • Passed cargo build -p iggy_connector_stdout_sink -p iggy_connector_random_source
  • Passed prek run --from-ref 4f58678d^ --to-ref 4f58678d
  • Confirmed the duplicate-mapping regression test fails without the validation guard

@github-actions

Copy link
Copy Markdown

This PR is closed because its description has no Closes #N, Fixes #N, Resolves #N or Relates to #N line naming an issue.

Every PR needs an issue that was assigned to you before work started. See CONTRIBUTING.md.

@github-actions github-actions Bot closed this Oct 19, 2026
@hubcio hubcio reopened this Oct 19, 2026
@codecov

codecov Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.10345% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.89%. Comparing base (d47ff5d) to head (ae5a991).

Files with missing lines Patch % Lines
core/connectors/sdk/src/encoders/proto.rs 68.10% 32 Missing and 5 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #4581      +/-   ##
============================================
+ Coverage     80.83%   80.89%   +0.06%     
  Complexity     3107     3107              
============================================
  Files          1467     1467              
  Lines        356533   356620      +87     
  Branches     300421   300508      +87     
============================================
+ Hits         288194   288505     +311     
+ Misses        61617    61237     -380     
- Partials       6722     6878     +156     
Components Coverage Δ
Rust Core 80.20% <68.10%> (+0.08%) ⬆️
Java SDK 78.15% <ø> (ø)
C# SDK 83.60% <ø> (ø)
Python SDK 85.67% <ø> (ø)
PHP SDK 90.67% <ø> (ø)
Node SDK 96.44% <ø> (ø)
Go SDK 69.13% <ø> (ø)
C++ SDK 85.51% <ø> (ø)
Files with missing lines Coverage Δ
core/connectors/sdk/src/encoders/proto.rs 77.03% <68.10%> (+0.08%) ⬆️

... and 529 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@justinmclean justinmclean added the S-waiting-on-review PR is waiting on a reviewer label Oct 20, 2026

@ByteBaker ByteBaker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@ParthibanRajasekaran

Copy link
Copy Markdown
Contributor Author

CI is currently blocked only by an unrelated integration-test failure in Rust tests / test (1/4): server::delete_segments::should_block_deletion_until_all_consumers_pass_segment::test_case_2_restart_on_expects. It failed all four retries at core/integration/tests/server/scenarios/delete_segments_scenario.rs:832 with transient LaneBlocked values (103, 105, and 104), while the other 816 tests in that shard, all three remaining Rust shards, and the Codecov patch check passed.\n\nThis PR's diff only changes core/connectors/sdk/src/encoders/proto.rs; it does not touch the failing integration scenario. The branch is also current with master and has no conflicts. Could a maintainer please rerun the failed job? I attempted gh run rerun 40493937074 --failed, but GitHub requires repository-admin permission.

@hubcio

hubcio commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

i'll check this flaky test @ParthibanRajasekaran

@mmodzelewski mmodzelewski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: encode_should_resolve_ambiguous_reverse_lookup_to_one_of_the_colliding_keys at core/connectors/sdk/src/encoders/proto.rs:1023 still treats duplicate JSON names as valid input. Once update_config runs the check, change it to expect the rejection.

Nit: core/connectors/sdk/README.md:185 lists the encoder fields but omits field_mappings. Add a bullet that says keys are protobuf names, values are JSON names, and values must be unique.

Comment thread core/connectors/sdk/src/encoders/proto.rs
Comment thread core/connectors/sdk/src/encoders/proto.rs Outdated
Comment thread core/connectors/sdk/src/encoders/proto.rs Outdated
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Oct 27, 2026
@kparisa

kparisa commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

/author

This branch has not been deployed

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

Labels

S-waiting-on-author PR is waiting on author response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants