-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Expose Tags
property in EventEnvelope
#6862
Expose Tags
property in EventEnvelope
#6862
Conversation
…ufus/akka.net into Query_Expose_tags_in_EventEnvelope
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.
One small comment, but otherwise LGTM
public EventEnvelope(Offset offset, string persistenceId, long sequenceNr, object @event, long timestamp) | ||
: this(offset, persistenceId, sequenceNr, @event, timestamp, null) |
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.
Want to use Array.Empty<string>
here instead of a nullable array?
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.
good idea
@@ -50,28 +57,30 @@ public EventEnvelope(Offset offset, string persistenceId, long sequenceNr, objec | |||
|
|||
public long Timestamp { get; } | |||
|
|||
public bool Equals(EventEnvelope other) | |||
public string[]? Tags { get; } |
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.
See previous comment about non-nullable Array.Empty<string>
@@ -59,30 +65,32 @@ public virtual void ReadJournal_query_CurrentEventsByTag_should_find_existing_ev | |||
var greenSrc = queries.CurrentEventsByTag("green", offset: NoOffset()); | |||
var probe = greenSrc.RunWith(this.SinkProbe<EventEnvelope>(), Materializer); | |||
probe.Request(2); | |||
probe.ExpectNext<EventEnvelope>(p => p.PersistenceId == "a" && p.SequenceNr == 2L && p.Event.Equals("a green apple")); | |||
probe.ExpectNext<EventEnvelope>(p => p.PersistenceId == "a" && p.SequenceNr == 4L && p.Event.Equals("a green banana")); | |||
ExpectEnvelope(probe, "a", 2L, "a green apple", "green"); |
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
@@ -188,7 +214,9 @@ public virtual void ReadJournal_query_CurrentEventsByTag_should_see_all_150_even | |||
[Fact] | |||
public void ReadJournal_query_CurrentEventsByTag_should_include_timestamp_in_EventEnvelope() | |||
{ | |||
var queries = ReadJournal as ICurrentEventsByTagQuery; | |||
if (ReadJournal is not ICurrentEventsByTagQuery queries) | |||
throw IsTypeException.ForMismatchedType(nameof(ICurrentEventsByTagQuery), ReadJournal?.GetType().Name ?? "null"); |
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 - shouldn't run these specs for journals that don't support it / should let the implementor know they've made a boo-boo.
if (SupportsTagsInEventEnvelope) | ||
{ | ||
envelope.Tags.Should().NotBeNull(); | ||
envelope.Tags.Should().Contain(tag); |
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
using static Akka.Persistence.Query.Offset; | ||
|
||
#nullable enable |
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.
👍
@Arkatufus I think you need to update API approval here |
API Approval list updated |
Nice one 👍🏻 |
Changes
close #6849
Tags
inEventEnvelope
The
Tags
property is, by default, an empty array. It is up to the plugin implementation to populate it.The built-in SQLite and in-mem journal is set to populate the property only for
EventsByTag
andCurrentEventsByTag
queries, all SQL based plugin "should" automatically inherit this. It is not populated on any other persistence queries at the moment.Full support is planned for
Akka.Persistence.Sql
plugin.Changes have been tested on
Akka.Persistence.SqlServer
andAkka.Persistence.MongoDb
to see if it breaks any API or unit test, they worked just fine.