-
Notifications
You must be signed in to change notification settings - Fork 22
Fix event adapter callback API not invoking adapters at runtime #674
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix event adapter callback API not invoking adapters at runtime #674
Conversation
Fixed a critical bug where event adapters configured via the NEW callback API (journalBuilder callback parameter in WithJournal/WithJournalAndSnapshot) were not being invoked at runtime, despite being correctly present in HOCON configuration. Root Cause: AkkaPersistenceJournalBuilder.Build() was creating a Config with .WithFallback(Persistence.DefaultConfig()) which interfered with the main journal configuration during HOCON merging, causing adapters to be registered but not invoked. The Fix: Removed the unnecessary .WithFallback(Persistence.DefaultConfig()) call in AkkaPersistenceHostingExtensions.cs line 130. The adapter configuration blocks (event-adapters and event-adapter-bindings) don't require fallback to default persistence config and merge correctly with the journal config without it. Impact: This bug affected users with: - Multiple journal configurations (e.g., default + sharding) - NEW callback API: journalBuilder: journal => journal.AddWriteEventAdapter<...>() - Explicit JournalOptions + WithJournalAndSnapshot pattern This fix is especially critical now that the old Adapters property has been deprecated in akkadotnet#665. All users must use the callback API which was broken. Testing: - Added comprehensive regression test suite (EventAdapterRuntimeInvocationSpecs.cs) - Tests verify adapter runtime invocation with various configuration patterns - All 20 Akka.Persistence.Hosting.Tests pass - Verified backwards compatibility with existing EventAdapterSpecs Related Issues: - Fixes runtime invocation issue reported in akkadotnet/Akka.Persistence.Sql#552 - Critical fix for akkadotnet#665 callback API requirement
89c8372 to
f7ca2ab
Compare
|
Updated tests to use |
Aaronontheweb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detailed my changes - I want to improve the tests but the substantial changes are good IMHO
| /// This is a regression test for https://github.com/akkadotnet/Akka.Persistence.Sql/issues/552 | ||
| /// where event adapters were present in HOCON but never invoked at runtime. | ||
| /// </summary> | ||
| public class EventAdapterRuntimeInvocationSpecs : Akka.Hosting.TestKit.TestKit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reproduces akkadotnet/Akka.Persistence.Sql#552 using the in-memory journal.
|
|
||
| public sealed class InvocationCountingAdapter : IWriteEventAdapter | ||
| { | ||
| public static int CallCount = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Static fields shouldn't / won't work if there are multiple tests all running in the same fixture. Probably should come up with something else here, like an Akka.Persistence.Query replay.
| adapters.AppendLine("}"); | ||
|
|
||
| var finalHocon = ConfigurationFactory.ParseString(adapters.ToString()) | ||
| .WithFallback(Persistence.DefaultConfig()); // add the default config as a fallback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the real bug - the default HOCON getting prepended to the front of the configuration causes the event-adapters collection to be empty.
- Remove complex EventsByTag query approach (in-memory journal doesn't support this) - Use simple static counter with proper resets for test isolation - Each test explicitly resets the counter at the start - Maintains same verification: adapters are invoked at runtime - Keeps all original test scenarios from issue akkadotnet#552 reproduction Addresses akkadotnet#674 (comment)
- Remove static CallCount field that caused concurrency issues in tests - Simplify test to use Akka.Persistence.Query with EventsByTag instead of static counters - Fix hardcoded AkkaVersion "1.5.53" to use $(AkkaVersion) variable in csproj - Add Akka.Persistence.Query.InMemory package dependency - Remove overly complex multi-journal test scenarios, focusing on single clear test case This properly verifies that event adapters configured via the callback API are invoked at runtime by tagging events and querying for them.
Verification Complete ✅Tested this fix using a local NuGet build (v1.5.52-alpha-1760450556) against the reproduction tests in akkadotnet/Akka.Persistence.Sql#554. Test ResultsNEW Callback API (the affected API from issue #552):
Both tests that use the new callback API pattern now pass with this fix. The sharding journal test specifically reproduces the exact scenario from akkadotnet/Akka.Persistence.Sql#552 where event adapters configured via OLD Deprecated API:
The old ConclusionThis fix successfully resolves akkadotnet/Akka.Persistence.Sql#552. Event adapters configured via the new callback API now invoke correctly at runtime in multi-journal configurations. The root cause fix (removing the |
Arkatufus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Fixed a critical bug where event adapters configured via the NEW callback API were not being invoked at runtime, despite being correctly present in HOCON configuration.
Root Cause
AkkaPersistenceJournalBuilder.Build()was creating a Config with.WithFallback(Persistence.DefaultConfig())which interfered with the main journal configuration during HOCON merging, causing adapters to be registered in config but not invoked during persistence operations.File:
src/Akka.Persistence.Hosting/AkkaPersistenceHostingExtensions.csline 130The Fix
Removed the unnecessary
.WithFallback(Persistence.DefaultConfig())call. The adapter configuration blocks (event-adaptersandevent-adapter-bindings) don't require fallback to default persistence config and merge correctly with the journal config without it.Impact
This bug affected users with:
journalBuilder: journal => journal.AddWriteEventAdapter<...>()JournalOptions+WithJournalAndSnapshotpattern (common in Akka.Persistence.Sql)This fix is especially critical now that the old
Adaptersproperty has been deprecated in #665. All users must now use the callback API which was broken without this fix.Testing
EventAdapterRuntimeInvocationSpecs.cs)Akka.Persistence.Hosting.Testspass ✅EventAdapterSpecs✅Test Evidence
Before Fix
After Fix
Related Issues
Changes
src/Akka.Persistence.Hosting/AkkaPersistenceHostingExtensions.cs(removed 1 line)src/Akka.Persistence.Hosting.Tests/EventAdapterRuntimeInvocationSpecs.cs(254 lines, 3 test classes)