feat: add batch support to Ecto projections - #30
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds batch-processing support to Ecto projections: a Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Projector as Projector (project_batch / update_projection_batch)
participant Multi as Ecto.Multi
participant Repo as Repo (DB)
participant Callback as after_update_batch (post-commit)
Caller->>Projector: update_projection_batch(events, multi_fn)
Projector->>Projector: validate batch format & options
Projector->>Projector: filter unseen events (watermark)
Projector->>Multi: build changes via multi_fn
Projector->>Multi: include watermark update
Multi->>Repo: run transaction
Repo-->>Multi: success / error
Multi-->>Projector: transaction result
alt success
Projector->>Callback: invoke after_update_batch(events, changes) (post-commit)
Callback-->>Projector: :ok | {:error, reason}
Projector-->>Caller: :ok
else error
Projector-->>Caller: {:error, reason}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Dynamic Schema Prefix Callback Silent Failure
When schema_prefix is configured as a 1-arity or 2-arity function, the generated schema_prefix/1 callback always returns nil instead of calling the configured function. This breaks the documented callback pattern where users can define schema_prefix/1 to dynamically determine schema prefixes. When a function-based prefix is used, schema_prefix(event) should invoke the configured function, not return nil.
lib/commanded/projections/ecto.ex#L494-L502
91ccb24 to
a538e90
Compare
There was a problem hiding this comment.
Bug: Configured function returns `nil` unexpectedly.
When schema_prefix is configured as a static binary (string), the generated 1-arity schema_prefix/1 function incorrectly returns nil instead of the configured prefix string. The 2-arity version correctly returns the string. This inconsistency violates the callback contract where both versions should return the same value. While the current code only calls the 2-arity version, this creates a subtle bug that could cause incorrect behavior if the 1-arity version is ever used directly.
lib/commanded/projections/ecto.ex#L495-L499
a538e90 to
60a2e1f
Compare
60a2e1f to
84d49b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/commanded/projections/ecto.ex (1)
204-267: Optional invariant check that all events share the same handler_nameThe implementation intentionally derives
projection_namefrom the first event’s metadata and uses it for the entire batch, with comments explaining that all events in a batch come from the same handler process and are enriched with the same:handler_name.Even if that invariant holds within the Commanded dispatcher, it might be useful to defensively assert it for callers who invoke
handle_batch/1directly (as in tests), e.g. by checking that all events ineventsshare the samehandler_nameasfirst_event_metadata.This isn’t strictly required—misconfigured metadata already constitutes misuse—but it would turn a subtle idempotency bug into a clear error if someone accidentally mixes handlers in a batch.
🧹 Nitpick comments (7)
guides/howtos/building-read-models-with-ecto.md (2)
84-137: Clarify:subscription_optsvs top‑level:concurrencyhandlingThe description of
:concurrencyand its mutual exclusivity with:batch_sizeis good, but note that the implementation inCommanded.Projections.Ecto.validate_mutual_exclusivity/1only checks the top‑level:concurrencyoption, notsubscription_opts[:concurrency]. Ifconcurrencyis set only inside:subscription_optstogether with:batch_size, the current validation won’t catch it.Consider either:
- Documenting that
:concurrencyshould be set at the top level when used, or- Extending the implementation to also look inside
:subscription_optsfor:concurrencyso the docs and behavior stay aligned.
176-179: Apply hyphenation fix: “built‑in”The sentence “there is currently no built in way” should use the hyphenated form “built‑in” (“no built‑in way”) per standard English usage and the static analysis hint.
lib/commanded/projections/ecto.ex (1)
90-121: Batch + dynamic schema_prefix guard is correct but only covers option‑based prefixes
validate_batch_schema_prefix_compatibility/2correctly preventsbatch_sizefrom being combined with a functional:schema_prefixoption or app config, enforcing “static string” prefixes for batch projectors in that configuration style.Note that this guard does not (and cannot easily) detect dynamic schema prefixes implemented via the
schema_prefix/1orschema_prefix/2callbacks. That’s fine so long as we treat callbacks as an escape hatch and keep the docs clear that batch support only guarantees correctness for static prefix options.No code change strictly required, but a short note in the guides that dynamic
schema_prefixcallbacks are not supported for batch projectors would reduce confusion.test/projections/commanded_batch_integration_test.exs (1)
12-115: Integration tests nicely exercise handle_batch/1 and batch watermarkingThis module does a good job of validating the new batch path end‑to‑end:
IntegrationBatchProjectorusesproject_batch fn events, multi -> ... endwithbatch_size: 5andEcto.Multi.insert_all/4, matching the intended API.- Tests assert that
handle_batch/1is exported, that projections are inserted as expected, and thatProjectionVersion.last_seen_event_numbertracks the highest event number seen.- The idempotency test (
handle_batchcalled twice with the same events) aligns with the:already_seen_eventbranch inupdate_projection_batch/2and verifies no duplicate projections are written.One optional enhancement would be a test where the second batch partially overlaps the first (some events already seen, some new) to exercise the “filter unseen events within a batch” behavior, but the current coverage is already strong.
test/projections/ecto_projection_batch_test.exs (3)
34-37: Consider asserting the Sandbox checkout result in setup
setup/1correctly startsTestApplicationand checks out the Repo connection. To fail fast if the Repo or Sandbox is misconfigured, you could pattern-match on the return value:setup do start_supervised!(TestApplication) - Sandbox.checkout(Repo) + :ok = Sandbox.checkout(Repo) end
39-112: Good coverage ofhandle_batch/1; consider avoiding DB order assumptions in assertionsThese four tests do a nice job exercising batch handling for multiple events, mixed event types, partial replays, and fully replayed batches, and they validate the projection watermark via
assert_seen_event/2.One potential robustness issue:
assert_projections/2(intest/support/projection_assertions.ex) compares the list fromRepo.all(schema) |> pluck(:name)directly toexpected. Without an explicitORDER BY, DBs are free to return rows in any order, which could make these expectations brittle if row-return order ever changes.If ordering isn’t semantically important, consider updating
assert_projections/2to be order-independent (or to add an explicit ordering), for example:def assert_projections(schema, expected) do actual = schema |> Repo.all() |> pluck(:name) |> Enum.sort() assert actual == Enum.sort(expected) endThis would make these new batch tests (and any existing users of
assert_projections/2) resilient to DB ordering differences.
162-201: Callback-with-changes test is valuable but tightly coupled to internal keysUsing
BatchProjectorCallbackWithChangesto assert thatafter_update_batch/2sees thechangesmap is great, but asserting specific keys like:lock_and_filter,:track_projection_version, and:prepare_user_multitightly couples this test to internal step naming insideupdate_projection_batch/2.If you mainly care that the callback receives the final projection result plus some internal metadata, you could relax the assertions slightly, e.g. by asserting that:
changesis a map,- it has a
:projectionentry, and- optionally that it’s non-empty,
while avoiding dependence on the exact internal step names. That would keep this test useful without constraining future refactors of the batch pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.formatter.exs(1 hunks)guides/howtos/building-read-models-with-ecto.md(4 hunks)lib/commanded/projections/ecto.ex(8 hunks)test/projections/commanded_batch_integration_test.exs(1 hunks)test/projections/ecto_projection_batch_test.exs(1 hunks)test/projections/projection_version_schema_prefix_test.exs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/projections/commanded_batch_integration_test.exs (2)
lib/commanded/projections/ecto.ex (1)
handle_batch(665-667)test/support/projection_assertions.ex (1)
assert_projections(7-11)
lib/commanded/projections/ecto.ex (3)
test/projections/projection_version_schema_prefix_test.exs (1)
schema_prefix(341-344)test/projections/ecto_projection_batch_test.exs (4)
after_update_batch(139-145)after_update_batch(178-184)after_update_batch(219-221)after_update_batch(250-252)test/projections/after_update_callback_test.exs (1)
after_update(19-25)
test/projections/ecto_projection_batch_test.exs (2)
lib/commanded/projections/ecto.ex (1)
handle_batch(665-667)test/support/projection_assertions.ex (1)
assert_projections(7-11)
🪛 LanguageTool
guides/howtos/building-read-models-with-ecto.md
[grammar] ~178-~178: Use a hyphen to join words.
Context: .... Note that there is currently no built in way to target a single type of event ...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Quality Assurance (1.18.x, 27)
🔇 Additional comments (15)
guides/howtos/building-read-models-with-ecto.md (1)
304-347: after_update/3 and after_update_batch/2 transaction semantics docs match implementationThe new “
⚠️ Transaction Semantics” sections correctly describe that bothafter_update/3andafter_update_batch/2run after the DB transaction commits and cannot roll back persisted changes. This matches the implementations inupdate_projection/3andupdate_projection_batch/2, where the callbacks are invoked only aftertransaction/1succeeds.No changes needed here; the guidance about restricting these callbacks to side‑effects is accurate and helpful.
lib/commanded/projections/ecto.ex (6)
199-355: Batch watermarking, locking, and idempotency logic look soundThe new
update_projection_batch/2implementation has several strong properties:
- Empty batches short‑circuit to
:ok, keeping them trivially idempotent.- Batches are validated to be
{event, metadata}tuples, and metadata must include both:handler_nameand:event_number. Malformed batches are rejected with a clear{:invalid_batch_structure, ...}error.:lock_and_filterusesSELECT ... FOR UPDATEonProjectionVersionkeyed byprojection_name, ensuring a single process owns the watermark during a transaction.current_last_seenis fetched inside that lock; unseen events are filtered withmetadata.event_number > current_last_seen, and an:already_seen_eventoutcome is treated as a no‑op.:track_projection_versionadvances the watermark to the highest unseen event via an upsert with a defensiveWHERE pv.last_seen_event_number < ^last_event_numberclause.- The user’s
multi_fnis required to return anEcto.Multi; anything else yields{:invalid_multi_return, other}.- All of the above—including watermark updates—run in a single
Repo.transaction/2. If the user’s projection fails, the entire transaction (watermark + projections) rolls back, keeping state consistent.- Idempotency behavior for replayed batches is explicitly handled in the
{:error, :lock_and_filter, :already_seen_event, _}branch by returning:ok.This matches the expectations from the new tests in
test/projections/ecto_projection_batch_test.exsandcommanded_batch_integration_test.exs. No changes needed here.
311-347: after_update_batch/2 invocation and error handling semanticsThe
withbranch for batches:
- Builds and runs the full
Ecto.Multitransaction, including watermark advancement and user projections.- Only after
transaction(multi)returns{:ok, changes}does it callafter_update_batch(unseen_events, changes).Because
after_update_batch/2is defined in the quoted__using__/1block with a default:okimplementation and markeddefoverridable, user projectors can override it. Any{:error, reason}or raised exception propagates out as the return value or error, while the persisted data remains committed—matching the documented transaction semantics.This callback wiring is correct and consistent with the tests for
after_update_batch/2behavior.
356-375: Default callbacks + @optional_callbacks are wired correctlyProviding default implementations:
def after_update(_event, _metadata, _changes), do: :ok def after_update_batch(_events, _changes), do: :okcombined with:
defoverridable after_update: 3, after_update_batch: 2, schema_prefix: 1, schema_prefix: 2 @optional_callbacks [...]ensures:
- All user projectors have callable
after_update/3andafter_update_batch/2out of the box.- Users can override these or
schema_prefixarities as needed without compiler warnings.update_projection/3andupdate_projection_batch/2can safely invoke the callbacks without extrafunction_exported?checks (though you still keep one forafter_update/3).This is a clean, backwards‑compatible way to expose the new batch callback and schema_prefix variants.
427-477: after_update_batch/2 callback docs align with implementationThe new
@docand@callback after_update_batch/2:
- Clearly state that the callback runs after the transaction commits.
- Explain that errors or exceptions will not roll back data but will propagate to the handler (potential retries / duplicate side‑effects).
- Mirror the semantics you’ve implemented in
update_projection_batch/2.- Provide a realistic example with
project_batchusage.This keeps the public contract precise and matches how the code actually behaves.
499-527: schema_prefix/1 and /2 generation now correctly matches option typesThe updated
__include_schema_prefix__/1correctly handles all supported cases:
nil→ both arities returnnil, with 2‑arity delegating to 1‑arity.- Binary prefix → both arities return the static string.
- 1‑arity function → both arities delegate to the configured function with just the event.
- 2‑arity function → 1‑arity returns
nil, 2‑arity delegates to the configured function.This behavior is thoroughly exercised by the new tests in
test/projections/projection_version_schema_prefix_test.exs(including the 1‑arity describe block), and fixes the earlier bug where the 1‑arity variant always returnednilregardless of configuration.Looks solid.
621-669: project_batch/1 macro and handle_batch/1 wiring look correctThe
project_batch/1macro:
- Enforces a single
handle_batch/1definition per projector usingModule.defines?/2, with a helpful error message and example.- Defines:
def handle_batch(all_events) do update_projection_batch(all_events, unquote(lambda)) endwhich matches the tests in
commanded_batch_integration_test.exsandecto_projection_batch_test.exs.
- Expects the user lambda to be
fn events, multi -> ... end, matching both the moduledoc examples and the batch implementation’smulti_fn.(unseen_events, Ecto.Multi.new()).Apart from the minor doc issues in the guide file, the macro API & wiring are consistent.
.formatter.exs (1)
3-11: Formatter locals updated consistently with new macroAdding
project_batch: 1tolocals_without_parensis appropriate and keeps formatter behavior consistent with existingprojectmacros and the intendedproject_batch fn ... -> ... endstyle. Therouter: 1entry is also fine.No changes needed.
test/projections/projection_version_schema_prefix_test.exs (1)
222-333: Great coverage of schema_prefix/1 behavior across configurationsThe new
"schema_prefix/1 callback (1-arity)"tests comprehensively validate:
- Static prefix via option uses same value for both arities.
- 1‑arity functional prefix is invoked for both arities.
- 2‑arity functional prefix returns
nilfor 1‑arity and uses metadata for 2‑arity.- Default (no prefix) returns
nilfor both.- Per‑event 1‑arity prefix works across multiple events.
- App‑config
schema_prefixis respected by both arities.These tests tightly couple to the behavior generated by
__include_schema_prefix__/1and should prevent regressions around the callback contract.test/projections/ecto_projection_batch_test.exs (6)
1-10: Module header and aliases are clear and idiomaticTop-level test module, imports, and aliases keep the tests focused and readable; no issues here.
11-32: BatchProjector’s batch handler matches the intended contractThe
project_batchfunction cleanly maps the three event types into projection maps or an error sentinel and leaves idempotency to the projection layer, which aligns with the tests below. No problems spotted.
114-121: Error-path behaviour on failing batch is well coveredThe
ErrorEventtest correctly asserts thathandle_batch/1returns{:error, :failure}and that no projections are inserted, which matches the intended “no partial writes on error” semantics.
123-160: after_update_batch/2 happy-path callback is exercised appropriately
BatchProjectorAfterUpdateCallbackand its test validate thatafter_update_batch/2is invoked once per batch with the full event list and thechangesmap, and that it can safely perform side effects (sending a message) after the DB work completes. The pattern of pullingpidfrom the first event keeps the test simple and focused on the callback contract.
203-232: Callback error semantics (“no rollback”) are tested clearly
BatchProjectorCallbackReturnsErrorand its test accurately capture the contract that an{:error, reason}fromafter_update_batch/2should be returned fromhandle_batch/1without rolling back the already-committed projections. The expectations on both the return value and the persisted projection look correct.
234-263: Callback raise semantics (“no rollback”) are likewise well coveredThe
BatchProjectorCallbackRaisescase properly verifies that an exception inafter_update_batch/2propagates (viaassert_raise/3) while still leaving the projection committed in the database. This cleanly complements the error-return test above and matches the documented “post-commit callback” behaviour.
84d49b4 to
403a964
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
guides/howtos/building-read-models-with-ecto.md (1)
174-201: Fixproject_batchmacro syntax in example.As previously noted, the example contains a syntax error. Line 190 shows:
project_batch fn events, multi ->but is incorrectly written as:
project_batch events, fn events, multi ->The
project_batchmacro is arity-1 and takes only the lambda function directly, not a separateeventsparameter.lib/commanded/projections/ecto.ex (1)
71-88: Validation gap::concurrencyin:subscription_optsnot checked.As previously noted,
validate_mutual_exclusivity/1only checks top-level:concurrencybut misses:concurrencynested in:subscription_opts. This allows invalid configurations like:use Commanded.Projections.Ecto, batch_size: 50, subscription_opts: [concurrency: 4]to pass validation despite being mutually exclusive.
🧹 Nitpick comments (1)
guides/howtos/building-read-models-with-ecto.md (1)
178-178: Minor: Hyphenate compound adjective.Consider changing "built in" to "built-in" when used as a compound adjective.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.formatter.exs(1 hunks)guides/howtos/building-read-models-with-ecto.md(4 hunks)lib/commanded/projections/ecto.ex(8 hunks)test/projections/commanded_batch_integration_test.exs(1 hunks)test/projections/ecto_projection_batch_test.exs(1 hunks)test/projections/projection_version_schema_prefix_test.exs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .formatter.exs
- test/projections/commanded_batch_integration_test.exs
🧰 Additional context used
🧬 Code graph analysis (2)
lib/commanded/projections/ecto.ex (3)
test/projections/projection_version_schema_prefix_test.exs (1)
schema_prefix(341-344)test/projections/ecto_projection_batch_test.exs (4)
after_update_batch(139-145)after_update_batch(178-184)after_update_batch(219-221)after_update_batch(250-252)test/projections/after_update_callback_test.exs (1)
after_update(19-25)
test/projections/ecto_projection_batch_test.exs (2)
lib/commanded/projections/ecto.ex (1)
handle_batch(665-667)test/support/projection_assertions.ex (1)
assert_projections(7-11)
🪛 LanguageTool
guides/howtos/building-read-models-with-ecto.md
[grammar] ~178-~178: Use a hyphen to join words.
Context: .... Note that there is currently no built in way to target a single type of event ...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Quality Assurance (1.18.x, 27)
🔇 Additional comments (9)
guides/howtos/building-read-models-with-ecto.md (2)
84-138: LGTM! Clear subscription options documentation.The subscription options section is well-structured, provides both nested and top-level formats, and clearly documents the mutual exclusivity constraint between
:batch_sizeand:concurrency.
304-346: LGTM! Excellent transaction semantics documentation.The warnings about post-commit callback behavior are clear, emphatic, and provide sound guidance. The formatting makes the critical nature of these constraints very visible to developers.
lib/commanded/projections/ecto.ex (5)
102-122: LGTM! Proper compile-time validation.The validation calls in
__using__properly enforce constraints at compile-time with clear error messages viaCompileError. This provides good developer feedback.
199-354: LGTM! Solid batch processing implementation.The
update_projection_batch/2implementation correctly:
- Uses
FOR UPDATElocking to prevent concurrent updates- Filters unseen events inside the transaction for idempotency
- Advances the watermark atomically
- Orchestrates user projections within the same transaction
- Handles empty batches, partial batches, and invalid formats
- Passes both
eventsandmultito the user's projection function (line 320)The architectural guarantee that all events in a batch share the same
handler_nameis well-documented in lines 207-210.
356-357: LGTM! Proper callback defaults and overridability.The default no-op implementations and
defoverridabledeclaration correctly support optional callback overrides for both single-event and batch processing.Also applies to: 363-363
369-477: LGTM! Complete callback documentation.The
@optional_callbacksdeclaration andafter_update_batch/2documentation are comprehensive. The transaction semantics warnings appropriately emphasize post-commit behavior and its implications.
621-669: LGTM! Well-implementedproject_batchmacro.The macro correctly:
- Takes a single lambda argument (the projection function)
- Prevents duplicate
project_batchdefinitions with a clear error message- Generates
handle_batch/1that delegates toupdate_projection_batch/2- Documents the expected signature clearly
The user's lambda function receives both
eventsandmultiarguments when invoked at line 320 inupdate_projection_batch/2.test/projections/projection_version_schema_prefix_test.exs (1)
222-333: LGTM! Comprehensive 1-arity callback tests.The new test suite thoroughly covers
schema_prefix/1behavior across:
- Static string prefixes
- Dynamic 1-arity and 2-arity functions
- Interaction between arities (2-arity returns nil when called as 1-arity)
- No-prefix scenarios
- Per-event dynamic prefixes
- Application config-driven prefixes
The tests verify consistency between 1-arity and 2-arity call paths, ensuring correct implementation.
test/projections/ecto_projection_batch_test.exs (1)
1-263: LGTM! Excellent batch projection test coverage.This test suite comprehensively validates batch processing behavior:
- Multiple events and mixed event types
- Idempotency for duplicate batches
- Partial batch filtering (only unseen events projected)
- Entire batch deduplication
- Error propagation without partial commits
after_update_batch/2callback invocation with correct arguments- Post-commit semantics (errors/exceptions in callbacks don't rollback the transaction)
The tests align perfectly with the implementation in
lib/commanded/projections/ecto.exand verify all critical batch processing guarantees.
403a964 to
3e14b01
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
3e14b01 to
cebac78
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/commanded/projections/ecto.ex (1)
71-88: Extend:batch_sizevs:concurrencyvalidation to:subscription_opts
validate_mutual_exclusivity/1only checks top‑levelopts[:batch_size]andopts[:concurrency]. However, the guides now encourage configuring:concurrencyunder:subscription_opts, and@handler_optspasses:subscription_optsthrough unchanged. This allows a configuration like:use Commanded.Projections.Ecto, batch_size: 50, subscription_opts: [concurrency: 4]to compile, despite the documented “mutually exclusive” rule. That’s a real config mismatch and may surprise users.
I’d fold
subscription_opts[:concurrency]into the validation, e.g.:- def validate_mutual_exclusivity(opts) do - batch_size = Keyword.get(opts, :batch_size) - concurrency = Keyword.get(opts, :concurrency) + def validate_mutual_exclusivity(opts) do + batch_size = Keyword.get(opts, :batch_size) + + subscription_concurrency = + opts + |> Keyword.get(:subscription_opts, []) + |> Keyword.get(:concurrency) + + concurrency = Keyword.get(opts, :concurrency) || subscription_concurrencyThe rest of the
casethen correctly rejects any combination where bothbatch_sizeand some concurrency (top‑level or nested) are set.
🧹 Nitpick comments (5)
lib/commanded/projections/ecto.ex (2)
161-339: Batch pipeline, idempotency, and error paths look solid; consider extra handler_name validationThe batch flow (
update_projection_batch→build_batch_projection_multi/3→lock_and_filter_batch_events/4→track_batch_projection_version/5→execute_batch_projection/2) is coherent:
- Empty batches short‑circuit to
:ok.- Input validation distinguishes structural errors (
{:invalid_batch_format, ...}/{:invalid_batch_structure, ...}) from the “already seen” case.FOR UPDATElocking plus conditional upsert ofProjectionVersiongives a sane watermark/idempotency story, and returning{:error, :already_seen_event}maps cleanly to:okinexecute_batch_projection/2.- The
project_batchlambda is now called asmulti_fn.(unseen_events, Ecto.Multi.new()), matching the documentedfn events, multi -> ... endsignature.Given the comment that all events in a batch are architecturally from the same handler, you rely on that invariant when you take
projection_namefrom the first event’s metadata. If you want extra defence against misuse (e.g. someone callinghandle_batch/1manually in downstream code), you could cheaply assert that all metadata share the same:handler_nameinlock_and_filter_batch_events/4and error out otherwise. That would turn a subtle idempotency footgun into an immediate, explicit failure without impacting the happy path.
343-347: Default callbacks +function_exported?check: behavior is correct but can be simplifiedYou now provide default no‑op implementations for
after_update/3andafter_update_batch/2via__define_helper_functions__/0and mark both asdefoverridable. That matches the new docs (“callbacks are optional; default to:ok”) and keepsafter_update_batch/2safe to call unconditionally in the batch pipeline.Given that the defaults are always defined in the using module, the
function_exported?(__MODULE__, :after_update, 3)check insideupdate_projection/3is now redundant: it will always be true. Not a correctness issue, but you could simplify by removing the guard and callingafter_update/3directly for a slightly cleaner implementation.Also applies to: 400-401
guides/howtos/building-read-models-with-ecto.md (1)
174-201:project_batchdocs and example now match the macro; minor wording nitThe
project_batchsection correctly describes:
- enabling batching via
batch_size,- the
project_batch/1macro signature, and- the handler function taking
eventsandmulti, with a working example.One small editorial tweak you might consider: change “no built in way” to “no built‑in way” (add the hyphen) for grammatical consistency.
test/projections/commanded_batch_integration_test.exs (1)
36-116: Integration tests cover batch processing, watermarking, and idempotency end‑to‑endThe three tests:
- verify
handle_batch/1is actually defined,- assert a single batch both writes projections and advances
last_seen_event_number,- and confirm multiple batches plus repeated calls behave idempotently,
which together validate the main observable behaviors of the new batch path against a real repo. One optional improvement would be to make
assert_projections/2order‑insensitive (e.g. by sorting names) to avoid relying on database row order, but that’s not a blocker here.test/projections/ecto_projection_batch_test.exs (1)
203-263: Callback error/exception semantics forafter_update_batch/2match the documented contractThe
BatchProjectorCallbackReturnsErrorandBatchProjectorCallbackRaisesmodules, with their respective tests, confirm that:
- returning
{:error, :callback_failed}fromafter_update_batch/2causeshandle_batch/1to return the error while leaving committed projections intact, and- raising an exception in
after_update_batch/2propagates the exception but likewise does not roll back the batch transaction.That behavior is precisely what the new “
⚠️ Transaction Semantics” docs describe and validates the post‑commit nature of these callbacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.formatter.exs(1 hunks)guides/howtos/building-read-models-with-ecto.md(4 hunks)lib/commanded/projections/ecto.ex(8 hunks)test/projections/commanded_batch_integration_test.exs(1 hunks)test/projections/ecto_projection_batch_test.exs(1 hunks)test/projections/projection_version_schema_prefix_test.exs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/projections/projection_version_schema_prefix_test.exs
🧰 Additional context used
🧬 Code graph analysis (3)
test/projections/ecto_projection_batch_test.exs (2)
lib/commanded/projections/ecto.ex (1)
handle_batch(702-704)test/support/projection_assertions.ex (1)
assert_projections(7-11)
test/projections/commanded_batch_integration_test.exs (2)
lib/commanded/projections/ecto.ex (1)
handle_batch(702-704)test/support/projection_assertions.ex (1)
assert_projections(7-11)
lib/commanded/projections/ecto.ex (3)
test/projections/projection_version_schema_prefix_test.exs (1)
schema_prefix(341-344)test/projections/ecto_projection_batch_test.exs (4)
after_update_batch(139-145)after_update_batch(178-184)after_update_batch(219-221)after_update_batch(250-252)test/projections/after_update_callback_test.exs (1)
after_update(19-25)
🪛 LanguageTool
guides/howtos/building-read-models-with-ecto.md
[grammar] ~178-~178: Use a hyphen to join words.
Context: .... Note that there is currently no built in way to target a single type of event ...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Quality Assurance (1.18.x, 27)
🔇 Additional comments (9)
lib/commanded/projections/ecto.ex (2)
536-552: Schema prefix handling now correctly exposes 1‑arity variantsThe updated
__include_schema_prefix__/1correctly makesschema_prefix/1return:
nilwhen no prefix is configured,- the configured string when
:schema_prefixis a binary, and- the result of the configured 1‑arity function when applicable.
That fixes the earlier issue where
schema_prefix/1always returnednilregardless of configuration and aligns with the documented callback contract. The special handling of a 2‑arity function (1‑arity returningnil, 2‑arity delegating to the user function) is reasonable given that callers needing metadata can useschema_prefix/2.
658-706:project_batch/1macro and handler wiring align with docs and testsThe
project_batch/1macro now:
- Enforces a single
handle_batch/1per projector viaModule.defines?/2.- Defines
handle_batch/1that simply delegates toupdate_projection_batch/2.- Matches the documented and tested handler shape
project_batch fn events, multi -> ... end, sinceexecute_batch_projection/2calls the lambda asmulti_fn.(unseen_events, Ecto.Multi.new()).That resolves the previous lambda‑arity mismatch and gives a clear, single entrypoint for batched processing.
guides/howtos/building-read-models-with-ecto.md (3)
84-137: Subscription options docs are clear and match the intended APIThe new
:subscription_optssection does a good job explainingstart_from,subscribe_to, andconcurrency, plus the precedence rules when mixing nested and top‑level options. This lines up with how the handler options are passed through from__using__/1. Oncevalidate_mutual_exclusivity/1is updated to also look atsubscription_opts[:concurrency], the documentation and implementation will be fully aligned.
304-347: Transaction semantics forafter_update/3andafter_update_batch/2are well documentedThe new “
⚠️ Transaction Semantics” sections make it explicit that both callbacks run after commit, cannot influence rollback, and are meant for side effects only, with clear notes about duplicate side effects on retries. This matches the implementation inupdate_projection/3andexecute_batch_projection/2, where callbacks are invoked aftertransaction/1completes, and where errors are propagated without undoing DB changes.
356-369: Schema prefix config path in docs matches__using__/1The updated example using:
config :commanded, Commanded.Projections.Ecto, schema_prefix: "example_schema_prefix"is consistent with the new
schema_prefixlookup in__using__/1(pulling fromApplication.get_env(:commanded, Commanded.Projections.Ecto, []) |> Keyword.get(:schema_prefix)). This keeps the guide in sync with the actual configuration path..formatter.exs (1)
3-11: Formatter update correctly supportsproject_batchwithout parensAdding
project_batch: 1tolocals_without_parenskeeps formatting consistent with existingprojectmacros and the examples in the docs/tests. The change is scoped to formatting only and is safe.test/projections/commanded_batch_integration_test.exs (1)
12-27: IntegrationBatchProjector setup is minimal and representativeThe nested
IntegrationBatchProjectorusesbatch_size: 5and aproject_batchhandler that mapsAnEventnames into plain projection maps and persists viaEcto.Multi.insert_all/4. This is a clean, focused projector for integration testing and exercises the batch pipeline without extra complexity.test/projections/ecto_projection_batch_test.exs (2)
11-32: Core batch projector tests thoroughly exercise success, idempotency, and failureThe
BatchProjectormodule plus the first group of tests validate:
- normal multi‑event batches across one or multiple event types,
- ignoring fully or partially already‑seen batches while still advancing the watermark,
- and propagating user‑level failures from
Ecto.Multi.error/3as{:error, :failure}without writing projections.These scenarios map directly onto the batch pipeline’s intended behavior and give strong coverage of idempotency and error handling.
Also applies to: 39-122
123-201:after_update_batch/2callback behavior andchangescontents are well coveredThe
BatchProjectorAfterUpdateCallbackandBatchProjectorCallbackWithChangesmodules plus their tests verify that:
after_update_batch/2is invoked once per batch with the fulleventslist,- the callback receives the committed
changesmap from theEcto.Multitransaction, including internal steps like:lock_and_filter,:track_projection_version,:prepare_user_multi, and the user step (:projection),- and that callers can safely use this for out‑of‑transaction notifications.
This directly matches how
execute_batch_projection/2callsafter_update_batch/2post‑commit.
1 participant
Note
Introduce batch event projection via
project_batch/1with watermark idempotency,after_update_batch/2, compile-time validations, updated docs, and comprehensive tests.project_batch/1,handle_batch/1, andupdate_projection_batch/2with lock-then-watermark idempotency and single-transaction execution.ProjectionVersion, and merge userEcto.Multiops; defaultafter_update_batch/2provided.:batch_sizeand:concurrency; disallow dynamic:schema_prefixwhen batching.after_update_batch/2(post-commit side-effects) and document transaction semantics; keep existingafter_update/3behavior.schema_prefix/1to return configured string or 1-arity function result; maintain 2-arity behavior.start_from,subscribe_to,concurrency) and top-level equivalents; note concurrency vs batch mutual exclusion.project_batch/1usage andafter_update_batch/2with post-commit semantics; clarifyafter_update/3semantics.config :commanded, Commanded.Projections.Ecto, schema_prefix: ....project_batch: 1tolocals_without_parens.schema_prefixbehavior for 1-arity/static/dynamic configs.Written by Cursor Bugbot for commit cebac78. This will update automatically on new commits. Configure here.