Fix broken object type serializer in QueryExecutor #6223

Arkatufus · 2022-11-01T18:40:07Z

Fix for akkadotnet/Akka.Hosting#127
Fix #6389

Changes

Removed persistence default deserializer feature

Issue

Any non-serializer mapped objects being serialized by Akka.Persistence.Sql.Common.Journal.QueryExecutor will use the JSON serializer and not the object type serializer.

Problem is with this line:

akka.net/src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/QueryExecutor.cs

Line 783 in 39a8f9e

    
           var serializer = Serialization.FindSerializerForType(e.Payload.GetType(), Configuration.DefaultSerializer);

which takes its value from the very obscure and very rarely used HOCON override here:

akka.net/src/core/Akka.Persistence/persistence.conf

Line 119 in 39a8f9e

serializer = "json"

Workaround

Override the HOCON setting:

akka.persistence.journal-plugin-fallback.serializer = null
akka.persistence.snapshot-store-plugin-fallback.serializer = null

or declare the default non-mapped object serializer in the journal/snapshot settings:

akka.persistence.journal.sqlite.serializer = mySerializer
akka.persistence.snapshot-store.sqlite.serializer = mySerializer

Arkatufus · 2022-11-01T18:41:19Z

src/contrib/persistence/Akka.Persistence.Sqlite.Tests/CustomObjectSerializerSpec.cs

+            await using var cmd = new SqliteCommand(sql, conn);
+            var record = await cmd.ExecuteReaderAsync();
+            await record.ReadAsync();
+            record[0].Should().Be(9999);


This fails because the payload is being serialized using the JSON serializer (1) and not MySerializer (9999)

Could you add a comment explaining what this test actually tests - like, what use case is it here to assert? Just need a snippet at the top explaining this.

Aaronontheweb · 2022-11-01T18:55:22Z

Nice work @Arkatufus - yep, that will do it.

So what can be done to safely change this default, given that this behavior is somewhat destructive but has also been in-place for years?

Arkatufus · 2022-11-01T19:06:34Z

To be honest, no idea... this is breaking change anyway we approach it.

Aaronontheweb · 2022-11-03T14:11:45Z

https://github.com/akkadotnet/akka.net/blame/dev/src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/QueryExecutor.cs#L815 <-- so it looks like for at least the past 5 years, we've been recording both the manifest and the serializer id in their own columns in all SQL persistence implementations. I think this means we can safely work around this default to some extent, but it will require making some backwards compatibility tests for the journal itself.

Basically, we need to try loading the explicit serializer by its ID first during reads - and on writes we should just use whatever the default object serializer is if there's not a specific match defined. If we can still read previous reads compatibly then this change is much less risky to implement.

cc @ismaelhamed

Aaronontheweb · 2022-11-03T14:13:31Z

So what would a backwards compat spec look like? Using SQLite as an example:

Write events using the current config, commit SQLite file to repository (this is key - can't produce on the fly);
Change the configuration so there's no more default fallback serializer for persistence implementations - update the read event to use the serializer ID explicitly;
Verify that all of the expected events were read and recovered correctly.

Aaronontheweb · 2022-11-03T14:13:58Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/QueryExecutor.cs

@@ -780,7 +780,7 @@ protected DbCommand GetCommand(DbConnection connection, string sql)
        protected virtual void WriteEvent(DbCommand command, IPersistentRepresentation e, IImmutableSet<string> tags)
        {

-            var serializer = Serialization.FindSerializerForType(e.Payload.GetType(), Configuration.DefaultSerializer);


Aaronontheweb · 2022-11-03T14:15:56Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/QueryExecutor.cs

@@ -846,7 +846,7 @@ protected virtual IPersistentRepresentation ReadEvent(DbDataReader reader)
            {
                // Support old writes that did not set the serializer id
                var type = Type.GetType(manifest, true);
-                var deserializer = Serialization.FindSerializerForType(type, Configuration.DefaultSerializer);


so we'd need to keep this configuration setting here for the rare cases where the data is really old and never had a serialzier ID in-place - we should revert this change specifically.

TL;DR; this is probably not the hotpath for reads on 99.999999% of events in Akka.Persistence.Sql

This Do we have a backward compat spec around this specific condition? My concern is that if someone had a different default serializer defined, will we be using the correct one here?

Exactly my question

So we'll need to find which versions of Akka.Persistence didn't include a serializerId.

It looks like we first started adding serializerIds to Akka.Persistence.Sql.Common back in 1.3.1: https://github.com/akkadotnet/akka.net/releases/tag/v1.3.1

So, we should create a Sqlite database that runs on version 1.3.0 or earlier, writes some data, commit the binary to this repo, and then replay it using the latest development version of Akka.Persistence.Sql.Common - if we can still recover that data with this change, we're in good shape.

ismaelhamed · 2022-11-04T07:52:34Z

Don't forget this also applies to the BatchingJournal.

So, what happens now for users that have set a custom serializer for persistence that is different from the default serializer?

actor {
    serializers {
        hyperion = "Akka.Serialization.HyperionSerializer, Akka.Serialization.Hyperion"
    }
    serialization-bindings {
        "System.Object" = hyperion
    }
}

akka.persistence.snapshot-store.sqlite.serializer = msgpack

Aaronontheweb · 2022-11-04T18:50:53Z

So, what happens now for users that have set a custom serializer for persistence that is different from the default serializer?

New messages will be serialized using your object serializer, old messages that were previously serialized (incorrectly) with JSON are still deserialized with the JSON serializer. It's just a write-side change if we do it the way it's laid out in the review comments.

ismaelhamed · 2022-11-05T08:14:35Z

I may be missing something, but with my example above, does it mean that messages that were being persisted with msgpack will now be serialized with hyperion?

Arkatufus · 2022-11-07T13:10:14Z

No, this is a very specific bug where if there's any types that does not have any serializer mapping, the persistence plugin will use the serializer with the name "json" or whatever is being set inside akka.persistence.journal.{persistence_plugin}.serializer, regardless of which serializer were set to handle the System.Object serialization.

I misread your comment, yes, that would mean that messages that used to be persisted using msgpack will now be persisted using hyperion

Arkatufus · 2022-11-07T14:47:01Z

This is a "breaking" change for those people that sets their akka.persistence.journal.{persistence_plugin}.serializer setting in their HOCON if the serializer is not the same as the one being mapped to System.Object AND they have used persistence since the first version where serializer id were not written to the database. Data will still be written and read correctly, but the serializer used to write the data would be different, reading really really old legacy events with no serializer id set will still work because we will still use the legacy code if and only if there are no serializer id set in the database row.

…cy data

Arkatufus · 2022-11-07T15:06:59Z

This is how very old legacy serialized data would look like.

Notice that the serializer_id column is DbNull. In this very specific case, the QueryExecutor.ReadEvent will be forced to read the data using the serializer defined in akka.persistence.{persistence_plugin}.journal.serializer setting.

Arkatufus · 2022-11-07T21:10:24Z

OK, looking at the history of why this setting is introduced in the first place, I'm in two minds about this.
This is the original reason: #2743 (comment)

Seems like removing this feature would introduce a bigger problem than it would solve.

…apshotStore`

…rkatufus/akka.net into Persistence.Sql_CustomSerializer_repro

ismaelhamed · 2022-11-08T06:42:49Z

This is a "breaking" change for those people that sets their akka.persistence.journal.{persistence_plugin}.serializer setting in their HOCON if the serializer is not the same as the one being mapped to System.Object

This is clear for me now, thanks @Arkatufus.

Aaronontheweb

Overall looks good, but we do need to harden the backwards compat test and add some additional comments for future committers to understand the significance of these tests - most of this backwards compatibility data is very "inside baseball."

Aaronontheweb · 2023-03-16T14:09:04Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/BatchingSqlJournal.cs

@@ -234,6 +234,7 @@ public abstract class BatchingSqlJournalSetup
        /// <summary>
        /// The default serializer used when not type override matching is found
        /// </summary>
+        [Obsolete(message: "This property should never be used for writes, use the default `System.Object` serializer instead")]


Aaronontheweb · 2023-03-16T14:09:18Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/BatchingSqlJournal.cs

@@ -1325,7 +1326,7 @@ private async Task<WriteMessagesResult> HandleWriteMessages(WriteMessages req, T
        protected virtual void WriteEvent(TCommand command, IPersistentRepresentation persistent, string tags = "")
        {
            var payloadType = persistent.Payload.GetType();
-            var serializer = _serialization.FindSerializerForType(payloadType, Setup.DefaultSerializer);


LGTM - this is really the key fix here.

Aaronontheweb · 2023-03-16T14:18:30Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/QueryExecutor.cs

@@ -846,7 +846,7 @@ protected virtual IPersistentRepresentation ReadEvent(DbDataReader reader)
            {
                // Support old writes that did not set the serializer id
                var type = Type.GetType(manifest, true);
-                var deserializer = Serialization.FindSerializerForType(type, Configuration.DefaultSerializer);


So we'll need to find which versions of Akka.Persistence didn't include a serializerId.

It looks like we first started adding serializerIds to Akka.Persistence.Sql.Common back in 1.3.1: https://github.com/akkadotnet/akka.net/releases/tag/v1.3.1

So, we should create a Sqlite database that runs on version 1.3.0 or earlier, writes some data, commit the binary to this repo, and then replay it using the latest development version of Akka.Persistence.Sql.Common - if we can still recover that data with this change, we're in good shape.

Aaronontheweb · 2023-03-16T14:19:12Z

src/contrib/persistence/Akka.Persistence.Sql.Common/Snapshot/SqlSnapshotStore.cs

@@ -224,22 +224,5 @@ protected override async Task DeleteAsync(string persistenceId, SnapshotSelectio
                await QueryExecutor.DeleteBatchAsync(connection, nestedCancellationTokenSource.Token, persistenceId, criteria.MaxSequenceNr, criteria.MaxTimeStamp);
            }
        }
-
-        private SnapshotEntry ToSnapshotEntry(SnapshotMetadata metadata, object snapshot)


Why was this removed?

Aaronontheweb · 2023-03-16T14:19:57Z

src/contrib/persistence/Akka.Persistence.Sqlite.Tests/CustomObjectSerializerSpec.cs

+            await using var cmd = new SqliteCommand(sql, conn);
+            var record = await cmd.ExecuteReaderAsync();
+            await record.ReadAsync();
+            record[0].Should().Be(9999);


Could you add a comment explaining what this test actually tests - like, what use case is it here to assert? Just need a snippet at the top explaining this.

Aaronontheweb · 2023-03-16T14:20:41Z

src/contrib/persistence/Akka.Persistence.Sqlite.Tests/CustomObjectSerializerSpec.cs

+        }
+
+        [Fact(DisplayName = "Persistence.Sql should be able to read legacy data")]
+        public void LegacyDataTest()


So to make this cover the issue referenced here: https://github.com/akkadotnet/akka.net/pull/6223/files#r1012961321 - we just need even older data, correct?

Aaronontheweb · 2023-03-16T14:24:58Z

Also, you'll need to add an entry to the upgrade advisories page for this change too @Arkatufus

Arkatufus · 2023-03-16T15:23:29Z

Closing this issue, moving the changes to dev branch instead of v1.4.
PR moved to #6528

Reproduction unit test for akkadotnet/Akka.Hosting#127

75e24b3

Arkatufus commented Nov 1, 2022

View reviewed changes

Arkatufus added 2 commits November 2, 2022 02:20

Clarify the unit test

a0b5e8f

Remove persistence default serializer feature

3cf2eee

Arkatufus changed the title ~~Reproduction unit test for broken object type serializer in QueryExecutor~~ Fix broken object type serializer in QueryExecutor Nov 2, 2022

Aaronontheweb requested changes Nov 3, 2022

View reviewed changes

Arkatufus added 2 commits November 7, 2022 20:52

Revert changes on EventRead, add legacy data read test

b1daa66

Update API Verify list

800e7b1

Revert persistence.conf changes, modify sqlite database to mimic lega…

f3fd0e9

…cy data

Merge branch 'v1.4' into Persistence.Sql_CustomSerializer_repro

b4e9e67

Arkatufus added 2 commits November 8, 2022 04:20

Extend DefaultSerializer removal to BatchingSqlJournal and `SqlSn…

02b3f44

…apshotStore`

Merge branch 'Persistence.Sql_CustomSerializer_repro' of github.com:A…

623085e

…rkatufus/akka.net into Persistence.Sql_CustomSerializer_repro

Merge branch 'v1.4' into Persistence.Sql_CustomSerializer_repro

014a4cc

Aaronontheweb requested changes Mar 16, 2023

View reviewed changes

Aaronontheweb added serialization akka-persistence-sql-common Akka.Persistence.Sql.Common labels Mar 16, 2023

Merge branch 'v1.4' into Persistence.Sql_CustomSerializer_repro

1b6c2d0

Arkatufus mentioned this pull request Mar 16, 2023

Fix broken object type serializer in QueryExecutor #6528

Merged

Arkatufus closed this Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix broken object type serializer in QueryExecutor #6223

Fix broken object type serializer in QueryExecutor #6223

Arkatufus commented Nov 1, 2022 •

edited by Aaronontheweb

Loading

Arkatufus Nov 1, 2022

Aaronontheweb Mar 16, 2023

Aaronontheweb commented Nov 1, 2022

Arkatufus commented Nov 1, 2022

Aaronontheweb commented Nov 3, 2022

Aaronontheweb commented Nov 3, 2022

Aaronontheweb Nov 3, 2022

Aaronontheweb Nov 3, 2022

Aaronontheweb Nov 3, 2022

to11mtm Nov 5, 2022

ismaelhamed Nov 7, 2022

Aaronontheweb Mar 16, 2023

ismaelhamed commented Nov 4, 2022 •

edited

Loading

Aaronontheweb commented Nov 4, 2022

ismaelhamed commented Nov 5, 2022

Arkatufus commented Nov 7, 2022 •

edited

Loading

Arkatufus commented Nov 7, 2022 •

edited

Loading

Arkatufus commented Nov 7, 2022 •

edited

Loading

Arkatufus commented Nov 7, 2022

ismaelhamed commented Nov 8, 2022

Aaronontheweb left a comment

Aaronontheweb Mar 16, 2023

Aaronontheweb Mar 16, 2023

Aaronontheweb Mar 16, 2023

Aaronontheweb Mar 16, 2023

Aaronontheweb Mar 16, 2023

Aaronontheweb Mar 16, 2023

Aaronontheweb commented Mar 16, 2023

Arkatufus commented Mar 16, 2023

Fix broken object type serializer in QueryExecutor #6223

Fix broken object type serializer in QueryExecutor #6223

Conversation

Arkatufus commented Nov 1, 2022 • edited by Aaronontheweb Loading

Changes

Issue

Workaround

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb commented Nov 1, 2022

Arkatufus commented Nov 1, 2022

Aaronontheweb commented Nov 3, 2022

Aaronontheweb commented Nov 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ismaelhamed commented Nov 4, 2022 • edited Loading

Aaronontheweb commented Nov 4, 2022

ismaelhamed commented Nov 5, 2022

Arkatufus commented Nov 7, 2022 • edited Loading

Arkatufus commented Nov 7, 2022 • edited Loading

Arkatufus commented Nov 7, 2022 • edited Loading

Arkatufus commented Nov 7, 2022

ismaelhamed commented Nov 8, 2022

Aaronontheweb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb commented Mar 16, 2023

Arkatufus commented Mar 16, 2023

Arkatufus commented Nov 1, 2022 •

edited by Aaronontheweb

Loading

ismaelhamed commented Nov 4, 2022 •

edited

Loading

Arkatufus commented Nov 7, 2022 •

edited

Loading

Arkatufus commented Nov 7, 2022 •

edited

Loading

Arkatufus commented Nov 7, 2022 •

edited

Loading