Setting a saga data property in the constructor overrides values explicitly set previously in the saga

[kbaley]

In a ContainsSagaData class, if you have an integer or decimal property (for example, MyProperty) and explicitly set this in the class's constructor, this can override the value of the saga data if it was explicitly set to the default value earlier in the saga.

See the behaviour in a reproduction.

Summary

1. A saga has a ContainsSagaData class with a decimal property called MyValue. In the constructor for this class, the value of this property is set to decimal.MinValue.
2. Create a message FirstMessage also with a decimal property
3. Create a second message SecondMessage to end the saga
4. Create an instance of FirstMessage and set the value of the decimal property explicitly to zero
5. Send the message and in the handler, copy the contents of the decimal property to MyValue in the SagaData
6. Create an instance of SecondMessage and send it to continue the saga

Expected behaviour

MyValue in the saga data should be set to zero

Observed behaviour

MyValue in the saga data is set to decimal.MinValue

Other notes

• In step 4 above, if you set the value of the decimal property to anything other than zero, it works fine.
• If you look in the database after the saga has started but before it has completed, the JSON for the saga data does not contain a MyValue property
• This was observed in both PostgreSQL and SQL Server; I haven't tested in Oracle or MySQL

[kbaley]

On review of the code, we are explicitly setting DefaultValueHandling to Ignore rather than the default Include. This was a conscious decision to minimize the amount of data we store in the database for a saga. This means the code is working as designed but we have an option for people to override the default behaviour if needed: https://docs.particular.net/persistence/sql/saga#json-net-settings-custom-settings.

So I've reclassified this as an Improvement rather than a Bug and it can be reviewed for inclusion in a future enhancement release. Before committing to any code changes, there should be a discussion on whether the default behaviour should be changed and what the impact might be on existing sagas if we do. An argument has been made that the existing behaviour is a suitable default and that we have a documented way of changing it if this doesn't work. Plus changing it may have an unknown effect on existing sagas. Furthermore, the conditions that must be in place for this to manifest itself are not common:

• Set a property to a non-default value in a saga data's constructor, AND
• Set the same property to the data type's default value at some point during the saga

Conversely, when these conditions are met, the behaviour could be unexpected and it's questionable whether someone would even test for it. Also, this behaviour is different than other persisters (RavenDB, for example) so if someone moves from one persister to another, this may introduce bugs that aren't discovered until production.