Skip to content

Fix field count encoding in DescribedList and add tests for full field count compliance#645

Open
kamil-mrzyglod wants to merge 1 commit into
Azure:masterfrom
kamil-mrzyglod:feature/fixed-numeric-index
Open

Fix field count encoding in DescribedList and add tests for full field count compliance#645
kamil-mrzyglod wants to merge 1 commit into
Azure:masterfrom
kamil-mrzyglod:feature/fixed-numeric-index

Conversation

@kamil-mrzyglod

Copy link
Copy Markdown

DescribedList.EncodeValue() previously stopped encoding at the last non-null field, which is permitted by AMQP 1.0 spec section 1.4 but breaks non-.NET clients (Python _pyamqp, uamqp) that access performative fields by fixed numeric index — causing IndexError when the encoded list is shorter than expected.

This change replaces the last-set-bit calculation with this.fieldCount, so all defined fields are always written (unset ones as null bytes). This matches the behaviour of the real Azure Service Bus / Event Hubs broker.

References: TheCloudTheory/Topaz#210.

@xinchen10

Copy link
Copy Markdown
Member

Since this is permitted by AMQP 1.0 spec, should we fix the other clients instead?

@kamil-mrzyglod

Copy link
Copy Markdown
Author

@xinchen10 I am not in a position to influence the direction, but IMHO fixing other clients isn't the solution. Yes, AMQP spec allows the existing behaviour but other SDKs fail to comply with AMQPNet for a reason. To me the proposed fix is a much better solution - it fixes the issue centrally, is simple and the change is strictly additive.

@xinchen10

Copy link
Copy Markdown
Member

Thanks for bringing up the issue. I am reaching out to the SDK owners to get it fixed there. Azure SDKs should be protocol compliant. AMQP is a standard and has more clients than just Azure SDKs. The Apache qpid-proton-j does same compact encoding of composite list types.

@kamil-mrzyglod

Copy link
Copy Markdown
Author

Thanks @xinchen10 :)

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.

2 participants