Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exchange and Queue name length validation #649

Closed
wants to merge 6 commits into from

Conversation

tmasternak
Copy link
Member

fixes #453

@tmasternak tmasternak requested review from ramonsmits and bording July 7, 2020 10:35
@ramonsmits ramonsmits modified the milestone: 6.0.0 Jul 7, 2020
@bording
Copy link
Member

bording commented Jul 7, 2020

I'm not sure this the approach we should take for trying to solve #453.

The catching of exceptions around the declarations I believe are still needed because there are other scenarios where those can through.

I'm also not sure the length check you added will actually catch all scenarios for length problems. The underlying AMQP protocol allows for 255 bytes, not characters, so for UTF-8, that could result in fewer characters if they are multi-byte encoded.

For the direct routing topology, there is still a missing scenario where the binding key can be too long.

Based on all of this, I think we might just need to catch the exception and rethrow a friendlier exception.

@tmasternak
Copy link
Member Author

tmasternak commented Jul 8, 2020

@bording the 6.x Rabbit Client removed the check (in WireFormatting.cs line 442) altogether.

The current behavior is that when a name gets encoded to more than 255 bytes the call will timeout on the client-side. Looking at the broker logs it seems that this is the client library issue.

In summary, the client is no longer throwing an exception that we can "wrap". Secondly, in my opinion, the current behavior is even more confusing than previously. As a result, I'd rather do the check on our end.

When it comes to

For the direct routing topology, there is still a missing scenario where the binding key can be too long.

This check needs to be done whenever we do DeclareQueue, DeclareExchange, or ExchangeBind (this includes delayed delivery routing key validation for example).

@bording bording force-pushed the entity-name-validation branch from 9cbed15 to b5d93fc Compare July 8, 2020 20:01
@bording
Copy link
Member

bording commented Jul 8, 2020

@bording the 6.x Rabbit Client removed the check (in WireFormatting.cs line 442) altogether.

The current behavior is that when a name gets encoded to more than 255 bytes the call will timeout on the client-side. Looking >at the broker logs it seems that this is the client library issue.

This is definitely a bug that should be fixed in the client: rabbitmq/rabbitmq-dotnet-client#907

At this point, I'd like to get this fixed there vs. trying to work around it in the transport.

Let's discuss what that means for this PR.

@tmasternak
Copy link
Member Author

@bording I agree that fixing that in the client is the best option.

I think we should open a PR in the client.

@tmasternak
Copy link
Member Author

PR opened in the Rabbit Client rabbitmq/rabbitmq-dotnet-client#908

@tmasternak tmasternak closed this Jul 10, 2020
@tmasternak tmasternak deleted the entity-name-validation branch July 10, 2020 09:30
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.

Throw a meaningful exception when a binding name is too long
3 participants