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

[AMQP] Fix Filter Set Encoding For 2 Char Length Session id #32860

Merged
merged 13 commits into from
Nov 13, 2023

Conversation

kashifkhan
Copy link
Member

Fix for the case when a session id was of length 2. There was a logic error in encode that would try to do try/except on the passed in data and improperly split the values leading to an error from the service

The link contains invalid filter type. System only support Jms or Apache selector filter type but we found type 'Microsoft.ServiceBus.Messaging.Amqp.Encoding.DescribedType' associated with key 'com.microsoft:session-filter'.

  • if session id is (6, 671, 6712, .....) , line 769 raises a ValueError which gets handled on line 774 and described_filter = 671 as excepted
  • if session id is of length 2 ( 67, ka ) line 769 unpacks making descriptor = 6, filter_value = 7 , making the value borked

The fix was to make sure strs or bytes would not be attempted to be unpacked.

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@kashifkhan
Copy link
Member Author

/azp run python - servicebus - tests

@kashifkhan
Copy link
Member Author

/azp run python - eventhub - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@kashifkhan
Copy link
Member Author

/azp run python - eventhub - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kashifkhan
Copy link
Member Author

/azp run python - eventhub - tests

@kashifkhan
Copy link
Member Author

/azp run python - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kashifkhan
Copy link
Member Author

/azp run python - eventhub - tests

@kashifkhan
Copy link
Member Author

/azp run python - servicebus - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@l0lawrence l0lawrence left a comment

Choose a reason for hiding this comment

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

lgtm! Can we add a unittest just to track the change?

Copy link
Member

@swathipil swathipil left a comment

Choose a reason for hiding this comment

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

tests? 🙂

@kashifkhan
Copy link
Member Author

/azp run python - servicebus - tests

@kashifkhan
Copy link
Member Author

/azp run python - eventhub - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kashifkhan
Copy link
Member Author

/azp run python - servicebus - tests

@kashifkhan
Copy link
Member Author

/azp run python - eventhub - tests

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kashifkhan kashifkhan merged commit 09f0ab0 into Azure:main Nov 13, 2023
28 checks passed
@kashifkhan kashifkhan deleted the peek_error branch December 14, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants