Skip to content

Better handling for described encoding#45878

Open
fafhrd91 wants to merge 8 commits intoAzure:mainfrom
fafhrd91:described
Open

Better handling for described encoding#45878
fafhrd91 wants to merge 8 commits intoAzure:mainfrom
fafhrd91:described

Conversation

@fafhrd91
Copy link
Copy Markdown
Member

Description

  1. Current implementation drops "descriptor" for described types.
    This pr keeps descriptor as "descriptor" attribute.
  2. Fix decoding for array of described types

All SDK Contribution checklist:

  • [*] The pull request does not introduce [breaking changes]
  • [ *] CHANGELOG is updated for new features, bug fixes or other significant changes.
  • [ *] I have read the contribution guidelines.

Copilot AI review requested due to automatic review settings March 24, 2026 17:51
@fafhrd91 fafhrd91 requested review from axisc, hmlam and sjkwak as code owners March 24, 2026 17:51
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Improves AMQP described-type decoding in the Event Hubs pyamqp stack by preserving the descriptor on decoded values and fixing decoding for arrays whose element type is described.

Changes:

  • Adds described-value wrapper types (e.g., DescribedList) to retain a .descriptor attribute on decoded values.
  • Updates array decoding to correctly handle arrays of described element types (small + large).
  • Extends unit tests to cover described values and arrays of described values.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
sdk/eventhub/azure-eventhub/tests/pyamqp_tests/unittest/test_decode.py Adds tests for described decoding and described arrays.
sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/described.py Introduces wrapper subclasses used to attach .descriptor to decoded values.
sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/_decode.py Updates described decoding and array decoding; introduces constructor→wrapper mapping.
Comments suppressed due to low confidence (1)

sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/_decode.py:325

  • In _decode_described_array, _COMPOSITES maps descriptor -> string (e.g., "accepted"), but the code uses cast(int, _COMPOSITES[descriptor]) and stores it in composite_type. This cast is incorrect/misleading for type checking and makes the variable name confusing. Update the cast/type annotation (likely str) and/or rename the variable to reflect it holds a composite name/key, not an int type code.
    try:
        composite_type = cast(int, _COMPOSITES[descriptor])
        return buffer, {composite_type: value}
    except KeyError:

Comment on lines +305 to +308
try:
value = _DESCR_BY_CONSTRUCTOR[tp](value, descriptor=descriptor)
except KeyError:
pass
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

This only attaches .descriptor for constructors listed in _DESCR_BY_CONSTRUCTOR; for any other described underlying type (e.g., uuid/decimal128/bool/null), the descriptor is still dropped via the KeyError path. If the intent is to preserve the descriptor for all described values (per PR description), consider adding a generic fallback wrapper that stores (descriptor, value) when a specialized subclass wrapper is not available.

Copilot uses AI. Check for mistakes.
@kashifkhan
Copy link
Copy Markdown
Member

/azp run python - eventhub - tests

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.

3 participants