Remove Message base class by slang25 · Pull Request #685 · justeattakeaway/JustSaying

slang25 · 2020-05-03T13:48:37Z

A draft to start discussion around removing the JS dependency on domain messages.

I think the api would be more intuitive if we go down the route of message envelopes. You get to keep your domain message clean, and still feel like the apis are steering you with types, where as with an unbounded TMessage if feels like you could easily put any parameter in there and not find out till runtime you messed up.

slang25 · 2020-05-03T17:45:26Z

If the resulting API feels weird (and it does) it's cause at the top of the publish api we don't have a generic IMessagePublisher, that would require rework of the builder apis, but would reduce a bunch of the method level generics, and where we switch to and from object & TMessage.

slang25 · 2020-05-03T17:51:18Z

src/JustSaying/Messaging/MessageSerialization/IMessageSerialiser.cs

@@ -1,13 +1,12 @@
 using System;
-using JustSaying.Models;

 namespace JustSaying.Messaging.MessageSerialization
 {
    public interface IMessageSerializer


This whole interface should be generic and object killed from the api surface.

slang25 · 2020-05-03T17:52:01Z

src/JustSaying/Messaging/MessageSerialization/IMessageSerialisationRegister.cs

@@ -1,5 +1,3 @@
-using JustSaying.Models;
-
 namespace JustSaying.Messaging.MessageSerialization
 {
    public interface IMessageSerializationRegister


This whole interface should be generic and object killed from the api surface.

slang25 · 2020-05-03T17:59:02Z

src/JustSaying/Messaging/MessageHandling/ExactlyOnceHandler.cs


        public async Task<bool> Handle(T message)
        {
-            string lockKey = $"{message.UniqueKey()}-{_lockSuffixKeyForHandler}";


So here UniqueKey is a virtual method that defaults to Id.ToString(), I've changed it so we can optionally pass a function when we register the handler, maybe not the right place to surface the api.

slang25 · 2020-05-03T18:01:24Z

src/JustSaying/AwsTools/MessageHandling/MessageDispatcher.cs


            _logger.LogTrace(
-                "Handled message with Id '{MessageId}' of type {MessageType} in {TimeToHandle}.",
-                message.Id,


So here it is, the one place where we actually do something with a property on Message (ok, there was UniqueKey() too I guess). It's on a trace log, we arguably would want to use the SQS message id for this level of debugging, but tying it back to the domain event is going to be the challenge.

slang25 · 2020-05-03T18:53:31Z

There's a failing test because the uniqueKeySelector is missing from the new fluent api, I think it needs addressing by a different design, it's too error prone otherwise.

martincostello · 2020-05-04T07:41:50Z

src/JustSaying/AwsTools/MessageHandling/MessageHandlerWrapper.cs

@@ -49,14 +48,19 @@ private IHandlerAsync<T> MaybeWrapWithExactlyOnce<T>(IHandlerAsync<T> handler) w
                throw new Exception("IMessageLock is null. You need to specify an implementation for IMessageLock.");


We should change this to InvalidOperationException(?)

martincostello · 2020-05-04T07:42:59Z

src/JustSaying/AwsTools/MessageHandling/MessageHandlerWrapper.cs

                throw new Exception("IMessageLock is null. You need to specify an implementation for IMessageLock.");
            }

+            if (uniqueKeySelector == null)


Should throw this in the caller before we start to do any work, rather than in the delegate further along the call path.

martincostello · 2020-05-04T07:43:43Z

src/JustSaying/AwsTools/MessageHandling/SqsNotificationListener.cs

        }

-        public void AddMessageHandler<T>(Func<IHandlerAsync<T>> futureHandler) where T : Message
+        public void AddMessageHandler<T>(Func<IHandlerAsync<T>> futureHandler, Func<T, string> uniqueKeySelector = default) where T : class


If this is optional, how does it avoid causing the ArgumentNullException later? Or is this causing the test failure you mentioned?

Wow. I didn't know there was a default for Func<T, string>!

There isn't, well there is, it's just null.

Oh, haha. I don't know why I assumed otherwise.

default is just syntactic sugar added in C# 8 (at least I think it was 8, maybe a minor revision of 7) for default(Type). It's null for any reference type and the "zero" value for value types (0, 0l, etc.). The same behaviour you'd get from LINQ's FirstOrDefault()`.

martincostello · 2020-05-04T07:45:25Z

src/JustSaying/AwsTools/QueueCreation/SnsWriteConfiguration.cs

        /// </summary>
        /// <returns>Boolean indicating whether the exception has been handled</returns>
-        public Func<Exception, Message, bool> HandleException { get; set; }
+        public Func<Exception, object, bool> HandleException { get; set; }


Maybe we'd need to bite the bullet and introduce the generics you mentioned? Otherwise, using the error handler is going to be quite bleurgh from a caller having to cast/pattern-match the type all the time.

Yeah, this is the bit that persuaded me we need to do better. As it stands there are islands of generic type information, we can fix that by having more generic apis at the top level and flowing it down. A big change with this approach would be IMessagePublisher<T> and reworking the builder APIs (yet again).

Or we do the message envelope thing, which is looking more appealing. It would contain Id & Timestamp (perhaps) and a TMessage property, but other than that, is really just a way of allowing people to cleanly "pick out" their message types by defining a concrete envelope, then they get the nice typed guide rails they'd come to expect.

The fiddly bit for us with this approach is then splitting out the properties on (de)serializing, but shouldn't be too tricky.

Quite the conundrum! I'm not such a fan of the idea of IMessagePublisher<T>, although if we did choose to go down that route, we could probably easily have some sort of IAnyMessagePublisher.

I think it's worth thinking about what use there was in having this typed as Message in the first place. What sort of error handling code would we like to be able to write? If you want to write some generic error handling code then you can't write that in terms of any specific domain message type anyway. In that case you'd rather want to write a try-catch in your handler for that specific message.

That's my thoughts too @shaynevanasperen, I think a message envelope concept doesn't solve this problem, it acts as a potential way to separate the non-domain stuff from the transporty stuff.

When the base class was Message, you were either:

using Id, Tenant etc... on Message

ignoring the message and doing some generic error reporting (I think this is the primary use case)

upcasting to the known inheriting types

You could still do those last two with a message of type object, it just looks bad.

Oh, I just realised that this is for handling SNS publishing errors! I thought it was for errors in implementations of IHandlerAsync<T>. I've had a look around the code now and I see that the equivalent of this on the other side (SQS) is in SqsReadConfiguration. Over there, we work exclusively with the Amazon.SQS.Model.Message type (with no strong typing of our domain message).

So why not have symmetry with that approach and just do the equivalent here? That would mean using Amazon.SimpleNotificationService.Model.PublishRequest making the signature public Func<Exception, PublishRequest, bool> HandleException { get; set; }. The PublishRequest object has the message as a serialized string, so the error logging can easily use that. This abstraction seems like it should remain at the raw transport level, and if consumers want more control they can always wrap the IMessagePublisher this library returns with some decorator that adds custom error logging and possibly even things like a retry mechanism etc.

While looking around at the code which uses this configuration delegate, I noticed something else that we might want to do there. The error handling there only handles exceptions but won't call our error handler if the response.HttpStatusCode is not 200. I haven't checked this though (maybe the Client.PublishAsync call does throw for non-success status codes?).

martincostello · 2020-05-04T07:46:03Z

src/JustSaying/Fluent/SnsWriteConfigurationBuilder.cs

+            {
+                throw new ArgumentNullException(nameof(handler));
+            }
+            var wrappedHandler = new Func<Exception, object, bool>((ex, message) => handler(ex, (T)message));


Like here 😄

martincostello · 2020-05-04T07:46:34Z

src/JustSaying/IPublishConfiguration.cs

        int PublishFailureReAttempts { get; set; }
        TimeSpan PublishFailureBackoff { get; set; }
-        Action<MessageResponse, Message> MessageResponseLogger { get; set; }
+        Action<MessageResponse, object> MessageResponseLogger { get; set; }


Similarly here.

shaynevanasperen · 2020-05-04T18:06:46Z

src/JustSaying/AwsTools/MessageHandling/SqsNotificationListener.cs

        }

-        public void AddMessageHandler<T>(Func<IHandlerAsync<T>> futureHandler) where T : Message
+        public void AddMessageHandler<T>(Func<IHandlerAsync<T>> futureHandler, Func<T, string> uniqueKeySelector = default) where T : class


Wow. I didn't know there was a default for Func<T, string>!

shaynevanasperen · 2020-05-04T18:09:14Z

src/JustSaying/AwsTools/MessageHandling/MessageHandlerWrapper.cs

        }

-        public Func<Message, Task<bool>> WrapMessageHandler<T>(Func<IHandlerAsync<T>> futureHandler) where T : Message
+        public Func<object, Task<bool>> WrapMessageHandler<T>(Func<IHandlerAsync<T>> futureHandler, Func<T, string> uniqueKeySelector = default) where T : class


Is it not possible to just use the SNS/SQS message ID for the unique key? Why does it have to be a value in the domain message?

The original thinking behind this was that only the developer knows what determines the messages identity, and what is a duplicate. Perhaps the event publisher has OrderId and that is the real identity of the message, or a composite of OrderId and Status. Maybe it's a hash of all of the properties on the message?

I see. And with the benefit of hindsight, has that thinking paid off? Perhaps it's better to remove this "exactly once" functionality from this library and force consumers to make their solutions inherently idempotent in the domain rather than only at the message bus layer?

You know: Smart Endpoints and Dumb Pipes

shaynevanasperen · 2020-05-04T18:21:11Z

src/JustSaying/AwsTools/QueueCreation/SnsWriteConfiguration.cs

        /// </summary>
        /// <returns>Boolean indicating whether the exception has been handled</returns>
-        public Func<Exception, Message, bool> HandleException { get; set; }
+        public Func<Exception, object, bool> HandleException { get; set; }


Quite the conundrum! I'm not such a fan of the idea of IMessagePublisher<T>, although if we did choose to go down that route, we could probably easily have some sort of IAnyMessagePublisher.

I think it's worth thinking about what use there was in having this typed as Message in the first place. What sort of error handling code would we like to be able to write? If you want to write some generic error handling code then you can't write that in terms of any specific domain message type anyway. In that case you'd rather want to write a try-catch in your handler for that specific message.

slang25 · 2020-05-22T20:54:32Z

After some discussion, while this is possible it's not something we are prioritising at the moment.

Having a little pain involved in sharing types as contracts will nudge people in the direction we want, which is to declare their own type (share contracts not types).

slang25 force-pushed the remove-message-base branch from cba521d to eea63cf Compare May 3, 2020 16:34

slang25 commented May 3, 2020

View reviewed changes

Remove Message base class 🔥

c8c8546

slang25 force-pushed the remove-message-base branch from eea63cf to c8c8546 Compare May 3, 2020 17:55

slang25 commented May 3, 2020

View reviewed changes

martincostello reviewed May 4, 2020

View reviewed changes

shaynevanasperen reviewed May 4, 2020

View reviewed changes

slang25 closed this May 22, 2020

shaynevanasperen mentioned this pull request May 26, 2020

Implements a channels based consumption pipeline #698

Merged

shaynevanasperen mentioned this pull request Jun 17, 2020

Revisit the initiative to remove the message base class #721

Open

gkinsman mentioned this pull request Apr 28, 2022

Support deserialization of arbitrary payloads #1003

Open

		@@ -49,14 +48,19 @@ private IHandlerAsync<T> MaybeWrapWithExactlyOnce<T>(IHandlerAsync<T> handler) w
		throw new Exception("IMessageLock is null. You need to specify an implementation for IMessageLock.");

Conversation

slang25 commented May 3, 2020

Uh oh!

slang25 commented May 3, 2020

Uh oh!

slang25 May 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slang25 commented May 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slang25 May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slang25 commented May 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

slang25 May 3, 2020 •

edited

Loading

slang25 May 4, 2020 •

edited

Loading