Skip to content

fix: consistent hashing and equality for RabbitMQ schemas#2796

Open
RinZ27 wants to merge 4 commits intoag2ai:mainfrom
RinZ27:fix/rmq-schemas-identity-final
Open

fix: consistent hashing and equality for RabbitMQ schemas#2796
RinZ27 wants to merge 4 commits intoag2ai:mainfrom
RinZ27:fix/rmq-schemas-identity-final

Conversation

@RinZ27
Copy link
Copy Markdown

@RinZ27 RinZ27 commented Mar 5, 2026

Description

Consistent hashing and equality logic for RabbitMQ schemas (Queue, Exchange, and Channel). These changes allow schema objects to be used correctly in sets or as dictionary keys, which is essential for the internal declarer cache and prevents redundant declarations.

The robust parameter is now excluded from identity checks (__eq__ and __hash__) as requested, ensuring that connection-level properties do not affect the schema's unique identity.

Relates to #2791 (replacement for the closed PR)

Type of change

  • Bug fix (a non-breaking change that resolves an issue)
  • New feature (a non-breaking change that adds functionality)

Checklist

  • My code adheres to the style guidelines of this project
  • I have conducted a self-review of my own code
  • I have made the necessary changes to the documentation
  • My changes do not generate any new warnings
  • I have added tests to validate the effectiveness of my fix or the functionality of my new feature
  • Both new and existing unit tests pass successfully on my local environment
  • I have ensured that static analysis tests are passing
  • I have included code examples to illustrate the modifications

@RinZ27 RinZ27 requested a review from Lancetnik as a code owner March 5, 2026 02:58
@RinZ27 RinZ27 force-pushed the fix/rmq-schemas-identity-final branch from f1d1cd2 to d383d1a Compare March 5, 2026 03:00
@RinZ27 RinZ27 force-pushed the fix/rmq-schemas-identity-final branch from d383d1a to f1bacfa Compare March 6, 2026 03:19
@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Mar 6, 2026
@RinZ27
Copy link
Copy Markdown
Author

RinZ27 commented Mar 6, 2026

Explicitly defined __eq__ for the Channel schema to ensure consistency with the updated hashing logic.

Modified test_subscriber_use_shared_channel to use a unique channel_number for sub2. Since the new hash logic correctly identifies default Channel() instances as identical (which is the goal of this PR), providing a distinct property is now required to verify that the broker can still handle multiple independent channels when configured to do so.

Additionally, included the missing on: push trigger in the CodeQL workflow to resolve the baseline security scanning alerts previously seen in the actions tab.

@RinZ27 RinZ27 requested a review from Sehat1137 as a code owner March 8, 2026 00:57
@Lancetnik
Copy link
Copy Markdown
Member

@RinZ27 did you run tests and linter locally?

@Lancetnik
Copy link
Copy Markdown
Member

Anyway, we are strongly tied to current hash and eq logic and I don't see a reason to change it. It's a sensitive place and could leads to breaking changes. Do we have a real edge case required such changes? I suggest to close the PR if we haven't

@RinZ27 RinZ27 force-pushed the fix/rmq-schemas-identity-final branch from 606905d to 1f9c245 Compare March 31, 2026 15:30
@RinZ27
Copy link
Copy Markdown
Author

RinZ27 commented Mar 31, 2026

@Lancetnik I've verified the changes with ruff and ran the schema tests locally (everything is green).

During my research, I noticed the current implementation violates the hash-equality invariant. Specifically, RabbitExchange was using a name-only __eq__ from NameRequired while its __hash__ factored in durable, type, and other params. This makes caching in RabbitDeclarerImpl inconsistent and potentially prone to subtle bugs when the same exchange name is used with different configurations across different modules.

Using id(self) for Channel hashing means identical configurations result in multiple redundant channels being opened on the broker. By moving to value-based equality, I've ensured that subscribers with the same QoS requirements share a single channel effectively.

Logic for both Exchange and Queue now includes routing_key in the identity. This ensures that different bindings for the same resource are treated as distinct configurations in the declarer, which aligns with the goal of allowing multiple identical publishers/subscribers. I also updated the tests to reflect these changes.

@Lancetnik
Copy link
Copy Markdown
Member

Using id(self) for Channel hashing means identical configurations result in multiple redundant channels being opened on the broker.

Yes, it was designed this way. And now we can't change the behavior because all existing code based on that behavior

@RinZ27 RinZ27 force-pushed the fix/rmq-schemas-identity-final branch from 1f9c245 to 51a4380 Compare March 31, 2026 15:46
@RinZ27
Copy link
Copy Markdown
Author

RinZ27 commented Mar 31, 2026

@Lancetnik reverted Channel to its original identity-based behavior to maintain compatibility.

However, I've kept the changes for RabbitExchange and RabbitQueue. As mentioned before, they currently violate the Python hash-equality invariant (name-only __eq__ but multi-field __hash__). This leads to inconsistent caching in RabbitDeclarerImpl. For example, two exchanges with the same name but different durable settings would currently be treated as equal by __eq__ but have different hashes, which is a bug in any dict-based cache.

By fixing the Exchange/Queue identity logic (and including routing_key), we ensure the cache is reliable and align with the future goal of allowing multiple distinct publishers/subscribers for the same resource. I've also updated the tests to match these changes.

@Lancetnik
Copy link
Copy Markdown
Member

Logic for both Exchange and Queue now includes routing_key in the identity. This ensures that different bindings for the same resource are treated as distinct configurations in the declarer, which aligns with the goal of allowing multiple identical publishers/subscribers.

We shouldn't check routing_key while tracking Exchange / Queue identity. It's a binding option and has no effect for Exchange itself. Declarer doesn't rely on it while tracking identity

@Lancetnik
Copy link
Copy Markdown
Member

I mean all tests now - are pinned behavior. Current behavior was designed to cover such behavior. Any changes requires tests updates - breaking changes.

@Lancetnik
Copy link
Copy Markdown
Member

I am OK to align __eq__ logic with actual hashing. But can't merge any changes in hash functions without examples of incorrect behavior (bugreports)

@RinZ27 RinZ27 force-pushed the fix/rmq-schemas-identity-final branch from 51a4380 to 9e57353 Compare March 31, 2026 16:05
@RinZ27
Copy link
Copy Markdown
Author

RinZ27 commented Mar 31, 2026

@Lancetnik Reverted routing_key from the identity logic for both Exchange and Queue, agree it's a binding option rather than an intrinsic property of the object itself.

I've kept the changes to align __eq__ with the existing multi-field __hash__ implementation (excluding routing_key). This fixes the Python hash-equality invariant (ensuring a == b => hash(a) == hash(b)) without changing the actual hashing behavior or logic used by the declarer.

All schema tests are passing locally and match the current design's expectations, updated the tests to reflect these reverts.

@RinZ27 RinZ27 force-pushed the fix/rmq-schemas-identity-final branch from 9e57353 to 398ee78 Compare April 1, 2026 11:37
@RinZ27 RinZ27 force-pushed the fix/rmq-schemas-identity-final branch from 398ee78 to c30d003 Compare April 1, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

github_actions Pull requests that update GitHub Actions code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants