Better stream eventing - #537
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #537 +/- ##
===========================================
+ Coverage 90.07% 91.28% +1.21%
===========================================
Files 74 10 -64
Lines 1853 195 -1658
===========================================
- Hits 1669 178 -1491
+ Misses 184 17 -167
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
mqus
added
feature
|
I just noticed that I missed |
vitusortner
left a comment
vitusortner
left a comment
There was a problem hiding this comment.
I'll continue the review ASAP. So far, I left some questions.
| _primaryKeyColumnNames = primaryKeyColumnName, | ||
| _valueMapper = valueMapper, | ||
| _changeListener = changeListener; | ||
| _changeHandler = changeHandler ?? (() {/* do nothing */}); |
There was a problem hiding this comment.
❓ No changes necessarily needed. Only asking out of curiosity: We can avoid this default parameter by making _changeHandler nullable and calling it this way _changeHandler?.call(). What made you rely on default parameters (empty function body) instead?
There was a problem hiding this comment.
Checking for null on each invocation might incur some very minor runtime cost but apart from that I have no real reason. If you prefer this version then I'll change it.
| } | ||
| await batch.commit(noResult: true); | ||
| _changeListener?.add(_entityName); | ||
| _changeListener?.add({_entityName}); |
There was a problem hiding this comment.
❓ We now have these two approaches in our codebase where we feed into the change listener directly in the adapter (insertion_adapter) and where we feed into the change listener indirectly with the help of lambdas (update_adapter, deletion_adapter). Does it make sense to align all of them?
There was a problem hiding this comment.
Actually, the insertion_adapter also needs lambdas because of the onconflict:update issue... I thought I committed that already but it seems like I didn't...
There was a problem hiding this comment.
ah right, I didn't do this because the onConflict clause can be different for each call of insertX, altering the entities which have to be notified... And I could not decide what is best.
There are basically three main options here:
- always assume
onConflict:Updateif it occurs even once for aInsertionAdapter.insertcall - give the
InsertionAdaptertwo different sets/lambdas via the constructor and it will choose which one to use for triggering updates, based on theonConflictclause given in theInsertionAdapter.insertcall - insert the lambda/set via the invocation of the
InsertionAdapter.insertcall and not in the constructor
I'm not sure which is best here, but 1 seems to be suboptimal to me.
| /// | ||
| /// Traverses the given dependency map (Parent:[(ForeignKey:Child)]) and | ||
| /// tries to figure out which entities an update of [e] could at most change. | ||
| ///It excludes all children with a [ForeignKeyAction.noAction] and [ForeignKeyAction.restrict] |
There was a problem hiding this comment.
| ///It excludes all children with a [ForeignKeyAction.noAction] and [ForeignKeyAction.restrict] | |
| /// It excludes all children with a [ForeignKeyAction.noAction] and [ForeignKeyAction.restrict] |
| class ForeignKeyMap { | ||
| final Map<String, Map<ForeignKey, Entity>> fkDependencies; | ||
|
|
||
| ForeignKeyMap.fromEntities(Iterable<Entity> stuff) |
There was a problem hiding this comment.
| ForeignKeyMap.fromEntities(Iterable<Entity> stuff) | |
| ForeignKeyMap.fromEntities(Iterable<Entity> entities) |
…Conflict:replace)
|
following working awsome! |
3 participants
This changes the datatype of the events for streaming to
Set<String>and implements two features that benefit from that:What did not change: the handling of streams of views. They will still get updated by any changes in the database, since those dependencies can only be resolved with a query parser/analyzer.
To summarize, this will cause better stream updates for entities with FK-relationships and transaction events, and while also outputting less events than before.
It is probably best to review this commit-by-commit.
Addresses #373 partially.
Closes #360