Nservicebus: Prevent using Saga ID as correlation property

[timbussmann]

It is possible to specify the saga id as a correlation property, e.g:

        protected override void ConfigureHowToFindSaga(SagaPropertyMapper<MultiTimeoutsSaga1Data> mapper)
        {
            mapper.ConfigureMapping<StartSaga1>(m => m.ContextId)
                .ToSaga(s => s.Id);
        }

at first glance, this might work for existing sagas where a message contains the saga id as a property. But as we require all mappings to use the same correlation id for a saga, this means that you'd have to use the saga id also as the correlation property with messages starting a saga.
Therefore that doesn't look like a valid scenario, as we assign a new saga id when creating a new saga and users might interfere with that logic.

cc @andreasohlund @DavidBoike

[andreasohlund]

Edge case but you can use a custom finder for the message starting the saga and "SagaId" mappings for the other messages. I can't come up with a valid use case for this though!

[DavidBoike]

What if you had nested sagas, and because in that situation auto-correlation won't work, you send the Outer saga id as part of the message to the Inner saga, and then the response message from the Inner saga includes the OuterSagaId as a property?

Is that a valid use case?

If so perhaps the rule should be relaxed to allow a single correlation property as well as mappings to the sagaId, which just wouldn't count. Those would have to be detected correctly and use GetBySagaId(sagaId), rather than GetByProperty("SagaId", sagaId).

[andreasohlund]

That might be a use case but I'm a bit on the fence still:

• The autocorrelation issue is IMO a bug on our end
• When I've had similar situations in the past I've always felt it more natural to pass the correlation id in the request so that the endpoint responding can put that on the reponse message. Using the saga id seems like a uneeded hack since you already have a better id to us?

[danielmarbach]

We will check whether we could implicitly implement the IContainsSagaDatainterface so that the Id is no longer visible in IntelliSense without casting.

[danielmarbach]

It is not possible to do the trick outlined in the previous comment. The ContainsSagaData class cannot explicitly implement the interface since the properties need to be marked as virtual for NHibernate. I played around with the following:

    public abstract class ContainSagaData : IContainSagaData
    {
        [EditorBrowsable(EditorBrowsableState.Never)]
        public virtual Guid Id { get; set; }

        [EditorBrowsable(EditorBrowsableState.Never)]
        public virtual string Originator { get; set; }

        [EditorBrowsable(EditorBrowsableState.Never)]
        public virtual string OriginalMessageId { get; set; }
    }

this would remove the properties from IntelliSense for all those that inherit from ContainSagaData and don't expose the editor browsable stuff via resharper.

For direct implementors of IContainSagaData adding the [EditorBrowsable(EditorBrowsableState.Never)] to the interface doesn't work since the declared properties on the saga data wouldn't inherit automatically those attributes. We could add to to the ContainSagaData class as a helpful way to guide users in the right direction but it is not an generic solution for the problem. Thoughts?

[timbussmann]

it won't address the issue, but I think it would be a good change to add the attributes mentioned by @danielmarbach 👍

[danielmarbach]

@Particular/nservicebus-maintainers thoughts on https://github.com/Particular/NServiceBus/issues/4344#issuecomment-264101496 ?

[DavidBoike]

Aside from the attribute, I think it comes down to two options:

1. Prevent mapping a saga id completely, throwing an exception if you try.
2. Allow mapping a saga id, don't throw during the check for a single correlation property (so single correlation property + sagaId), and make sure to use the correct method during lookup.

First option seems like painting ourselves in a corner which we might regret. Second is more permissive but I can't think of any strong reason to slap the user on the wrist for doing it.

[andreasohlund]

Most of the time mapping the ID is a misstake and you can workaround using a custom finder so I'm voting for the hard line (aka no.1)

[danielmarbach]

Plus one for option 1

[DavidBoike]

So then the PoA is:

1. Add EditorBrowsable attributes on ContainSagaData
2. Throw if user attempts to map to the Id property, but allow from a custom finder.

Does point 2 constitute a breaking change?

[andreasohlund]

Does point 2 constitute a breaking change?

I'd say this is in the greyzone, not from a api perspective but definitely a change in behavior.

Is workaround with the custom finder is low friction enough to force the (very few I'd say) affected users to go down that path?

[timbussmann]

Is workaround with the custom finder is low friction enough to force the (very few I'd say) affected users to go down that path?

you mean to apply the mentioned changes in a minor instead of a major version or anyway?

[andreasohlund]

you mean to apply the mentioned changes in a minor instead of a major version or anyway

I was thinking if we can do this in a minor

[timbussmann]

I'm not sure there is enough value in addressing this in a minor? Let's schedule it for v7 and just go with the breaking change?

[andreasohlund]

Fair enough, I'll apply the label

[timbussmann]

played around with some validation logic, there is an edge case which is not super trivial to solve which is using explicit interface implementations like this:

        public class SagaData : IContainSagaData
        {
            Guid IContainSagaData.Id { get; set; }
            string IContainSagaData.Originator { get; set; }
            string IContainSagaData.OriginalMessageId { get; set; }
            public Guid Id { get; set; }
        }

this case is hard to identify, as it would be valid to map to the Id property in this case. I'm not even sure this is really supported by persisters tbh.
I'd prefer to not go with an approach which completely forbids the usage of an "Id" property in mappings therefore (although that would work in most cases) and propose to go with the editorbrowsable attributes in v7 and backlog the rest for a later milestone. Thoughts @Particular/nservicebus-maintainers

[timbussmann]

after talking to @Particular/nservicebus-maintainers we agreed that, while it would be possible to handle the scenario using explicit interface implementations, but it's probably not worth the reflection effort to do so. We'd like to address this issue with the new saga API instead and therefore removing this issue from the v7 milestone.

[CrispyDrone]

@timbussmann

Hello, I'm new to nservicebus. I found this issue because it seemed logical for me to map the correlation id to the saga id. Do I understand correctly that when mapping the correlation id to the saga id the issue boils down to:

1. nservicebus creates a saga with a saga id = correlation id
2. the saga is marked as complete
3. a new message with the same correlation id comes in after the saga is completed so a new saga needs to be created
4. the saga id remains constant as the correlation id hasn't changed
5. some internal storage issue such as duplicate key exception?

A natural follow-up question is: when nservicebus creates a new saga instance (after having completed one with the same correlation id), does the new saga instance have access to data from the previous saga instance or not? I would presume no.

Thanks in advance for your reply.

[SzymonPobiega]

Hi @CrispyDrone

When you use the code similar to the one in the description, the NServiceBus will treat the Id field as storages for correlation Id when you start a new saga instance. Suppose you use order ID from you domain as saga correlation because the goal of the saga is to handle orders. The OrderId field from the incoming message would be assigned to the Id property of the saga instance but after that happens, NServiceBus would overwrite that value with guid-based infrastructure saga id. Because of that you would lose the original value of order id and when another message comes and should be correlated to the same instance of the saga, it cannot be matched because the Id field of the saga contains that infrastructure guid instead of order id.

[CrispyDrone]

Hi @SzymonPobiega

Sorry for the late response; thanks a lot for your reply! That makes sense.