Move Raven ID out of persisence contract - #5705
Conversation
0dc2f39 to
735dd16
Compare
rbev
changed the title
| class NotificationsSettingsDocument | ||
| { | ||
| public string Id { get; set; } | ||
| public EmailNotifications Email { get; set; } = new(); |
There was a problem hiding this comment.
Should Raven have its own version of EmailNotifications?
There was a problem hiding this comment.
That depends, if we are going to just serialise EmailNotifications as the type in the SQL settings table then they will effectively be the same.
I'm not opposed to it, but EmailNotifications is a pretty simple POCO and unlikely to change significantly so I don't know how much value will be gained from replicating it in each persistence along with object-object mapping.
johnsimons
left a comment
johnsimons
left a comment
There was a problem hiding this comment.
LG but Raven entity is still using the model, not sure if that is ok.
735dd16 to
caf1b04
Compare
| throw new NotImplementedException(); | ||
|
|
||
| public void Dispose() | ||
| public ValueTask DisposeAsync() |
There was a problem hiding this comment.
Should this be public ValueTask DisposeAsync() => default;
Therre is no need for GC.SuppressFinalize(this);
2 participants
Description
The current Notifications data store is stored as a singleton document in RavenDB, and is returned directly from the INotificationsManager. The Id field on this document is not used or relevant to consumers of this interface.
To remove this field from the contract this change implements a new internal class for Raven to persist, and removes the ID field from the returned object.
Testing Improvements:
NotificationsDataStoreTestswith comprehensive tests to verify default values, round-trip persistence, field updates, and field preservation for notification settings. (src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs)