AGENTS.md · saleor/saleor

Sections

1. Deployment model (read this first)
2. Multi-tenancy: never rely on settings.SITEID
3. Graphql
4. Running in a git worktree
5. Running tests
6. Writing tests
7. GraphQL Authorization Tests
8. Dispatching webhook events
9. Atomic Increment Pattern
10. Avoid Check-Then-Act Race Conditions
11. Row-Level Locking with selectforupdate
12. Database Transactions
13. Schedule background work after commit
14. Never block a request/worker thread
15. Other concurrency rules
16. Summary of Patterns Used in Saleor
17. Marking search indexes as dirty
18. Adding indexes / unique constraints concurrently
19. Correctness (Django): use pk instead of id
20. Prefer docstrings over comments

First lines (shown under the repo's BSD-3-Clause license)

# Saleor

## Deployment model (read this first)

Saleor is deployed for **horizontal scale-out: many identical Kubernetes pods** (web + Celery
workers) run concurrently behind a load balancer, against a **shared Postgres (with read replicas)
and shared Redis/broker**. Any given request or task may land on any pod, and the same code runs in
many processes at once. Write code with that in mind:

- **No in-process/local state as source of truth.** Never rely on module globals, in-memory caches,
  or a value set by a previous request being present on the next one — the next call hits a different
  pod. Shared state lives in Postgres and Valkey or Redis.
- **Assume every operation runs concurrently with itself.** Use atomic DB operations
  (`F()`, `update_or_create`, `select_for_update`) instead of check-then-act — see *Concurrency and
  Thread Safety* below.
- **Never block a request/worker thread.** No `time.sleep()` and no unbounded `while True` retry loops
  in request-handling code — a blocked thread wastes a finite pod worker slot. Use bounded retries and
  background tasks.
- **Schedule background work after the transaction commits.** Use `on_commit` / `call_event` (not a
  bare `.delay()` inside an open transaction) so a task can't run on another pod before the row it
  needs is committed.
- **Protect the shared database.** It is the scaling bottleneck: avoid N+1 queries and per-row writes
  in loops (bulk instead), keep `select_for_update` transactions short, fetch only the columns you
  need, and don't add per-request queries to hot paths — reuse dataloaders.
- **Make tasks idempotent and safe to retry** — the broker may deliver a task more than once, and pods
  can be killed/rescheduled mid-run.

## Multi-tenancy: never rely on `settings.SITE_ID`

A deployment may serve **several sites from one process**, and `SITE_ID` may be unset or differ from
what a dev environment has. Never read it directly to identify the current site, and never assume
there is only one.

- **Resolve the site with `Site.objects.get_current()`** (or the `get_site_promise` dataloader in the
  GraphQL layer). Both pick the site by `SITE_ID` when it is configured and fall back to the request
  host when it is not — `settings.SITE_ID` alone silently collapses every tenant into one.
- **Key anything per-site by the resolved site**, not by a constant. A cache key, module-level dict or
  query filter that bakes in a single site lets one tenant read another's data.
- **Don't assume a single row.** `Site.objects.get()` breaks as soon as a second site exists; filter by
  the resolved site instead.

`Site.objects.get_current()` reads the patched process-global `THREADED_SITE_CACHE`
(`saleor/site/patch_sites.py`), which is never invalidated by `Site`/`SiteSettings` saves. So a value
read through it can be stale after a write in the same process — see *Writing tests* for what that
means for assertions.

## Graphql

###  API versioning

- Check last git tag to find what version we are branched from (e.g. 3.22 tag). Based on that, add
  description to new fields with ADDED_IN_{VERSION} clause. Use the version the change actually ships
  in — for a backport, that is the backport's version, not the one on `main`.
- Import the constant from `saleor/graphql/core/descriptions.py` and concatenate it onto the
  description. Add a new constant there if the target version has none yet.
- **For a new type, annotate only the type-level `Meta.description`.** Its fields are covered by the
  type's own annotation, so leave them unstamped:

  ```python
  class TransactionEventFilterInput(BaseInputObjectType):
      created_at = DateTimeRangeInput(
          description="Filter transaction events by created at date.",
      )

      class Meta:
          doc_category = DOC_CATEGORY_PAYMENTS
          description = "Filter input for transaction events data." + ADDED_IN_323
  ```

- Annotate an individual field only when it is added to a type that already exists. That is the case
  where the field's version genuinely differs from its type's.
- Do not stamp a pre-existing field just because you changed its behavior. ADDED_IN marks new schema
  surface, not modifications.
- Regenerate `saleor/graphql/schema.graphql` afterwards and commit it — the annotation is part of the
  public schema's description text, and a stale schema file fails CI.


### GraphQL permissions
- Use PermissionsField to describe field restrictions

# Architecture

- Do not use Django signals (e.g. `post_save`, `pre_delete`, `@receiver`). Call the relevant logic explicitly from the code that triggers it instead of wiring it through signal handlers. (The one exception is the `post_migrate` signal used to trigger data-migration tasks — see Data Migrations below.)

# Code style

- Use global import statements. Place all imports at the top of the file rather than inside functions, methods, or other local scopes.

# Testing

## Running in a git worktree

When Saleor is checked out in a git worktree, prefer running operations (tests, management
commands, etc.) inside that worktree's containers rather than on the host machine. The host's
services may belong to a different worktree, so running directly on the host can hit the wrong
database/cache or collide with another worktree's stack.

- Use the `.worktree-container/compose.sh` wrapper, which targets the current worktree's
  isolated stack. Run commands inside the `saleor` service, e.g.:
  ```sh
  .worktree-container/compose.sh up -d                       # start this worktree's stack
  .worktree-container/compose.sh exec saleor pytest --reuse-db saleor/path/to/test_file.py
  .worktree-container/compose.sh exec saleor python manage.py migrate
  ```
- Do not run these operations directly on the host machine when working in a worktree.

## Running tests

- Run tests using `pytest`
- Attach `--reuse-db` argument to speed up tests by reusing the test database
- Select tests to run by passing test file path as an argument
- Inside the dev container, dependencies are installed system-wide, so run `pytest` directly — no virtual environment to activate
- On the host (outside the container), enter the virtual environment before executing tests

## Writing tests

- Use given/when/then structure for clarity
- Use `pytest` fixtures for setup and teardown
- Declare test suites flat in file. Do not wrapp in classes
- Prefer using fixtures over mocking. Fixtures are usually within directory "tests/fixtures" and are functions decorated with`@pytest.fixture`
- When you create an object for testing and fixture for it doesn't exist, create new one. You can use factory to pass arguments to the fixture.
- When writing assertions, prefer assertion on actual returned value instead of checking if it's not none. For example: `assert email is "a@b.com` instead if `assert email is not None`
- When asserting GraphQL errors, assert error message too
- When asserting to Enum, import enum in a test file and use `assert error.code == MyEnum.SOME_ERROR.name` instead plain string comparison
- Avoid assertion to plain values, if you already have references to existing value. For example, when you create entity in database and assert if entity is returned in response, compare response fields with entity fields, instead plain values.
- When building GraphQL query variables, use enum values instead of plain strings. For example: `{"field": TransactionSortField.CREATED_AT.name}` instead of `{"field": "CREATED_AT"}`
- Do not write unnecessary prefix to test names. If file name is `test_my_mutation.py`, do not write `test_my_mutation_with_x_y` but `test_with_x_y`
- When testing graphQL cases with expected errors ALWAYS assert expected length of errors list
- When setting up test data, extract values into variables and reuse them in assertions. Do not repeat literal values between setup and assertion — use the variable instead.
- When comparing JSON payloads in tests, use `json.loads()` to compare dicts instead of comparing serialized strings with `json.dumps()`. String comparison breaks when key order changes.
- When a test expects exactly one row, use `Model.objects.get()` instead of `.first()` followed by an `is not None` assertion. `get()` both fetches the object and asserts that exactly one exists, so drop the separate not-None check.
- For presence/absence checks prefer `qs.exists()` over `qs.count() == 0` / `!= 0`. `exists()` is cheaper than a `COUNT`. Example: `assert checkout.lines.exists() is False`.
- Use `@pytest.mark.parametrize` for variations of the same scenario instead of copy/pasting near-identical test bodies. Label each case with a leading `_case` string parameter rather than the `ids=` argument — it keeps the description next to the data and is easier to read and edit:
  ```python
  @pytest.mark.parametrize(
      ("_case", "value"),
      [
          ("omitted", {}),
          ("empty_list", {"lines": []}),
      ],
  )
  def test_something(_case, value): ...
  ```
- When a mutation input is optional/nullable, test the explicit `null` value in addition to omitting the field — they are distinct inputs and can be handled differently.
- **Never assume a cache is cold — or warm.** Caches are process-global and survive across tests, so
  what warmed them depends on test ordering. To assert a write landed, read the row back
  (`obj.refresh_from_db(fields=(...))`) rather than through a cached accessor like
  `Site.objects.get_current()`; to count queries, warm the cache first so the count is the steady
  state; otherwise bust it explicitly (`Site.objects.clear_cache()`). A test that passes only because
  a cache happened to be empty will fail as soon as an unrelated caller warms it.

### Assert precisely (the #1 source of review comments)

- **Assert exact values and counts, never bounds or mere absence.** No `len(x) > 0`, `>= 1`,
  `assert not errors`, or `call_count >= 1` — assert the exact count, enum, or object so unexpected
  extra results are caught. When one error is expected, assert *exactly one*, its full message, and
  its `path`/field.
- **On negative / permission-denied tests, also assert the side effect did NOT happen** — balance
  unchanged, `SomeEvent.objects.exists() is False`, the object left unmutated. An error assertion
  alone doesn't prove the action was blocked.