Skip to content

Conversation

Aaronontheweb
Copy link
Member

Fixes #665

Summary

The Adapters property on JournalOptions created confusion by providing two competing patterns for configuring event adapters. This PR deprecates the property in favor of the unified callback pattern introduced in PR #662.

Problem

Having two ways to configure event adapters was:

  • ❌ Confusing - No clear guidance on which pattern to use
  • ❌ Inconsistent - Health checks only support callbacks
  • ❌ Undocumented - Users could use both simultaneously with unclear merge semantics
  • ❌ Dangerous - Property initialized with dummy builder that could cause NRE

Solution

  1. Mark property as [Obsolete] with clear migration guidance pointing to issue Deprecate JournalOptions.Adapters property in favor of unified callback API #665
  2. Remove internal usage - Stop reading the property in JournalOptions.Build()
  3. Add regression test - Verify deprecated property is ignored and callbacks work

Changes

  • JournalOptions.cs:78 - Added [Obsolete] attribute to Adapters property
  • JournalOptions.cs:105 - Removed Adapters.AppendAdapters(sb) call
  • EventAdapterSpecs.cs:134-207 - Added DeprecatedAdaptersPropertySpec regression test

Benefits

✅ Single, consistent pattern for configuring adapters and health checks
✅ Cleaner API surface - options contain only configuration data
✅ Clear migration path with deprecation warning
✅ No silent failures - deprecated property is truly ignored

Migration Example

Before (deprecated):

var journalOptions = new SqlJournalOptions(true)
{
    Adapters = new AkkaPersistenceJournalBuilder("sql", builder)
};
journalOptions.Adapters.AddWriteEventAdapter<Foo>("foo", [typeof(Bar)]);

builder.WithJournal(journalOptions);

After (recommended):

var journalOptions = new SqlJournalOptions(true);

builder.WithJournal(
    journalOptions,
    journal => journal.AddWriteEventAdapter<Foo>("foo", [typeof(Bar)]));

Testing

All 17 persistence hosting tests pass, including new regression test that verifies:

  • Deprecated property is ignored (doesn't configure adapters)
  • Callback pattern works correctly
  • Uses #pragma warning disable to test deprecated API without warnings

Note

This is Phase 1 of the deprecation plan. The property will be removed entirely in v1.6.0.

Fixes akkadotnet#665

The Adapters property created confusion by providing two competing
patterns for configuring event adapters. This deprecates the property
in favor of the unified callback pattern introduced in PR akkadotnet#662.

Changes:
- Mark JournalOptions.Adapters as [Obsolete] with clear migration message
- Remove internal usage of the property (Adapters.AppendAdapters call)
- Add regression test DeprecatedAdaptersPropertySpec that verifies:
  * Deprecated property is ignored and does not configure adapters
  * Callback pattern continues to work correctly
  * Uses #pragma warning disable to test deprecated API

Benefits:
- Single, consistent pattern for configuring adapters and health checks
- Cleaner API surface with options containing only configuration data
- Clear migration path with deprecation warning

Migration:
Before (deprecated):
  var options = new JournalOptions();
  options.Adapters.AddWriteEventAdapter<Foo>(...);
  builder.WithJournal(options);

After (recommended):
  var options = new JournalOptions();
  builder.WithJournal(options, journal =>
    journal.AddWriteEventAdapter<Foo>(...));

All 17 tests pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate JournalOptions.Adapters property in favor of unified callback API
1 participant