Repository navigation
feat: add Commanded.Projections.Ecto - #27
Conversation
|
Warning Rate limit exceeded@yordis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
WalkthroughAdds built-in Ecto-backed projections: a new Changes
Sequence Diagram(s)sequenceDiagram
participant ES as Event Store / Handler
participant Projector as Generated Projector (handle/2)
participant EctoProj as Commanded.Projections.Ecto
participant Multi as Ecto.Multi
participant Repo as Commanded.Projections.Repo
participant UserFn as user multi_fn
participant After as after_update/3
ES->>Projector: handle(event, metadata)
Projector->>EctoProj: update_projection(event, metadata, user_multi_fn)
EctoProj->>Multi: build base Multi (incl. ProjectionVersion upsert)
EctoProj->>UserFn: invoke to extend Multi
EctoProj->>Repo: Repo.transaction(Multi)
Repo-->>EctoProj: {:ok, changes} / {:error, reason} / Ecto.StaleEntryError
alt success
EctoProj->>After: after_update(event, metadata, changes) (if implemented)
After-->>EctoProj: :ok | {:error, reason}
else already-seen / stale
EctoProj-->>Projector: treat as already processed (no-op)
end
EctoProj-->>ES: return handling result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
6512394 to
2cbee05
Compare
a895ff2 to
e52ff09
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
priv/repo/migrations/20170929080308_create_projection_version_with_prefix.exs (1)
1-19: Migration for prefixedprojection_versionsis consistent with the base schemaThe
test.projection_versionstable mirrors the default projection_versions schema and the up/down pair (create schema, create table; drop table, drop schema CASCADE) is sound for the intended Postgres test setup. If you ever expect thetestschema to be pre-created, you could switch toCREATE SCHEMA IF NOT EXISTS test, but for isolated test DBs this is not required.If you want to be extra safe about
CREATE SCHEMAsemantics across Postgres versions, please confirm via local migration runs against the Postgres 17 image used in CI.lib/commanded/projections/ecto.ex (1)
178-205: Tiny style nit: avoid binding an unusedexceptionin rescueIn the
rescueblock you bindexception in Ecto.StaleEntryError ->but never useexception. This is harmless, but you could simplify torescue Ecto.StaleEntryError ->to avoid an unused variable warning and make intent clearer, while keeping the genericexception -> reraise exception, __STACKTRACE__clause.Please confirm your Elixir version’s compiler doesn’t already optimize away the unused binding or emit a warning here; if it does, the suggested tweak will silence it.
test/test_helper.exs (1)
5-15: Consider extracting the inline migration to a separate file.While the inline migration works, it's unconventional. Most Elixir projects keep all migrations in
priv/repo/migrations/for consistency and visibility.If you prefer to keep it inline, verify that the timestamp
20_170_609_120_000doesn't conflict with any existing migrations:#!/bin/bash # Description: Check for migration timestamp conflicts # List all migration files and their timestamps fd -e exs . priv/repo/migrations/ --exec basename | sort # Check if the timestamp 20170609120000 is used elsewhere rg -n "20_?170_?609_?120_?000" priv/repo/migrations/test/support/runtime_config_projector.ex (1)
7-13: Consider simplifying the pattern matching.The event is pattern-matched twice: once in the function head (line 7) and again in the function body (line 8). You can extract the fields directly in the function head.
Apply this diff to simplify:
- project(%AnEvent{} = event, fn multi -> - %AnEvent{name: name, pid: pid} = event + project(%AnEvent{name: name, pid: pid}, fn multi ->guides/howtos/building-read-models-with-ecto.md (1)
7-15: Docs are strong; consider a few minor clarity and wording tweaksOverall this guide is very clear and matches the new
Commanded.Projections.EctoAPI well. A few minor polish points you might want to address:
- In the first example schema (
ExampleProjection), later snippets insert a struct includingmetadata(e.g.%ExampleProjection{name: name, metadata: metadata}). To avoid confusing copy‑paste users, either addfield :metadata, :mapto the schema example or omitmetadatafrom the insert example so they’re consistent.- Wording/grammar nits:
- “error tagged tuple” → “error‑tagged tuple” (hyphenates the compound adjective).
- “tenant specific projections” → “tenant‑specific projections” in both occurrences.
- The sentence “in a similar manner since the errors are caught …” could be shortened to “similarly, since the errors are caught …” to read a bit more smoothly.
These are all editorial; the technical content looks accurate.
Also applies to: 90-99, 160-193, 286-313
test/projections/ecto_projection_test.exs (1)
75-90: Double‑check Ecto sandbox ownership for the concurrentTask.asynctestsThe two concurrency tests (
"should prevent first event being projected more than once"and"should prevent an event being projected more than once"), lines 75–90 and 92–131, runProjector.handle/2insideTask.async/3processes. Becausehandle/2executes Ecto operations viaRepo, these Task processes need appropriate sandbox access.If your global test setup uses
Ecto.Adapters.SQL.Sandboxin:manualmode (and onlySandbox.checkout(Repo)here), those Task processes won’t own a connection and may raise sandbox ownership errors. A common pattern is to make the test connection shared before spawning tasks, for example:setup do start_supervised!(TestApplication) :ok = Sandbox.checkout(Repo) :ok = Sandbox.mode(Repo, {:shared, self()}) endor to explicitly
Sandbox.allow/3each spawned Task PID.If your
test_helperalready switches this repo to:sharedfor these tests, then you’re fine; otherwise, I’d adjust the setup to avoid intermittent failures when running the suite concurrently.Please confirm your
Ecto.Adapters.SQL.Sandboxmode forCommanded.Projections.Repointest/test_helper.exsand, if it’s:manual, consider addingSandbox.mode(Repo, {:shared, self()})or equivalent here so the concurrency tests remain stable across environments.Also applies to: 92-131
test/support/projection_assertions.ex (1)
7-24: Projection assertion helpers match usage; consider slightly richer error reporting inlast_seen_event/1The helpers line up nicely with how the tests use them:
assert_projections/2expects anamefield and keeps the assertions terse.assert_seen_event/2pluslast_seen_event/1map directly onto theprojection_versionsschema introduced in the migration.One optional robustness improvement would be to handle unexpected results from
SQL.query/3explicitly, rather than falling through thecaseand raising a genericCaseClauseError. For example, you could add something like:case SQL.query(Repo, sql, [projection_name]) do {:ok, %{num_rows: 0}} -> nil {:ok, %{rows: [[last_seen]], num_rows: 1}} -> last_seen {:error, reason} -> flunk("failed to query projection_versions: #{inspect(reason)}") endThis keeps failures self‑describing if the table is missing or the query changes, which can be helpful when debugging CI.
If you adopt a change like this, please re‑run the projection tests to confirm no callers depend on
CaseClauseErrorbeing raised today (which seems unlikely, but worth double‑checking).test/projections/projection_version_schema_prefix_test.exs (3)
19-173: Schema prefix tests provide thorough coverage; only minor naming nitNice spread of cases (default, static, config-driven, anonymous functions, and callback-based
schema_prefix/1and/2), which should give strong confidence in the prefix resolution logic. As a minor future-proofing tweak, you might give each projector a unique:nameto avoid any chance of cross-test interference if these ever start persisting data.
199-213: Consider loosening the assertion on the exact error message textThis test clearly guards against invalid
:schema_prefixconfiguration, but asserting on the full error string can become brittle if the message wording changes. Matching on a stable substring or using a regex (e.g. containing"expected :schema_prefix option") would keep the intent while being less fragile.
216-225:apply/3in helper can be replaced with direct module callSince
projectoris a module,projector.schema_prefix(event, metadata)would work and let you drop the Credo suppression aroundapply/3. Not a big deal, but it keeps the helper in line with common Elixir style.test/support/events.ex (1)
1-33: Event struct definitions are consistent;@enforce_keyswould be a nice-to-haveThe event structs are straightforward and align with how they’re used in the projections and tests. If you ever want tests to fail fast on missing required fields (like
:pidor:schema), adding@enforce_keysto those event modules would make that intent explicit, but it’s not necessary for the current test usage.test/support/error_projector.ex (2)
1-18: Module wiring looks correct for a test-only projectorThe projector is correctly wired with application, name, and repo, and the aliases keep the rest of the module readable. If you run docs generation, consider adding
@moduledoc falsehere to keep this test helper out of generated docs.
47-81: Error callbacks are explicit but could be slightly DRYed upThe four
error/3clauses all follow the same pattern of extractingpid, forwarding the error withsend/2, and returning:skip. That explicitness is fine, but if this grows you might consider a small private helper to forward the error and return:skipto reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mix.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/ci.yml(1 hunks)config/test.exs(1 hunks)guides/explanations/read-model-projections.md(1 hunks)guides/howtos/building-read-models-with-ecto.md(1 hunks)guides/howtos/ecto-projections-getting-started.md(1 hunks)lib/commanded/projections/ecto.ex(1 hunks)mix.exs(4 hunks)priv/repo/migrations/20170609113553_create_projection_versions.exs(1 hunks)priv/repo/migrations/20170929080308_create_projection_version_with_prefix.exs(1 hunks)test/projections/after_update_callback_test.exs(1 hunks)test/projections/deprecated_projection_test.exs(1 hunks)test/projections/ecto_projection_test.exs(1 hunks)test/projections/error_callback_test.exs(1 hunks)test/projections/init_callback_test.exs(1 hunks)test/projections/projection_version_schema_prefix_test.exs(1 hunks)test/projections/runtime_config_projector_test.exs(1 hunks)test/support/error_projector.ex(1 hunks)test/support/events.ex(1 hunks)test/support/projection.ex(1 hunks)test/support/projection_assertions.ex(1 hunks)test/support/repo.ex(1 hunks)test/support/runtime_config_projector.ex(1 hunks)test/support/test_application.ex(1 hunks)test/test_helper.exs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
test/projections/ecto_projection_test.exs (2)
lib/commanded/projections/ecto.ex (4)
handle(232-236)handle(256-258)handle(269-273)handle(294-296)test/support/projection_assertions.ex (1)
assert_projections(7-11)
test/projections/error_callback_test.exs (1)
test/support/projection_assertions.ex (1)
assert_projections(7-11)
test/projections/runtime_config_projector_test.exs (2)
test/support/projection_assertions.ex (1)
assert_projections(7-11)test/projections/error_callback_test.exs (1)
stub_event_store(150-163)
test/projections/projection_version_schema_prefix_test.exs (1)
lib/commanded/projections/ecto.ex (4)
handle(232-236)handle(256-258)handle(269-273)handle(294-296)
test/projections/after_update_callback_test.exs (1)
lib/commanded/projections/ecto.ex (4)
handle(232-236)handle(256-258)handle(269-273)handle(294-296)
test/projections/deprecated_projection_test.exs (2)
lib/commanded/projections/ecto.ex (4)
handle(232-236)handle(256-258)handle(269-273)handle(294-296)test/support/projection_assertions.ex (1)
assert_projections(7-11)
lib/commanded/projections/ecto.ex (2)
test/projections/projection_version_schema_prefix_test.exs (1)
schema_prefix(222-225)test/projections/after_update_callback_test.exs (1)
after_update(19-25)
🪛 LanguageTool
guides/howtos/building-read-models-with-ecto.md
[grammar] ~162-~162: Use a hyphen to join words.
Context: ... example projector module where an error tagged tuple is explicitly returned from...
(QB_NEW_EN_HYPHEN)
[style] ~162-~162: Consider replacing this phrase with the adverb “similarly” to avoid wordiness.
Context: ...faulty Ecto.Multi database operations in a similar manner since the errors are caught and returne...
(IN_A_X_MANNER)
[grammar] ~286-~286: Use a hyphen to join words.
Context: ... An example usage could be for tenant specific projections where each tenant's...
(QB_NEW_EN_HYPHEN)
[grammar] ~308-~308: Use a hyphen to join words.
Context: ... An example usage could be for tenant specific projections where each tenant's...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
guides/howtos/ecto-projections-getting-started.md
19-19: Dollar signs used before commands without showing output
(MD014, commands-show-output)
42-42: Dollar signs used before commands without showing output
(MD014, commands-show-output)
⏰ 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 (22)
.github/workflows/ci.yml (1)
80-87: Order of checks after tests looks fineRunning tests first and then format/compile/credo is a reasonable trade-off; the new steps are correctly wired and won’t interfere with the Dialyzer cache/use that follows.
lib/commanded/projections/ecto.ex (2)
1-117: Ecto-backed projection wiring and idempotency logic look solidThis module is a faithful in-tree adaptation of
commanded_ecto_projectionswith updated config keys (:commanded, :ecto_projection_repo/:ecto_projection_schema_prefix), aCode.ensure_loaded?(Ecto)guard, and the same ProjectionVersion-based idempotency pattern (conditional upsert withEcto.StaleEntryErrorhandling).update_projection/3correctly:
- tracks
last_seen_event_numberperprojection_name,- uses the optional
schema_prefix/2andprojection_versionsschema,- wraps user
multi_fnin a transaction,- and distinguishes already-seen events from real projection failures.
From a behaviour standpoint, this should match existing users’ expectations while making the repo configurable from either
useoptions orcompile_env.To double-check behaviour parity with the original library, you can diff this module against
lib/projections/ecto.exincommanded_ecto_projectionsand run the new projection tests (including replay/idempotency cases) against both versions.
123-223: No issues found; original assessment is correctVerification confirms the review comment's key claim: the
after_update/3invocation at line 102 usesapply()and returns the result directly without pattern-matching. The return value flows through unchanged, making the widened spec (:ok | {:ok, any} | {:error, any}) safe. The callback contract and schema_prefix handling are well-designed as stated.mix.exs (1)
75-89: Ecto integration in deps, docs, aliases, and Dialyzer is correctly configuredAll changes are properly aligned:
- Optional Ecto/Ecto.SQL deps prevent forced dependency on consumers
- Docs grouping and module nesting support the projections story
- Aliases reference ecto tasks safely within test environment scope
- Dialyzer
plt_add_appsexplicitly includes optional apps, which is the correct pattern for handling optional dependenciesThe configuration follows Dialyxir best practices and poses no risk to the codebase.
test/support/projection.ex (1)
1-7: LGTM!The test support schema is simple and appropriate for validating projection functionality.
config/test.exs (1)
42-50: LGTM!The test configuration correctly sets up a dedicated Ecto repository with Sandbox mode for isolated, concurrent test execution.
test/test_helper.exs (2)
1-3: LGTM!The repository startup follows standard Ecto test patterns.
17-19: LGTM!The ExUnit configuration with manual Sandbox mode is correct for concurrent test execution with explicit checkouts.
test/support/test_application.ex (1)
1-8: LGTM!The test application configuration is appropriate, using an in-memory event store for fast test execution.
test/projections/init_callback_test.exs (2)
7-21: LGTM!The
init/1callback implementation is clean and correctly implements the test coordination pattern.
28-37: LGTM!The test correctly verifies that the
init/1callback is invoked on start with the expected configuration.test/support/repo.ex (1)
1-5: LGTM!The repository definition is standard and correctly configured for the Postgres adapter.
guides/explanations/read-model-projections.md (1)
7-7: No issues found. The referenced guide files exist and links are correctly formatted.Both
guides/howtos/ecto-projections-getting-started.mdandguides/howtos/building-read-models-with-ecto.mdare present in the repository. The documentation build configuration (mix.exs) includes all guides viaPath.wildcard("guides/**/*.{cheatmd,md}"), and ExDoc converts these markdown files to HTML with the corresponding filenames. The link format is consistent with other guide references in the codebase.priv/repo/migrations/20170609113553_create_projection_versions.exs (1)
4-10: Schema forprojection_versionsmatches test helpers and idempotency use‑caseThe table structure (text PK
projection_name, nullablelast_seen_event_numberas bigint,naive_datetime_usectimestamps) lines up with howlast_seen_event/1queries and how idempotent projections are tracked in tests. I don’t see missing constraints or indices for the intended usage.If you haven’t already, please run the full migration set against a fresh Postgres instance and ensure the tests that rely on
projection_versions(e.g.projection_version_schema_prefix_test.exs) execute cleanly.test/projections/error_callback_test.exs (1)
24-68: Error‑path coverage and projector lifecycle handling look solidThe tests do a good job of exercising all error paths (tagged
{:error, reason}, raised exceptions, invalidEcto.Multi, and continuation after multiple failures) while keeping the projector process alive and asserting correct messages/log output. The Mox stubs andSandbox.allow/3usage for the projector process are also wired correctly for this scenario.One small thing to keep in mind long‑term: the log assertion that matches an exact file/line (
error_projector.ex:37) is quite brittle and will need updating if that module changes; if it becomes noisy, consider relaxing it to just assert the module/function.Please confirm that these tests pass under your CI with
MIX_ENV=testand your currentEcto.Adapters.SQL.Sandboxmode, as the behavior of the projector plus sandbox can differ slightly across Ecto versions.test/projections/after_update_callback_test.exs (1)
9-26: Focused validation ofafter_update/3behavior looks correctThe embedded
Projectorplus the single test exercise the newafter_update/3callback nicely: you verify thathandle/2returns:ok, that the callback is invoked with the exact event/metadata, and thatchanges[:my_projection]contains the projectedProjectionstruct. Running the projection in the test process (withSandbox.checkout/1) keeps the DB ownership simple.If you want stronger compile‑time guarantees in this test module, you could optionally add
@impl Commanded.Projections.Ectoaboveafter_update/3, but it isn’t required here.Please run this test with the same Ecto version you intend to support to confirm that the shape of the
changesmap (keys and structs) matches your expectations across versions.Also applies to: 33-45
test/projections/runtime_config_projector_test.exs (1)
21-54: Runtime configuration scenario and isolation between projector instances look goodThis test cleanly demonstrates that two
RuntimeConfigProjectorinstances, configured via runtime:application/:nameoptions, share the same repo but maintain independentlast_seen_eventstate. The pattern of sending{:events, events}directly, coupled with Mox stubs for the event store andSandbox.allow/3for both projector processes, is consistent with how the Commanded event handler pipeline works.Just keep an eye on
stub_event_store/1if the Mock adapter’s callback surface evolves in the future; you’ll want the stubbed functions to match whatever the real adapter is expected to implement so tests keep exercising the realistic path.Please confirm under your current Commanded/EventStore versions that no additional EventStore callbacks (e.g. streaming functions) are invoked in this scenario, so the Mox stubs here remain sufficient.
Also applies to: 57-74
test/projections/ecto_projection_test.exs (1)
11-28: Comprehensive coverage of projection semantics and configuration behaviorThis suite does a nice job of exercising the main behaviors of
Commanded.Projections.Ecto:
- Event handling and idempotent projection for multiple event types.
- Ignoring unprojected events and ensuring
last_seen_event/1only advances on projected events.- Returning error tuples from a projection and verifying no projection rows are written.
- Validating configuration: environment‑based
:reporequirement, override viarepo:option, and enforcing:namepresence on start.The patterns you use (calling
Projector.handle/2directly with metadata containinghandler_name/event_number, and querying viaProjectionAssertions) are consistent and easy to follow.It would be good to re‑run this module in isolation to ensure the configuration‑validation tests don’t leak app env changes into other tests; the
try/afterblocks look correct, but env‑mutation bugs can be subtle.Also applies to: 35-73, 133-177, 179-192
test/projections/deprecated_projection_test.exs (1)
17-71: Deprecation coverage forprojectdo/end forms is well structuredCapturing
stderraround thedefmoduledefinitions is a good way to assert that the deprecatedproject/2andproject/3do/end forms emit warnings while still verifying they behave correctly (first test) and that the new function‑style forms are encouraged in the messages (second and third tests).Defining these projector modules inside the tests keeps the surface area small. Just be aware that if you ever re‑run these test modules in a code‑reloading setup, you may see module redefinition warnings; that’s usually harmless in CI environments.
When you bump Elixir or the compiler, please re‑run these tests to ensure the deprecation warnings still flow to
stderrin the same way, as compiler messaging behavior can occasionally change between versions.test/projections/projection_version_schema_prefix_test.exs (2)
8-17: Setup correctly isolates configuration and database stateCapturing the original
:ecto_projection_schema_prefix, restoring it inon_exit/1, and checking out the sandboxed repo gives good test isolation; no changes needed here.
175-197: ProjectionVersion prefix test exercises the critical persistence pathDefining a projector with
schema_prefix: "test", invokinghandle/2, and asserting viaRepo.get/3withprefix: "test"is a clear way to ensure the projection version record is written under the correct schema and thatlast_seen_event_numberis updated as expected.test/support/error_projector.ex (1)
19-45: Projection clauses cleanly cover normal and failure scenariosEach
project/2clause sets up a distinct scenario: a normal insert with a side-effectsend/2, a manual{:error, :failed}return, a bad change, a raised exception, and an invalid query. This gives good coverage of success and different failure paths without any obvious correctness issues in the callbacks themselves.
1498e46 to
d7278b2
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
d7278b2 to
12b7941
Compare
Note
Integrates
Commanded.Projections.Ectointo core with optional Ecto deps, migrations, tests, and docs for building read models via projections.Commanded.Projections.Ectoprovidingproject/2-3,after_update/3, andschema_prefix/1-2callbacks with idempotent tracking viaprojection_versions.projection_versions(with and without schema prefixes).Repo, test projectors, events, assertions).:ectoand:ecto_sql; update Dialyzer apps and docs module groups.ectotasks tosetup/resetaliases.ecto_reposandCommanded.Projections.Repoinconfig/test.exs.ecto-projections-getting-startedandbuilding-read-models-with-ecto; update read model projections and fork differences to reflect built-in Ecto projections.Written by Cursor Bugbot for commit 12b7941. This will update automatically on new commits. Configure here.