Skip to content

A failed serialization makes StoreAsync overwrite the stored value with an empty file and report success #70

Description

@matt-edmondson

What's wrong

ISerializationProvider.Serialize discards the result of TrySerialize (Essentials/ISerializationProvider.cs, around lines 119–132):

public string Serialize(object obj)
{
    using StringWriter writer = new();
    TrySerialize(obj, writer);   // false is ignored
    return writer.ToString();    // "" on failure
}

public void Serialize(object obj, TextWriter writer) => TrySerialize(obj, writer);

The file-based persistence providers write whatever SerializeAsync returns. That covers FileSystemPersistenceProvider.StoreAsync (around line 62) and TempPersistenceProvider.StoreAsync (around line 59). DataHome and ConfigHome delegate to FileSystem, so they are affected too.

Repro

Use TempPersistenceProvider<string> over JsonSerializationProvider:

  1. StoreAsync("k", good). RetrieveAsync("k") returns good.
  2. StoreAsync("k", cyclic), where a node's Next points to itself. System.Text.Json throws JsonException, and TrySerialize returns false.
  3. StoreAsync returns normally and ExistsAsync("k") is true, but RetrieveAsync("k") now returns null.

The previous good value has been replaced with an empty file and no error was raised. The same path is hit by any NotSupportedException from STJ (a System.Type property, for example), and by a YAML or TOML serializer failure.

Why it matters

A single unserializable value silently destroys the last good persisted state. The caller has no signal that anything went wrong. This is the kind of data loss a persistence layer exists to prevent.

Suggested fix

  • Make Serialize(object) and Serialize(object, TextWriter) throw InvalidOperationException when TrySerialize returns false. This matches how the other providers' non-Try methods behave, for example Encode, Hash and Decrypt.
  • StoreAsync's existing catch then wraps the failure in PersistenceProviderException before the file is touched, so the old value survives.
  • This changes the public behaviour of Serialize, so it likely needs a [minor] or [major] version tag.

Acceptance: a test stores a good value, then attempts to store an unserializable one. The second StoreAsync throws PersistenceProviderException, and RetrieveAsync still returns the good value.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions