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

Handle the body is undefined from AMQP 1.0 to AMQP 091 #6835

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

Gsantomaggio
Copy link
Member

@Gsantomaggio Gsantomaggio commented Jan 10, 2023

Fixes part of #6837

If the body is undefined, the payload returns an empty binary.
Signed-off-by: Gabriele Santomaggio [email protected]

This PR fixes a bug in case the native stream client sends a message without a body and an AMQP client reads the message.
example:

 Producer producer = environment.producerBuilder()
                .stream(stream)
                .name("reference")//
                .build();
        for (int i = 0; i < 20; i++) {
            producer.send(
                    producer.messageBuilder().publishingId(i).properties().messageId(i).messageBuilder().build(),

then try to read in AMQP:

channel.basic_qos(prefetch_count=100)  # mandatory
channel.basic_consume(
    queue=q_name,
    on_message_callback=callback,
    arguments={
        'x-stream-offset': 'first'
        #   'x-stream-offset': 15
        # with first start consuming always from the beginning
    },
    auto_ack=False)
channel.start_consuming()

The server raises:

2023-01-03 16:58:44.428652+00:00 [warning] <0.2303.0> Non-AMQP exit reason '{function_clause,
2023-01-03 16:58:44.428652+00:00 [warning] <0.2303.0>                        [{rabbit_msg_record,to_amqp091,
2023-01-03 16:58:44.428652+00:00 [warning] <0.2303.0>                          [{rabbit_msg_record,
2023-01-03 16:58:44.428652+00:00 [warning] <0.2303.0>                            {cfg},
2023-01-03 16:58:44.428652+00:00 [warning] <0.2303.0>                            {msg,undefined,

This because:

to_amqp091(#?MODULE{msg = #msg{properties = P,
application_properties = APR,
message_annotations = MAR,
data = #'v1_0.data'{content = Payload}}}) ->

Does not consider the empty data

Proposed Changes

With this fix, the function returns an empty binary <<>> When the body message is not defined.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

@Gsantomaggio
Copy link
Member Author

thanks to @gpad for reporting the bug

If the body is undefined the payload return empty binary

Signed-off-by: Gabriele Santomaggio <[email protected]>

remove comments

Signed-off-by: Gabriele Santomaggio <[email protected]>
@lukebakken lukebakken force-pushed the amqp10_to91_properties branch from d889de0 to ceb78b7 Compare January 10, 2023 20:48
@Gsantomaggio Gsantomaggio marked this pull request as ready for review January 12, 2023 14:09
@Gsantomaggio Gsantomaggio changed the title Handle the body is undefined when from AMQP 1.0 to AMQP 091 Handle the body is undefined from AMQP 1.0 to AMQP 091 Jan 12, 2023
@michaelklishin michaelklishin merged commit 43b78aa into main Jan 12, 2023
@michaelklishin michaelklishin deleted the amqp10_to91_properties branch January 12, 2023 20:35
michaelklishin added a commit that referenced this pull request Jan 12, 2023
Handle the body is undefined from AMQP 1.0 to AMQP 091 (backport #6835)
michaelklishin added a commit that referenced this pull request Jan 12, 2023
Handle the body is undefined from AMQP 1.0 to AMQP 091 (backport #6835) (backport #6869)
acogoluegnes added a commit to rabbitmq/rabbitmq-stream-java-client that referenced this pull request Jan 13, 2023
github-actions bot pushed a commit to rabbitmq/rabbitmq-stream-java-client that referenced this pull request Jan 13, 2023
@ansd
Copy link
Member

ansd commented May 13, 2024

Note that each AMQP message must contain a body:

The body consists of one of the following three choices: one or more data sections, one or more amqp-sequence sections, or a single amqp-value section.

@Gsantomaggio
Copy link
Member Author

@ansd Yes.
We add this fix to make the stream (amqp 10)--> amqp (amqp 091) compatible.

We could add this constraint on the client side or handle it on the server side but with better error handling.

acogoluegnes added a commit to rabbitmq/rabbitmq-stream-java-client that referenced this pull request May 13, 2024
Null message body is forbidden in the AMQP 1.0 specification.
We set a data section with an empty binary when the message
body has not been set.

Stream messages are not checked on the server side, but
messages from other protocols will be checked for a non-null
body in RabbitMQ 4.0, and letting null body stream messages
through will crash when those messages are read with other protocols.

References rabbitmq/rabbitmq-server#6835
Fixes #544
github-actions bot pushed a commit to rabbitmq/rabbitmq-stream-java-client that referenced this pull request May 13, 2024
Null message body is forbidden in the AMQP 1.0 specification.
We set a data section with an empty binary when the message
body has not been set.

Stream messages are not checked on the server side, but
messages from other protocols will be checked for a non-null
body in RabbitMQ 4.0, and letting null body stream messages
through will crash when those messages are read with other protocols.

References rabbitmq/rabbitmq-server#6835
Fixes #544
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.

4 participants