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

Allow setting content-type AMQP attribute via DispatchProperties for the purposes of native integration directly via the transport seam #1468

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,8 @@ namespace NServiceBus.Transport.RabbitMQ
System.Threading.Tasks.ValueTask SetupSubscription(RabbitMQ.Client.IChannel channel, NServiceBus.Unicast.Messages.MessageMetadata type, string subscriberName, System.Threading.CancellationToken cancellationToken = default);
System.Threading.Tasks.ValueTask TeardownSubscription(RabbitMQ.Client.IChannel channel, NServiceBus.Unicast.Messages.MessageMetadata type, string subscriberName, System.Threading.CancellationToken cancellationToken = default);
}
public static class NativeIntegrationDispatchPropertiesExtensions
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick question:

This PR describes a native integration scenario, but as far as other native rabbitmq libraries are concerned it doesn't really care about the content type property at all. Is this "native integration" specific to a specific integration scenario?

It also seems that there are some other "native integration" related scenarios popping up that require some low level dispatcher / receiver configuration but the domain specific language is not aligned for the extension methods etc.

Particular/NServiceBus.Transport.AzureServiceBus#1072

{
public static void SetContentType(this NServiceBus.Transport.DispatchProperties dispatchProperties, string contentType) { }
Copy link
Contributor

Choose a reason for hiding this comment

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

By looking at the dispatch properties it means whenever a transport operation is available this method can be set. For example, it is possible to interact with PendingTransportOperations over the extension bag. If you do that the content type can be set which sets the property in the dispatch properties. Those get serialized into the outbox message which means the information is persisted into the outbox. Is that desired? If yes should this be documented at least with a comment somewhere?

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
namespace NServiceBus.Transport.RabbitMQ.TransportTests;

using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using global::RabbitMQ.Client;
using global::RabbitMQ.Client.Events;
using NServiceBus.TransportTests;
using NUnit.Framework;

public class When_setting_content_type : NServiceBusTransportTest
{
[Test]
public async Task Should_use_provided_value()
{
var onMessageInvoked = CreateTaskCompletionSource<MessageContext>();
IReadOnlyBasicProperties basicProps = null;

await StartPump(
(context, _) =>
{
var basicEventArgs = context.Extensions.Get<BasicDeliverEventArgs>();
basicProps = basicEventArgs.BasicProperties;

return onMessageInvoked.SetCompleted(context);
},
(_, __) => Task.FromResult(ErrorHandleResult.RetryRequired),
TransportTransactionMode.ReceiveOnly);

var dispatchProperties = new DispatchProperties();
dispatchProperties.SetContentType("my-content");

await SendMessage(InputQueueName, new Dictionary<string, string>
{
{"test-header", "original"}
}, null, dispatchProperties);

var messageContext = await onMessageInvoked.Task;

Assert.That(basicProps.ContentType, Is.EqualTo("my-content"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,11 @@ public static void Fill(this IBasicProperties properties, OutgoingMessage messag
}
}

if (messageHeaders.TryGetValue(NServiceBus.Headers.ContentType, out var contentType) && contentType != null)
if (dispatchProperties.TryGetValue(NativeIntegrationDispatchPropertiesExtensions.ContentTypeAttribute, out var contentType) && contentType != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

By looking at the PR description on the order of evaluation, it is unclear why dispatch properties should take precedence over the presence of the header. By looking at the code change one could argue that the now content type attribute is a temporary header (one that gets persisted into the outbox too) which the only difference that is never in the "NSB headers" but technically for integration scenarios in a raw dispatch setting the NServiceBus.Headers.ContentType would achieve the same and it is not clear why that route is not desirable.

Copy link
Contributor

Choose a reason for hiding this comment

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

By looking at the scenarios in ASB and RabbitMQ it seems that ASB has the possibility to customize the native outgoing message to the needs of the user while the rabbitmq transport doesn't provide that. Has it been considered to provide a general outgoing native message customization opportunity in the transport to unlock similar customization scenarios like ASB has that could also make it possible to override basic property values where desirable?

FYI I'm not a huge fan of the customization in ASB because it is implemented a bit in awkward ways but at least considering some alignment seems warranted

{
properties.ContentType = contentType;
}
else if (messageHeaders.TryGetValue(NServiceBus.Headers.ContentType, out contentType) && contentType != null)
{
properties.ContentType = contentType;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
namespace NServiceBus.Transport.RabbitMQ;

/// <summary>
/// Provides APIs useful for integrating with non-NServiceBus receivers.
/// </summary>
public static class NativeIntegrationDispatchPropertiesExtensions
{
internal static readonly string ContentTypeAttribute = "AMQP.content-type";

/// <summary>
/// Sets the content type to be used in the AMQP content-type field, overriding NServiceBus header.
/// </summary>
/// <param name="dispatchProperties">Dispatch properties object.</param>
/// <param name="contentType">Content type value.</param>
public static void SetContentType(this DispatchProperties dispatchProperties, string contentType)
{
dispatchProperties[ContentTypeAttribute] = contentType;
}
}