diff --git a/CLAUDE.md b/CLAUDE.md index e99d2be..be6d24d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,50 +6,32 @@ the browser page over that API, in Vue, and nothing in Go imports it. ## How to run and test -**A hand run is `direnv exec . go run .` from the repo root, against the dev -database. Do not build a throwaway binary to `/tmp` for one, and do not seed a -synthetic database for one.** A test fixture is allowed, and it needs at least one -long note. - -`.envrc` pairs a dedicated dev database with the sync API's local dev stack, so -nothing run here touches production data. This is the whole file: +**A hand run is `go run .` from the repo root, against the dev database, with the +machine's config kept out. Do not build a throwaway binary to `/tmp` for one, and +do not seed a synthetic database for one.** A test fixture is allowed, and it needs +at least one long note. ```bash -export PROJECTS_DB="$HOME/.local/share/projects/dev.db" -export PROJECTS_SYNC=true -export PROJECTS_SYNC_URL="https://api.docker.localhost" +XDG_CONFIG_HOME="$(mktemp -d)" PROJECTS_DB="$HOME/.local/share/projects/dev.db" go run . list +XDG_CONFIG_HOME="$(mktemp -d)" PROJECTS_DB="$HOME/.local/share/projects/dev.db" go run . tui ``` -The dev API needs no token, because its proxy injects the user header. The -server's own repo starts it, and the first sync binds `dev.db` to it. Its seed -data carries notes one line long. A change about notes needs a real-length -markdown note written into the dev API first. - -⚠️ **`.envrc` is machine-local and is not in this repo. Check it exists before -running anything here.** Without it nothing is overridden, so `go run . tui` opens -the production database against the production API — which is the accident the -coupled-variables rule below exists to prevent, arrived at from the other -direction. `task setup`, which `worktree new` runs, links the primary -checkout's `.envrc` into a worktree. +⚠️ **Name both, every time.** A desk's config file holds `api`, which puts every +command in API mode against the production API and ignores `PROJECTS_DB`. So +`PROJECTS_DB` alone writes to production. An empty `XDG_CONFIG_HOME` leaves no +`api`, and the run opens the database `PROJECTS_DB` names. -**A shell without direnv's hook ignores `.envrc`, and Claude Code's tool shell is -one.** There, a bare `go run . list` reads production even with the file in place. -Load it explicitly: +`.envrc` is machine-local. `task setup`, which `worktree new` runs, links the +primary checkout's copy into a worktree. `TestRealDatabaseSurvivesEverySizeAndProject` +reads `PROJECTS_DB`, and skips without it. A `PROJECTS_SYNC*` line left in an +`.envrc` stops every run with exit 2. -```bash -direnv exec . go run . tui # the board against dev.db -direnv exec . go run . list # a command against dev.db -direnv exec . go run . sync # one reconcile against the dev API -direnv exec . env | rg '^PROJECTS' # nothing printed: no .envrc here -``` +**A retired variable refuses from one table.** `config/retired_variables.go` holds it, `main.go` +refuses any variable in it that is set before anything runs, and each retirement adds its rows from +its own file. -`.envrc` turns sync on, so a command that writes refuses there, naming `api`. Reads -answer from `dev.db`. A write is checked in the board, or through `both_modes_test.go`. -A config file that sets `api` beside the `PROJECTS_SYNC=true` from `.envrc` refuses -every command, naming both, so the pair cannot cross into production that way either. - -**The former name is handled in one file.** `config/former_name.go` refuses a retired variable that is -still set, and a config still at the former path. It moves the old data directory on the first run +**The former name is handled in one file.** `config/former_name.go` adds the `TODOUI_*` variables to +that table, and refuses a config still at the former path. It moves the old data directory on the first run that opens the default database. A run naming its database moves nothing, so a run from `.envrc` leaves the real data where it is. An `.envrc` still exporting the old names is refused rather than ignored, which keeps it from falling through to the production database. On a machine upgrading, run `projects` @@ -58,6 +40,13 @@ marker there says when the file retires. Anything that opens the database stays `MoveDataUnderFormerName` in `openDatabase` in `cli/data_source.go`, because opening first creates an empty database, and the move refuses to run over one. +**The synced copy's leftovers retire together, under the `MIGRATION` marker in +`config/retired_sync.go`.** Its rows refuse a set `PROJECTS_SYNC*` or `TODOUI_SYNC*` variable with +exit 2, because `PROJECTS_SYNC_URL` chose an API. `Config.RetiredSync` keeps a `[sync]` section +loading, and `warnOfRetiredSyncSection` in `cli/data_source.go` warns of it rather than refusing, +because a machine's config changes only at its next apply. `dropSyncTables` in `store/sqlite/open.go` +drops the synced copy's own tables on the first open, and refuses while its push queue holds a row. + Build, vet and unit tests check code correctness, not TUI correctness. A TUI change is verified by driving the built binary, which the flows below do with nobody at the keyboard. @@ -80,8 +69,7 @@ go test -run TestTheCommandsAnswerTheSameInBothModes . # the commands, in both m ``` - **`seedFixtureDatabase` writes through `service.Service`**, the path the TUI writes through, so - the fixture holds only what the tool's own rules allow. There is no stand-in of the sync API, and sync - stays off because no `PROJECTS_SYNC` reaches the program. + the fixture holds only what the tool's own rules allow. - **`XDG_CONFIG_HOME` points under the test's temporary directory**, so the machine's own config file never reaches the program. - **The API is `routes.New` over a seeded SQLite database in an `httptest.Server`.** `serveOverHTTP` @@ -117,9 +105,9 @@ Ports and adapters at the storage seam, so a second store runs the same rules an ```text cli commands tui api/main.go │ apiclient.Client │ backend.Backend, which │ - │ │ sync.SyncBackend and │ - ▼ │ apiclient.Backend also ▼ -api/routes, in process │ satisfy api/routes + │ │ apiclient.Backend also │ + ▼ │ satisfies ▼ +api/routes, in process │ api/routes │ in database mode; │ │ *service.Service, scoped │ across HTTP to │ │ to each request by │ projects-api otherwise │ │ WithContext @@ -132,7 +120,7 @@ store/postgres the same, with goose migrations and an audit trigger ``` - **`store/sqlite` opens the database and holds no decision.** It owns the schema, the migrations, - `queries.sql` and sqlc's `generated`. It turns the driver's no-row answer, and each unique answer + `queries.sql` and sqlc's `internal/generated`, which nothing outside it can import. It turns the driver's no-row answer, and each unique answer `store/store.go` promises a sentinel for, into `model` sentinels, so `service` matches no driver text and imports no `database/sql`. - **`service.Service` satisfies `backend.Backend`**, so the TUI reaches every rule through that @@ -157,19 +145,12 @@ store/postgres the same, with goose migrations and an audit trigger `store/store.go` states each on its method: a new row's position, an item's `updated_at`, and a restore's handling of a row already there. Each store states them in its own `queries.sql`. -Two carve-outs are declared here rather than assumed: +One carve-out is declared here rather than assumed: -- **`sync` reaches `store/sqlite/generated` directly.** A pull rewrites rows wholesale, with no undo entry - and none of the rules, and the push queue and origin binding belong to no rule. Routing that through - `service` would give the rules a surface only the engine calls. A pull writes only the columns it has - always carried, so on a synced database `kind` and `completed_at` are what that database set, and neither - arrives from elsewhere. It ends when the sync engine is deleted. `generated` can then move under - `store/sqlite/internal`, where the compiler enforces the seam. - **Database mode links `service` and `api/routes` directly.** A client normally depends on the wire contract and never on a server's types, because importing them couples deploy order. Database mode serves itself, so that reason does not reach it. `apiclient` speaks `api/wire` and converts to - `model` for the board, and imports no route, service or store. The sync engine keeps its own lean - structs. + `model` for the board, and imports no route, service or store. ## `projects` is a command line, and `projects tui` is the board @@ -180,12 +161,6 @@ builds the tree. Help renders through `gotermstyle/cobrahelp`. - **The `api` key, or `--api`, chooses the mode, and neither means the database.** `decideMode` in `cli/data_source.go` reads goclilogin's parse of the key, so the mode and the login read one setting. It runs in each command's `RunE`, so `help`, `completion` and `version` open nothing. -- **`api` beside sync refuses**, through `chooseMode`, the one check every command, `projects sync` - and `auth login` take. The two are two authorities for the same rows. The refusal names - `PROJECTS_SYNC` when the environment turned sync on, and the config file's `[sync]` otherwise. -- **With sync on, a command that writes refuses before it asks anything.** `openForWriting` holds the - sentence, and `refusingWrites` answers 409 with it for any write that reaches the in-process routes - without it. A write there would land in the copy with nothing queued, and the next pull deletes it. - **Exit codes are 0, 1 for refused or failed, 2 for usage, and 4 for no answer.** `exitCodeFor` in `cli/root.go` maps them, and `explain` in `cli/api_errors.go` words them. A 422 is usage. A refused credential is 1. @@ -209,17 +184,13 @@ builds the tree. Help renders through `gotermstyle/cobrahelp`. an API serves tags written from every machine. `repoRegistry` on the data source answers an empty registry in API mode. -An item shows its **number**, the handle the API assigns and answers with. The UUID is the sync key +An item shows its **number**, the handle the API assigns and answers with. The UUID is the key and stops there. -The number is assigned by whichever database is the authority. Through an API, the API numbers it as -it creates it. With sync on that is the API, and -`pushCreatedItem` writes the number from the create response into the local row, so the TUI shows -it once the push lands rather than after the next pull. With sync off, `AssigningItemNumbers` makes -`projects` allocate `max+1` itself, which cannot collide because that database is a disjoint universe. - -An item created while the API is unreachable has no number until its push lands, and shows its UUID -tail until then (`itemHandle` in `tui/app.go`). +`service.CreateItem` numbers each item as it is created, from the store's `NextItemNumber`: the +`item_numbers` sequence on Postgres, and `max+1` on SQLite. An item with no number, which only a +database from before numbers or from the synced copy holds, shows its UUID tail (`itemHandle` in +`tui/app.go`). ## A project name is bounded work, never a repo @@ -284,75 +255,8 @@ adding columns, because SQLite attaches the old column-level `UNIQUE(name)` as an implicit index no DDL can drop. Foreign keys are off across the swap: `project_item_memberships` cascades off `projects`, so dropping the old table with them on would take every membership. Do not simplify it into three -`ALTER TABLE ADD COLUMN`s — leaving the global constraint in place fails the -entire pull transaction the first time the server returns a done project beside -a live one sharing its name. - -## A database and its API are a pair — never mix a dev half with a prod half - -`.envrc` exports two coupled variables, `PROJECTS_DB` and `PROJECTS_SYNC_URL`, and -`PROJECTS_SYNC_TOKEN_COMMAND` where the dev API wants a token. They name one -environment. A URL from the environment never takes the config file's -`token_command` (`config.Load`), so the dev URL cannot carry the production -token. Overriding one of them names an environment that does not exist, and -for this tool that is destructive rather than merely wrong: a pull deletes -every local row the server did not return, so -a production database reconciled against the dev API is emptied of everything -dev has never heard of. A sync run that way once deleted 42 projects and 286 -items and printed `Synced.` - -**To run against production data, run from outside this repo**, where direnv has -loaded nothing. Un-setting your way out is how the accident happened: `env -u` -on the variable you remembered still leaves the one you did not. - -`sync_origin` is the enforcement, because a rule only prose enforces is -decoration. The first pull records which API the database belongs to, and every -pull and push after that refuses any other (`OriginMismatch`). `--adopt` is the -only way the pairing changes. A database that has pulled before but carries no -origin — anything predating this — refuses to guess and asks to be bound, since -inferring it from whichever environment is loaded *is* the mistake. - -`guardSweep` is the second layer, for when the API is right but its answer is -not. A pull that would delete ten or more local projects, and more than half of -them, is refused, and items are guarded the same way. A truncated response, an -auth failure that still returns 200, and a genuine mass deletion are the same -event seen from here. `--force` allows it. The floor counts rows deleted, not -rows held, so 8 deletions out of 15 pass. It keeps the guard silent on small -databases, where losing most of it is a handful of rows; a guard that fires on -ordinary days gets forced past by reflex and stops guarding anything. - -Neither override is reachable from the background timers. A guard something -automatic can waive is not a guard. - -## Sync is automatic; `projects sync` is a convenience, not a requirement - -`sync.interval` (default 2m, floored at 15s) is the single knob for how stale `projects` may ever be, -and it drives two loops: - -- **TUI** — `syncPullTickMsg` re-arms itself every interval, and always reschedules, even on the - ticks it skips: a tick that returns no command kills background sync for the session. - `safeToAutoPull` gates the reconcile to `modeNormal`, because a pull rewrites items, memberships - and ordering wholesale and would move the ground under a grab or a text entry. -- **Push** — `pushLoop` carries a retry ticker alongside `Notify`, which fires only on a local - mutation, so a push that failed while the API was down retries without another edit. - -An automatic pull neither flashes on success nor claims the status bar on failure; failure surfaces -through the engine status (`SYNC ERR`). Local-first means an unreachable API degrades to local -data, never an error. - -`Pull` records a pending-sync high-water mark before it fetches, then clears only the ops at or -below it and spares entities queued above it from the "deleted upstream" sweeps. Without that mark, -an item typed mid-pull is deleted by the same pull and its queued create dropped with it. Do not -restore `DeleteAllPendingSync` here. - -**Absent is not empty, and that is what keeps deploy order between this repo and its server -irrelevant.** The item list embeds memberships, dependencies and tasks; when the server omits -`dependency_ids`/`tasks`, the pull falls back to the per-item endpoints — decode into slices and -branch on nil, never on empty, or the first pull against an older API deletes every local task and -dependency. The project pull asks for `?status=all` because the sweep deletes any local project the -server did not return, so an active-only response would read "completed" as "deleted" and cascade -through the memberships. A server sending no project status is read as `active`. Do not drop either -fallback just because the API has shipped. +`ALTER TABLE ADD COLUMN`s — leaving the global constraint in place refuses a +done project beside a live one sharing its name. ## The API is `service` over Postgres, and a handler holds no rule @@ -400,7 +304,7 @@ per test. Set `TEST_DATABASE_URL` to use a running server. `-short` skips them, `lookup_rows.sql`. `postgres.Migrate` upserts Postgres's from `model`, and `TestEachStoreHoldsTheKnownKindsAndStatuses` holds both stores to it. The project schemas' `enum` and the web app's `types.ts` list them too, and a test holds each to `model`. -- **Item numbers come from the `item_numbers` sequence**, through `AssigningItemNumbers`. A database +- **Item numbers come from the `item_numbers` sequence**, through `NextItemNumber`. A database loaded from elsewhere needs the sequence set from the old sequence's own counter, because numbers it already handed out can outrun the rows left. - **`.sqlfluffignore` keeps the SQLite hook off `store/postgres`**, and a second hook in diff --git a/README.md b/README.md index 4509f85..a611086 100644 --- a/README.md +++ b/README.md @@ -147,7 +147,7 @@ empty database at the old path, and `projects` then stops until that file is gon Move `~/.config/todoui/config.toml` to `~/.config/projects/config.toml` first. While only the old file exists, every run that opens a database stops with exit 1, naming both paths. Reading no config would -turn sync off, and the next pull deletes what was written with it off. +drop its `api` and send every command to a database on this machine. The first run that opens the default database moves everything in `$XDG_DATA_HOME/todoui` to `$XDG_DATA_HOME/projects`. `todoui.db` becomes `projects.db`, and its `-wal` and `-shm` files go with @@ -169,7 +169,7 @@ j / k move through items and their tasks as one list J / K jump item to Tab switch pane Enter read it in full a add a project, or an item to this project space mark done T dependency browser / search every project -u undo the last change R sync, or read the API, now +u undo the last change R read the data again now ? every key the pane you are in takes q quit ``` @@ -183,7 +183,7 @@ items it waits on. The reader also names the repo the item is for. The board has that tag. `projects items edit --repo ` sets it. Through an API, the status bar reads `API` and the API's host. The board reads the API again every -`sync.interval`, and `R` reads it at once. +`apiRereadInterval` in `tui/app.go`, and `R` reads it at once. `Enter` opens the reader, a full screen holding one item or one project. An item shows its fields, what blocks it, what it blocks, its tasks, and its notes rendered as markdown. A project shows its @@ -247,70 +247,32 @@ next round of them. ## An item's handle is its number, and its key is a UUID An item shows its number, the same handle the API answers with. The number is not the primary key. -`projects` creates every id as a UUIDv7, so an item can be created offline and pushed without a round -trip to claim one. +`projects` creates every id as a UUIDv7. -Through an API, the API numbers the item as it creates it. With sync on, the number comes from the -API's response to the create, and the TUI shows it once the push lands. In database mode with sync -off, `projects` allocates the next number itself. An item created while a synced API is unreachable -shows the last 8 characters of its id until its push lands and earns it a number. +Through an API, the API numbers the item as it creates it. In database mode, `projects` allocates the +next number itself. An item with no number shows the last 8 characters of its id. Only a database +from before numbers existed, or one kept as a synced copy of an API, can hold such an item. -## Sync keeps a database a copy of an API, and only the board writes to the copy +## A synced copy's config and database still load, while a sync variable or an unpushed edit refuses -`[sync] enabled = true` keeps a database on this machine as a -copy of `sync.api_url`, and `projects tui` edits the copy and pushes each edit. With `api` set there -is no copy, so a config file holding both `api` and `[sync] enabled = true` stops every command, -naming both keys. +A config holding `[sync]`, a `PROJECTS_SYNC*` variable, or a database that was a synced copy of an +API behaves as follows. `api` is how a machine works through an API. -With sync on, every read on the command line answers from the copy. A command that writes refuses -with a sentence naming `api`. Its write would land in the copy with nothing queued to push, and the -next pull would delete it. `projects sync` refuses when `api` is set, because there is nothing to -reconcile. - -With sync on, every edit in the board is queued and pushed as you make it. The TUI pulls on launch, then again -every `sync.interval` (default two minutes) for as long as it is open. A pull waits while a text -input, an overlay, the reader or a move is open, and runs on the next tick instead. - -Queued pushes retry on a timer. A change made while the API was unreachable is sent once it comes -back, with no further edit to trigger it. - -The status bar names the API's URL beside one of `SYNCING...`, `PENDING (n)`, `SYNC ERR` or -`SYNCED`. An unreachable API shows `SYNC ERR` and leaves the local data in place. `R` in the TUI or -`projects sync` forces a reconcile. - -A pull is two requests, because the item list embeds memberships, dependencies and sub-tasks. -Against an API that does not embed them, it falls back to two more requests per item. Lower -`interval` to sync more eagerly. It is floored at 15 seconds. - -## A database belongs to one API, and a pull that would empty it stops - -```bash -projects sync # push anything queued, then pull, however recently the last pull ran -projects sync --adopt # bind this database to the configured API, replacing the old binding -projects sync --force # allow a pull that would delete most of the projects or items here -``` - -A pull deletes every local row the server did not return, so a database and its API are a pair. -The first pull records which API the database belongs to. Every pull and push after that refuses any -other. A database pointed at the wrong API would lose everything that API has never heard of, and -the reconcile cannot tell that from a real deletion. - -A database that has pulled before but records no API refuses as well. It asks for `--adopt` rather -than guessing from whatever is configured, because guessing from the loaded configuration is the -mistake the binding exists to stop. - -The second guard is for the right API giving a wrong answer. A pull that would delete ten or more -projects, and more than half of them, stops and asks for `--force`. Items are guarded the same way. -A truncated response, an authentication failure that still returns `200`, and a genuine mass -deletion look identical from the client. The floor keeps the guard quiet on a small database, where -losing most of it is a handful of rows. - -Neither flag is reachable from the background timers. +- A config file still holding `[sync]` loads. Every command that opens data warns on stderr, naming + the file, and ignores the section. +- A database that was a synced copy opens as it is. The first open drops its queue of pushes, its pull + times and the API it was bound to. Every project, item, task, dependency and membership stays. +- A queue still holding an edit stops that open with exit 1, naming how many and the API they were + for, because those edits never reached it. `projects sync` from v1.24.2 pushes them, and + `DELETE FROM pending_sync` discards them. +- `PROJECTS_SYNC`, `PROJECTS_SYNC_URL`, `PROJECTS_SYNC_TOKEN_COMMAND` or `PROJECTS_SYNC_INTERVAL`, or + its `TODOUI_` twin, set to a value stops every command with exit 2. `PROJECTS_SYNC_URL` chose an + API, so ignoring it would send the run wherever the config file points. ## Configuration — one TOML file, every key overridable from the environment `projects` reads `$XDG_CONFIG_HOME/projects/config.toml`, else `~/.config/projects/config.toml`, on macOS -as well as Linux. No config file is a valid configuration: a local database, with sync off. +as well as Linux. No config file is a valid configuration: a local database. [config.example.toml](config.example.toml) is an annotated copy. ```toml @@ -324,43 +286,16 @@ as well as Linux. No config file is a valid configuration: a local database, wit [local] # db_path = "/custom/path/projects.db" # default: ~/.local/share/projects/projects.db - -[sync] -enabled = false -# api_url = "https://example.com" # the server's host; the tool adds /api/v1 -# token_command = " auth token" # prints a bearer token for api_url -# interval = "2m" # longest it goes without reconciling; floored at 15s ``` -`enabled = true` without an `api_url` is an error. So is `enabled = true` beside `api`. - -The bare `token_command` belongs to `api`, and is an argument list printing a token response. The -`[sync]` one belongs to `api_url`, and is a string printing a bare token. The two never stand in for -each other, and either written in the other's shape is refused, naming the shape it takes. - -`[sync]`'s `token_command` is how `projects` authenticates to the API without holding a secret itself. It is split -on spaces and run without a shell or stdin. Whatever it prints becomes the bearer token. A command -that has not finished in 10 seconds fails. `projects` keeps the token until the API refuses it, then -runs the command once more and retries once. A failing command's first line of stderr is what the -status bar shows. Leave it unset for an API that needs no token, such as a dev stack that sets its -own identity. - -The command belongs to the URL beside it. When `PROJECTS_SYNC_URL` supplies the URL, the config -file's `token_command` is ignored and `PROJECTS_SYNC_TOKEN_COMMAND` is the only source. Overriding the -URL never sends one API's token to another. - Each variable overrides the key beside it: | Variable | Overrides | | -------- | --------- | | `PROJECTS_DB` | `local.db_path` | -| `PROJECTS_SYNC` | `sync.enabled` (`true` or `false`) | -| `PROJECTS_SYNC_URL` | `sync.api_url` | -| `PROJECTS_SYNC_TOKEN_COMMAND` | `sync.token_command` | -| `PROJECTS_SYNC_INTERVAL` | `sync.interval`, as a duration such as `30s` or `5m` | | `PROJECTS_REPOS_REGISTRY` | `repos_registry` | -`api`, `issuer`, `client_id` and the bare `token_command` take no variable. `--api` outranks `api` for +`api`, `issuer`, `client_id` and `token_command` take no variable. `--api` outranks `api` for one run. `PROJECTS_CLIENT_SECRET` switches the login to the client-credentials grant. ## Every command speaks HTTP to the API's routes, served in the process or across the network @@ -375,8 +310,8 @@ projects projects tui v v apiclient.Client backend.Backend | | - +-- database mode +-- database mode: service.Service, - | InProcess -> api/routes | in SyncBackend when sync is on + +-- database mode +-- database mode: service.Service + | InProcess -> api/routes | | -> service.Service | | -> store/sqlite +-- api: apiclient.Backend | -> apiclient.Client @@ -384,14 +319,10 @@ apiclient.Client backend.Backend -> projects-api ``` -The sync engine, when it is on, sits in the board's path alone. It pushes each queued write over -HTTP and pulls the API's state into the local database. - The data model is projects, items, the memberships joining them many to many, dependencies between items, and sub-tasks. -In database mode, the database at `db_path` holds the data, the queue of pending pushes, and the API -the database is bound to. The update check keeps its last result under `$XDG_STATE_HOME/projects/`, +In database mode, the database at `db_path` holds the data and the undo log. The update check keeps its last result under `$XDG_STATE_HOME/projects/`, which defaults to `~/.local/state/projects/`. `auth login` is the only command that writes the config file, and it adds its keys without rewriting the rest. The repo registry is read and never written. diff --git a/api/main.go b/api/main.go index b0e95d4..c399051 100644 --- a/api/main.go +++ b/api/main.go @@ -103,7 +103,7 @@ func serve(ctx context.Context, log *slog.Logger) error { log.WarnContext(ctx, "WEB_CLIENT_SECRET is unset, so the API logs no browser in and only a bearer token reaches it") } - svc := service.New(postgresStore, service.AssigningItemNumbers()) + svc := service.New(postgresStore) mux := routes.New(svc, log, checks...) if loaded.PublicURL != "" { authentication.Metadata(mux, loaded.PublicURL, loaded.OIDCIssuer) diff --git a/api/routes/routes_test.go b/api/routes/routes_test.go index 9b4a06b..a9a79ee 100644 --- a/api/routes/routes_test.go +++ b/api/routes/routes_test.go @@ -45,7 +45,7 @@ func newAPI(t *testing.T) api { provider := authtest.NewProvider(t) config := authentication.Config(provider.URL) verifier := provider.Verifier(t, config.PersonPrefix, config.ServicePrefix) - mux := routes.New(service.New(sqlite.New(database), service.AssigningItemNumbers()), log) + mux := routes.New(service.New(sqlite.New(database)), log) authentication.Metadata(mux, "https://projects.example.com", provider.URL) protected := authentication.Protect(mux, routes.ServiceScopes, verifier, nil, "", log, routes.PublicPaths...) diff --git a/backend/backend.go b/backend/backend.go index 31518c5..2367cd2 100644 --- a/backend/backend.go +++ b/backend/backend.go @@ -2,8 +2,8 @@ package backend import "github.com/datapointchris/projects/model" -// Backend defines the interface for all data operations. -// service.Service holds the rules over a store; SyncBackend wraps it with background push/pull. +// Backend defines the interface for all data operations. service.Service holds +// the rules over a store, and apiclient.Backend reaches them through the API. type Backend interface { // Projects // ListProjectsByStatus takes a status name or model.StatusAll. diff --git a/cli/auth.go b/cli/auth.go index 340c85c..6404228 100644 --- a/cli/auth.go +++ b/cli/auth.go @@ -1,7 +1,6 @@ package cli import ( - "cmp" "context" "errors" "fmt" @@ -14,8 +13,6 @@ import ( "github.com/pkg/browser" "github.com/spf13/cobra" "golang.org/x/oauth2" - - "github.com/datapointchris/projects/config" ) // loginTimeout is a backstop. The device code carries its own expiry, and the @@ -76,9 +73,6 @@ func newAuthLoginCommand() *cobra.Command { } url = args[0] } - if err := refuseLoginBesideSync(url); err != nil { - return err - } ctx, cancel := context.WithTimeout(cmd.Context(), loginTimeout) defer cancel() @@ -130,18 +124,6 @@ func newAuthLoginCommand() *cobra.Command { return cmd } -// refuseLoginBesideSync refuses a login that would write api beside a synced -// database, because every command after it would refuse the pair. -func refuseLoginBesideSync(url string) error { - cfg, err := config.Load() - if err != nil { - return err - } - _, err = chooseMode(cfg, cmp.Or(url, "the API it logs in to"), - "logging in writes api to "+homeAsTilde(configFilePath())) - return err -} - func newAuthLogoutCommand() *cobra.Command { return &cobra.Command{ Use: "logout", diff --git a/cli/data_source.go b/cli/data_source.go index 26fd23a..3509b57 100644 --- a/cli/data_source.go +++ b/cli/data_source.go @@ -3,7 +3,6 @@ package cli import ( "context" "database/sql" - "encoding/json" "errors" "fmt" "io" @@ -21,14 +20,12 @@ import ( "github.com/datapointchris/projects/api/requestlog" "github.com/datapointchris/projects/api/routes" - "github.com/datapointchris/projects/api/wire" "github.com/datapointchris/projects/apiclient" "github.com/datapointchris/projects/backend" "github.com/datapointchris/projects/config" "github.com/datapointchris/projects/repos" "github.com/datapointchris/projects/service" "github.com/datapointchris/projects/store/sqlite" - "github.com/datapointchris/projects/sync" ) // inProcessBase is the address the client is given when the routes run inside @@ -58,17 +55,11 @@ type dataSource struct { // Database mode. database *sql.DB local *service.Service - engine *sync.Engine - started bool - - // writeRefusal is set where a write through the routes would be lost. - writeRefusal string } -// LocalService is the service over a database this machine owns outright, which -// numbers items itself because nothing upstream will. +// LocalService is the service over a database this machine owns outright. func LocalService(database *sql.DB, reposRegistryPath string) *service.Service { - return service.New(sqlite.New(database), service.AssigningItemNumbers(), service.RefusingRepoNames(reposRegistryPath)) + return service.New(sqlite.New(database), service.RefusingRepoNames(reposRegistryPath)) } // runMode is where a run's data lives. @@ -81,7 +72,7 @@ const ( // openDataSource opens where this run's data lives, as decideMode answers it. func openDataSource(cmd *cobra.Command) (*dataSource, error) { - cfg, deployment, mode, err := decideMode(cmd.Context()) + cfg, deployment, mode, err := decideMode(cmd) if err != nil { return nil, err } @@ -92,9 +83,10 @@ func openDataSource(cmd *cobra.Command) (*dataSource, error) { } // decideMode reads the config file and the login settings in it, and answers -// which store this run uses. goclilogin's reading of api is the one the mode -// follows, because it is the one every request is then sent with. -func decideMode(ctx context.Context) (*config.Config, goclilogin.Deployment, runMode, error) { +// which store this run uses: the API api names, or this machine's database when +// it names none. goclilogin's reading of api is the one the mode follows, because +// it is the one every request is then sent with. +func decideMode(cmd *cobra.Command) (*config.Config, goclilogin.Deployment, runMode, error) { if err := config.RefuseConfigUnderFormerName(); err != nil { return nil, goclilogin.Deployment{}, databaseMode, err } @@ -102,10 +94,11 @@ func decideMode(ctx context.Context) (*config.Config, goclilogin.Deployment, run if err != nil { return nil, goclilogin.Deployment{}, databaseMode, err } - deployment, loadErr := loadDeployment(ctx, apiOverride) - mode, err := chooseMode(cfg, deployment.API.Value, apiPhrase(deployment.API)) - if err != nil { - return nil, deployment, mode, err + warnOfRetiredSyncSection(cmd, cfg) + deployment, loadErr := loadDeployment(cmd.Context(), apiOverride) + mode := databaseMode + if deployment.API.Value != "" { + mode = apiMode } // With no api, the deployment's only error worth stopping for is a file it // could not read: a broken api line must not fall back to the database. @@ -115,52 +108,16 @@ func decideMode(ctx context.Context) (*config.Config, goclilogin.Deployment, run return cfg, deployment, mode, nil } -// chooseMode is the one answer to where a run's data lives: the API api names, or -// this machine's database when it names none. A synced database beside an API is -// refused, because the two are authorities for the same rows. apiPhrase says -// what named the API. -func chooseMode(cfg *config.Config, api, apiPhrase string) (runMode, error) { - if api == "" { - return databaseMode, nil +// warnOfRetiredSyncSection names a [sync] section the run ignores. It warns rather +// than refuses, because a machine's config is rewritten only by its next apply, +// and a refusal would stop the tool there until then. +func warnOfRetiredSyncSection(cmd *cobra.Command, cfg *config.Config) { + if cfg.RetiredSync == nil { + return } - if cfg.Sync.Enabled { - chosenBy, remedy := syncChosenBy() - return apiMode, fmt.Errorf("%s, and %s keeps a synced database, which are two authorities for the same rows; "+ - "%s to work through the API", apiPhrase, chosenBy, remedy) - } - return apiMode, nil -} - -// apiPhrase names what chose the API, as the first thing a person checks. -func apiPhrase(api goclilogin.Setting) string { - if api.Source == goclilogin.SourceCommandLine { - return "--api sends this command to " + api.Value - } - return fmt.Sprintf("api in %s sends every command to %s", homeAsTilde(api.From), api.Value) -} - -// syncChosenBy names what turned sync on and how to turn it off. The variable -// outranks the file, so it is named whenever it is set. -func syncChosenBy() (chosenBy, remedy string) { - if value := os.Getenv(config.SyncVariable); value != "" { - return fmt.Sprintf("%s=%s in the environment", config.SyncVariable, value), "unset " + config.SyncVariable - } - path := homeAsTilde(configFilePath()) - return "[sync] enabled = true in " + path, "remove [sync] from " + path -} - -// openForWriting is openDataSource for a command that writes, refused before it -// asks anything where the write would be lost. -func openForWriting(cmd *cobra.Command) (*dataSource, error) { - source, err := openDataSource(cmd) - if err != nil { - return nil, err - } - if source.writeRefusal != "" { - _ = source.Close() - return nil, errors.New(source.writeRefusal) - } - return source, nil + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "warning: [sync] in %s is ignored, because projects keeps no synced copy "+ + "of an API. Remove the section. `projects auth login ` writes api, which sends every command to an API.\n", + homeAsTilde(configFilePath())) } func openAPI(ctx context.Context, cfg *config.Config, deployment goclilogin.Deployment) (*dataSource, error) { @@ -229,65 +186,26 @@ func openDatabase(cfg *config.Config) (*dataSource, error) { } database, err := sqlite.Open(cfg.Local.DBPath) if err != nil { - return nil, fmt.Errorf("opening database: %w", err) - } - source := &dataSource{cfg: cfg, database: database} - if cfg.Sync.Enabled { - source.local = service.New(sqlite.New(database), service.RefusingRepoNames(repos.DefaultPath())) - source.engine = sync.New(database, cfg.Sync.APIURL, cfg.Sync.TokenCommand) - chosenBy, remedy := syncChosenBy() - source.writeRefusal = fmt.Sprintf("%s keeps this database a copy of %s, and a write from a command would "+ - "never reach it; make the change in `projects tui`, or %s and set api in %s to write through the projects API", - chosenBy, cfg.Sync.APIURL, remedy, homeAsTilde(configFilePath())) - } else { - source.local = LocalService(database, repos.DefaultPath()) + return nil, fmt.Errorf("opening %s: %w", homeAsTilde(cfg.Local.DBPath), err) } + source := &dataSource{cfg: cfg, database: database, local: LocalService(database, repos.DefaultPath())} // A fault in a route is logged with its cause, which the answer only names by // request id. The request line itself is for a server's log, not a terminal. faults := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) quiet := slog.New(slog.NewTextHandler(io.Discard, nil)) handler := requestlog.Middleware(quiet, routes.New(source.local, faults)) - if source.writeRefusal != "" { - handler = refusingWrites(handler, source.writeRefusal) - } source.client = apiclient.New(inProcessBase, &http.Client{Transport: apiclient.InProcess(handler)}) return source, nil } -// refusingWrites answers every request but a read with the sentence saying why. -// It backs up openForWriting for any write a command reaches without it. -func refusingWrites(next http.Handler, sentence string) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodGet { - next.ServeHTTP(w, r) - return - } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusConflict) - _ = json.NewEncoder(w).Encode(wire.Error{Error: http.StatusText(http.StatusConflict), Message: sentence}) - }) -} - -// startSync starts the background push loop, for the commands that sync. -func (s *dataSource) startSync() { - if s.engine != nil && !s.started { - s.engine.Start() - s.started = true - } -} - // boardBackend is what the TUI reads and writes: the API over the client, or the -// database's own service, wrapped to queue each write when sync is on. +// database's own service. func (s *dataSource) boardBackend() backend.Backend { - switch { - case s.deployment != nil: + if s.deployment != nil { return apiclient.NewBackend(s.client) - case s.engine != nil: - return sync.NewSyncBackend(s.local, s.engine) - default: - return s.local } + return s.local } // repoRegistry is what a repo tag is checked against: this machine's registry in @@ -312,9 +230,6 @@ func (s *dataSource) apiHost() string { } func (s *dataSource) Close() error { - if s.engine != nil && s.started { - s.engine.Stop() - } if s.database != nil { return s.database.Close() } diff --git a/cli/item_create.go b/cli/item_create.go index d567421..2841a3c 100644 --- a/cli/item_create.go +++ b/cli/item_create.go @@ -49,7 +49,7 @@ func newItemsCreateCommand() *cobra.Command { return goclikit.UsageError(fmt.Errorf("%s required — pass %s, or run from a terminal to be asked", strings.Join(missing, " and "), pronoun)) } - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } diff --git a/cli/item_dependencies.go b/cli/item_dependencies.go index b4fe0e1..bb16f91 100644 --- a/cli/item_dependencies.go +++ b/cli/item_dependencies.go @@ -48,7 +48,7 @@ func newDependenciesAddCommand() *cobra.Command { if dependsOn == "" { return goclikit.UsageError(errors.New("--depends-on is required")) } - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } @@ -83,7 +83,7 @@ func newDependenciesRemoveCommand() *cobra.Command { if dependsOn == "" { return goclikit.UsageError(errors.New("--depends-on is required")) } - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } @@ -316,8 +316,7 @@ func crossProjectTag(item, parent wire.ProjectItem, hasParent bool) string { // drawing, so a consumer wanting another shape parses no box-drawing. // cli/schemas/dependency-tree.schema.json is its shape. An edge runs from an item // to what it depends on whichever way the drawing ran, and only roots follows -// --invert. Edges and roots name items by id, because an item made on a synced -// database has no number until its create reaches the API. +// --invert. Edges and roots name items by id, because an item may have no number. type treeShown struct { SchemaVersion int `json:"schema_version"` Nodes []treeNode `json:"nodes"` diff --git a/cli/item_memberships.go b/cli/item_memberships.go index 7287fa0..f4c0fc4 100644 --- a/cli/item_memberships.go +++ b/cli/item_memberships.go @@ -36,7 +36,7 @@ func newMembershipsAddCommand() *cobra.Command { if project == "" { return goclikit.UsageError(errors.New("--project is required")) } - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } @@ -71,7 +71,7 @@ func newMembershipsRemoveCommand() *cobra.Command { if project == "" { return goclikit.UsageError(errors.New("--project is required")) } - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } diff --git a/cli/item_tasks.go b/cli/item_tasks.go index ece2b78..cb20ba6 100644 --- a/cli/item_tasks.go +++ b/cli/item_tasks.go @@ -78,7 +78,7 @@ func newTasksAddCommand() *cobra.Command { if cmd.Flags().Changed("position") { input.Position = &position } - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } @@ -154,7 +154,7 @@ func newTasksEditCommand() *cobra.Command { } func runTaskUpdate(cmd *cobra.Command, itemRef, taskID string, edit apiclient.TaskChanges, asJSON bool) error { - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } @@ -178,7 +178,7 @@ func newTasksRemoveCommand() *cobra.Command { Example: " projects items tasks remove 118 019a2b3c-… --yes", Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } diff --git a/cli/items.go b/cli/items.go index a5ee85a..3427913 100644 --- a/cli/items.go +++ b/cli/items.go @@ -328,7 +328,7 @@ func newItemsEditCommand() *cobra.Command { if edit == (apiclient.ItemChanges{}) { return goclikit.UsageError(errors.New("nothing to change — pass --title, --notes or --repo")) } - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } @@ -372,7 +372,7 @@ func newItemStateCommand(verb, short, done string, edit apiclient.ItemChanges) * } func runItemUpdate(cmd *cobra.Command, ref string, edit apiclient.ItemChanges, asJSON bool, done string) error { - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } @@ -401,7 +401,7 @@ func newItemsDeleteCommand() *cobra.Command { Example: " projects items delete 118 --yes", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } @@ -449,7 +449,7 @@ func newItemsReorderCommand() *cobra.Command { if !cmd.Flags().Changed("position") { return goclikit.UsageError(errors.New("--position is required")) } - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } diff --git a/cli/projects.go b/cli/projects.go index c602bc1..8bc6d16 100644 --- a/cli/projects.go +++ b/cli/projects.go @@ -168,7 +168,7 @@ func newProjectsCreateCommand() *cobra.Command { if cmd.Flags().Changed("position") { input.Position = &position } - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } @@ -332,7 +332,7 @@ func newProjectStatusCommand(verb statusVerb) *cobra.Command { } func runProjectUpdate(cmd *cobra.Command, ref string, edit apiclient.ProjectChanges, asJSON bool, done string) error { - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } @@ -359,7 +359,7 @@ func newProjectsDeleteCommand() *cobra.Command { Example: " projects delete \"Kitchen remodel\"\n projects delete \"Kitchen remodel\" --yes", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - source, err := openForWriting(cmd) + source, err := openDataSource(cmd) if err != nil { return err } diff --git a/cli/root.go b/cli/root.go index 670498a..a5814ee 100644 --- a/cli/root.go +++ b/cli/root.go @@ -137,7 +137,7 @@ func NewRootCommand() *cobra.Command { )...) root.AddCommand(inGroup(groupItems, withAPIFlag(newItemsCommand()))...) root.AddCommand(inGroup(groupScreen, withAPIFlag(newTUICommand()))...) - root.AddCommand(inGroup(groupSetup, newAuthCommand(), newSyncCommand(), VersionCmd(), UpdateCmd())...) + root.AddCommand(inGroup(groupSetup, newAuthCommand(), VersionCmd(), UpdateCmd())...) return root } diff --git a/cli/schemas/dependency-tree.schema.json b/cli/schemas/dependency-tree.schema.json index 5399199..cd5ada3 100644 --- a/cli/schemas/dependency-tree.schema.json +++ b/cli/schemas/dependency-tree.schema.json @@ -35,7 +35,7 @@ "id": { "type": "string", "format": "uuid" }, "number": { "type": ["integer", "null"], - "description": "The item's handle. Null only on a synced database whose API has not assigned one yet." + "description": "The item's handle. Null on an item from before numbers existed, or one a database kept as a copy of an API created before the API numbered it." }, "title": { "type": "string" }, "status": { "type": "string", "enum": ["open", "completed", "archived"] }, diff --git a/cli/sync.go b/cli/sync.go deleted file mode 100644 index 6f5ac02..0000000 --- a/cli/sync.go +++ /dev/null @@ -1,67 +0,0 @@ -package cli - -import ( - "errors" - "fmt" - - "github.com/spf13/cobra" - - "github.com/datapointchris/projects/sync" -) - -func newSyncCommand() *cobra.Command { - var adoptOrigin, forceSweep bool - cmd := &cobra.Command{ - Use: "sync", - Short: "Reconcile a synced database with its API now", - Long: "For a database with [sync] enabled = true in the config file. `projects tui` on\n" + - "such a database pushes each edit as it is made and pulls on every interval, so\n" + - "this is for when the API's state is wanted now: it pushes anything queued, then\n" + - "pulls.\n" + - "\n" + - "A pull deletes everything the server did not return, so the database is bound to\n" + - "the API it was first pulled from and refuses to reconcile against any other.\n" + - "--adopt rebinds it; --force allows a pull that would delete most of what is here.\n" + - "\n" + - "With api in the config file there is no copy to reconcile, and this refuses.", - Example: " projects sync push queued edits, then pull from the API now\n" + - " projects sync --adopt bind this database to the configured API, then sync\n" + - " projects sync --force sync even when the pull would delete most local rows", - Args: cobra.NoArgs, - RunE: func(cmd *cobra.Command, _ []string) error { - cfg, deployment, mode, err := decideMode(cmd.Context()) - if err != nil { - return err - } - if mode == apiMode { - return fmt.Errorf("%s, so there is no database copy to sync", apiPhrase(deployment.API)) - } - source, err := openDatabase(cfg) - if err != nil { - return err - } - defer func() { _ = source.Close() }() - if source.engine == nil { - return errors.New("sync is disabled — enable it under [sync] in " + homeAsTilde(configFilePath())) - } - // Named on the way in: --adopt rewrites which API owns this database, - // and doing that silently is the mistake the binding exists to prevent. - if adoptOrigin { - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "Binding %s to %s\n", source.cfg.Local.DBPath, source.engine.APIURL()) - } - if err := source.engine.PullWith(cmd.Context(), sync.PullOptions{ - Adopt: adoptOrigin, - AllowLargeSweep: forceSweep, - }); err != nil { - return err - } - _, _ = fmt.Fprintln(cmd.OutOrStdout(), "Synced.") - return nil - }, - } - cmd.Flags().BoolVar(&adoptOrigin, "adopt", false, - "bind this database to the configured API, replacing whatever it was bound to") - cmd.Flags().BoolVar(&forceSweep, "force", false, - "allow a pull that would delete most of the local projects or items") - return cmd -} diff --git a/cli/tables.go b/cli/tables.go index ef1bd59..0622059 100644 --- a/cli/tables.go +++ b/cli/tables.go @@ -171,8 +171,7 @@ func statusMark(status string) string { } } -// handle is an item's number, or the tail of its id while it has none: an item -// made on a synced database before its create reached the API. +// handle is an item's number, or the tail of its id where it has none. func handle(number *int, id string) string { if number != nil { return strconv.Itoa(*number) diff --git a/cli/tui.go b/cli/tui.go index 2c36e0a..8339c78 100644 --- a/cli/tui.go +++ b/cli/tui.go @@ -46,9 +46,8 @@ func newTUICommand() *cobra.Command { Use: "tui", Short: "Open the board: projects on the left, the selected one's items on the right", Long: "Everything the commands do can be done here by key; ? lists the keys. The status\n" + - "line names where the data lives: LOCAL and the database's path, API and its host,\n" + - "or the sync state of a synced database. In API mode the board reads the API again\n" + - "every [sync] interval, and R reads it now.\n" + + "line names where the data lives: LOCAL and the database's path, or API and its\n" + + "host. In API mode the board reads the API again on a timer, and R reads it now.\n" + "\n" + "--once prints one frame at COLUMNS by LINES, else at the terminal's size, else at\n" + "120 by 40, and --json what that frame is drawn from, so an agent reads what a\n" + @@ -74,10 +73,7 @@ func newTUICommand() *cobra.Command { } defer func() { _ = source.Close() }() where := tui.Where{DatabasePath: source.cfg.Local.DBPath, APIHost: source.apiHost()} - if !printing { - source.startSync() - } - app := tui.NewApp(source.boardBackend(), source.engine, where, source.cfg.Sync.Interval) + app := tui.NewApp(source.boardBackend(), where) if !printing { _, err := tea.NewProgram(app).Run() return err diff --git a/config.example.toml b/config.example.toml index d4f43b3..05ce4ce 100644 --- a/config.example.toml +++ b/config.example.toml @@ -1,7 +1,7 @@ # projects configuration # Place at ~/.config/projects/config.toml -# The keys under [local] and [sync], and repos_registry, can be overridden with -# environment variables. Bare keys must stay above the first [section]. +# db_path and repos_registry can be overridden with environment variables. Bare +# keys must stay above the first [section]. # Repo registry, whose names a project may not take in database mode. # Set this when the registry is maintained outside the tool's data directory and @@ -22,26 +22,6 @@ # Set, no login runs. For an API that needs no real token, print a stand-in. # token_command = ["printf", '{"access_token":"dev","token_type":"Bearer"}'] -[sync] -# Keep the database a copy of an API, for the board. Refused beside api. -# With it on, a command that writes refuses: make the change in `projects tui`. -# Env: PROJECTS_SYNC -enabled = false -# The API server's host, required when enabled. The tool adds /api/v1 itself. -# Env: PROJECTS_SYNC_URL -# api_url = "https://example.com" -# Command printing a bearer token for api_url, split on spaces and run without a -# shell. Unset sends no token. A URL from PROJECTS_SYNC_URL takes its command from -# PROJECTS_SYNC_TOKEN_COMMAND alone, never from this file. -# Env: PROJECTS_SYNC_TOKEN_COMMAND -# token_command = " auth token" -# Longest the tool goes without reconciling: the TUI's background pull period. -# Through api, how often the board reads the API again. -# A duration such as "30s" or "5m". Anything below 15s is raised to 15s. -# Env: PROJECTS_SYNC_INTERVAL -# Default: 2m -# interval = "2m" - [local] # Path to SQLite database # Env: PROJECTS_DB diff --git a/config/config.go b/config/config.go index 43b12ce..fc6eebb 100644 --- a/config/config.go +++ b/config/config.go @@ -5,7 +5,6 @@ import ( "os" "path/filepath" "strings" - "time" "github.com/go-viper/mapstructure/v2" "github.com/spf13/viper" @@ -14,7 +13,10 @@ import ( // Config holds the application configuration loaded from TOML and env vars. type Config struct { Local LocalConfig `mapstructure:"local"` - Sync SyncConfig `mapstructure:"sync"` + + // RetiredSync holds a [sync] section, which the command layer warns is ignored. + // It is declared because ErrorUnused refuses a key the struct has no home for. + RetiredSync map[string]any `mapstructure:"sync"` // ReposRegistry names a registry maintained outside the tool's data directory, // which is how a machine sharing one between tools declares it. Empty, or no @@ -34,22 +36,6 @@ type Config struct { TokenCommand []string `mapstructure:"token_command"` } -// SyncConfig holds settings for background sync with the remote API. -type SyncConfig struct { - Enabled bool `mapstructure:"enabled"` - APIURL string `mapstructure:"api_url"` - - // TokenCommand prints a bearer token for APIURL, the way a CLI's - // `auth token` does. It is split on spaces and run without a shell. Empty - // sends no Authorization header, which a dev API that sets its own identity - // wants. - TokenCommand string `mapstructure:"token_command"` - - // Interval is the longest the tool will go without reconciling: the TUI's - // background pull period. - Interval time.Duration `mapstructure:"interval"` -} - // LocalConfig holds settings for the local embedded mode. type LocalConfig struct { DBPath string `mapstructure:"db_path"` @@ -60,10 +46,7 @@ type LocalConfig struct { func Load() (*Config, error) { v := newViper() - // Defaults. Every pull is a full reconcile, so the interval trades API load - // against staleness. Two minutes keeps an unattended TUI current. v.SetDefault("local.db_path", defaultDBPath()) - v.SetDefault("sync.interval", 2*time.Minute) if err := v.ReadInConfig(); err != nil { if _, ok := err.(viper.ConfigFileNotFoundError); !ok { @@ -74,10 +57,6 @@ func Load() (*Config, error) { // Env var overrides v.SetEnvPrefix("PROJECTS") _ = v.BindEnv("local.db_path", "PROJECTS_DB") - _ = v.BindEnv("sync.enabled", SyncVariable) - _ = v.BindEnv("sync.api_url", "PROJECTS_SYNC_URL") - _ = v.BindEnv("sync.token_command", "PROJECTS_SYNC_TOKEN_COMMAND") - _ = v.BindEnv("sync.interval", "PROJECTS_SYNC_INTERVAL") // ErrorUnused, so a key the tool does not read is refused rather than dropped. // viper decodes the merged settings map, and every default set above maps to a @@ -88,66 +67,30 @@ func Load() (*Config, error) { // naming its registry under a key the tool does not read would fall back to the // data directory, find no registry there, and lift the ban with nothing // reporting why. - if err := refuseTokenCommandInTheOtherShape(v); err != nil { + if err := refuseTokenCommandAsOneString(v); err != nil { return nil, err } var cfg Config - if err := v.Unmarshal(&cfg, viper.DecodeHook(mapstructure.StringToTimeDurationHookFunc()), func(dc *mapstructure.DecoderConfig) { + if err := v.Unmarshal(&cfg, func(dc *mapstructure.DecoderConfig) { dc.ErrorUnused = true }); err != nil { return nil, fmt.Errorf("parsing config: %w", err) } - - if cfg.Sync.Enabled && cfg.Sync.APIURL == "" { - return nil, fmt.Errorf("sync.api_url is required when sync is enabled (set in config or PROJECTS_SYNC_URL)") - } - - // A token command answers for the URL beside it. A URL from the environment - // names an API the config file does not describe, so the file's command - // never pairs with it: a dev .envrc overriding the URL would otherwise send a - // production token to the dev API. PROJECTS_SYNC_TOKEN_COMMAND is then the only - // source, and unset means no token. - if os.Getenv("PROJECTS_SYNC_URL") != "" { - cfg.Sync.TokenCommand = os.Getenv("PROJECTS_SYNC_TOKEN_COMMAND") - } - if cfg.Sync.TokenCommand != "" && strings.TrimSpace(cfg.Sync.TokenCommand) == "" { - return nil, fmt.Errorf("sync.token_command names no command") - } - - // time.NewTicker panics on a non-positive duration, and a config typo must not - // take the TUI down. Disabling automatic sync is what sync.enabled is for. - if cfg.Sync.Interval < minSyncInterval { - cfg.Sync.Interval = minSyncInterval - } - return &cfg, nil } -// SyncVariable turns sync on or off from the environment, over the file's -// sync.enabled. -const SyncVariable = "PROJECTS_SYNC" - -// refuseTokenCommandInTheOtherShape refuses a token_command written the way the -// other key takes it. Beside api it is an argument list printing a JSON token -// response; under [sync] it is one string printing a bare token. A command moved -// from one to the other would otherwise fail on a type error naming neither. -func refuseTokenCommandInTheOtherShape(v *viper.Viper) error { +// refuseTokenCommandAsOneString refuses a token_command written as one string. It +// is an argument list printing a JSON token response, and a string would otherwise +// fail on a type error naming neither. +func refuseTokenCommandAsOneString(v *viper.Viper) error { if command, isString := v.Get("token_command").(string); isString { return fmt.Errorf("token_command beside api is a list of arguments, run without a shell, that prints a JSON "+ - "token response: token_command = [\"%s\"]; one string is [sync]'s form", + "token response: token_command = [\"%s\"]", strings.Join(strings.Fields(command), `", "`)) } - if _, isList := v.Get("sync.token_command").([]any); isList { - return fmt.Errorf("[sync] token_command is one string, split on spaces, that prints a bare token, " + - "such as token_command = \" auth token\"; a list is the form beside api") - } return nil } -// minSyncInterval floors the reconcile period. A full pull is 2+2N requests, so -// anything shorter is a self-inflicted denial of service on the API. -const minSyncInterval = 15 * time.Second - // newViper points a fresh viper at the tool's config file. Shared by both readers // here so they cannot drift on where that file is. // @@ -168,10 +111,9 @@ func newViper() *viper.Viper { // should not have to hold a file saying so. // // A read of its own rather than a field off Load's result, because Load -// validates. Sync enabled without an api_url is an error there and has nothing -// to do with where the registry lives, so resolving through it would turn that -// unrelated failure into an empty registry — which bans nothing and reports -// nothing. +// refuses a key it does not read. That has nothing to do with where the registry +// lives, so resolving through it would turn an unrelated failure into an empty +// registry, which bans nothing and reports nothing. func ConfiguredReposRegistry() string { v := newViper() if err := v.ReadInConfig(); err != nil { diff --git a/config/config_test.go b/config/config_test.go index 03f7ddf..c3f94e8 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -4,33 +4,11 @@ import ( "os" "path/filepath" "testing" - "time" ) -func TestLoad_Defaults(t *testing.T) { - // No config file, no env vars — should use defaults - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) - t.Setenv("PROJECTS_SYNC", "") - t.Setenv("PROJECTS_SYNC_URL", "") - t.Setenv("PROJECTS_DB", "") - - cfg, err := Load() - if err != nil { - t.Fatalf("Load: %v", err) - } - - if cfg.Sync.Enabled { - t.Error("expected sync disabled by default") - } - if cfg.Local.DBPath == "" { - t.Error("expected non-empty default db_path") - } -} - func TestLoad_EnvOverrides(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) t.Setenv("PROJECTS_DB", "/tmp/test.db") - t.Setenv("PROJECTS_SYNC", "true") - t.Setenv("PROJECTS_SYNC_URL", "https://example.com") cfg, err := Load() if err != nil { @@ -40,23 +18,6 @@ func TestLoad_EnvOverrides(t *testing.T) { if cfg.Local.DBPath != "/tmp/test.db" { t.Errorf("expected db_path '/tmp/test.db', got %q", cfg.Local.DBPath) } - if !cfg.Sync.Enabled { - t.Error("expected sync enabled via env") - } - if cfg.Sync.APIURL != "https://example.com" { - t.Errorf("expected sync url 'https://example.com', got %q", cfg.Sync.APIURL) - } -} - -func TestLoad_SyncEnabledWithoutURL(t *testing.T) { - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) - t.Setenv("PROJECTS_SYNC", "true") - t.Setenv("PROJECTS_SYNC_URL", "") - - _, err := Load() - if err == nil { - t.Fatal("expected error when sync enabled without api_url") - } } func TestDefaultDBPath_XDGOverride(t *testing.T) { @@ -69,37 +30,10 @@ func TestDefaultDBPath_XDGOverride(t *testing.T) { } } -// The interval is the TUI's background pull period, so a missing default would -// mean no automatic sync at all. -func TestLoad_SyncIntervalDefault(t *testing.T) { - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) - t.Setenv("PROJECTS_SYNC_INTERVAL", "") - - cfg, err := Load() - if err != nil { - t.Fatalf("Load: %v", err) - } - if cfg.Sync.Interval != 2*time.Minute { - t.Errorf("expected a 2m default interval, got %v", cfg.Sync.Interval) - } -} - -func TestLoad_SyncIntervalFromEnv(t *testing.T) { - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) - t.Setenv("PROJECTS_SYNC_INTERVAL", "45s") - - cfg, err := Load() - if err != nil { - t.Fatalf("Load: %v", err) - } - if cfg.Sync.Interval != 45*time.Second { - t.Errorf("expected 45s from the environment, got %v", cfg.Sync.Interval) - } -} - // repos_registry is a bare top-level key, which TOML requires above the first // [section] header — declared under one it would belong to that section and -// unmarshal into nothing. +// unmarshal into nothing. The [sync] section below it has to load, so a machine +// whose config still holds one keeps running. func TestReposRegistryIsReadFromTheConfigFile(t *testing.T) { const declared = "/declared/repos.json" writeConfig(t, "repos_registry = \""+declared+"\"\n\n[sync]\nenabled = false\n") @@ -115,6 +49,9 @@ func TestReposRegistryIsReadFromTheConfigFile(t *testing.T) { if cfg.ReposRegistry != declared { t.Errorf("cfg.ReposRegistry = %q, want %q", cfg.ReposRegistry, declared) } + if cfg.RetiredSync == nil { + t.Error("the [sync] section was dropped, so nothing warns that it is ignored") + } } // A machine keeping its registry where the tool expects it holds no config at @@ -144,73 +81,3 @@ func writeConfig(t *testing.T, body string) { } t.Setenv("XDG_CONFIG_HOME", dir) } - -const syncedConfig = "[sync]\nenabled = true\napi_url = \"https://api.example\"\ntoken_command = \"print-token --for api\"\n" - -func TestTheTokenCommandIsReadFromTheConfigFile(t *testing.T) { - writeConfig(t, syncedConfig) - t.Setenv("PROJECTS_SYNC_URL", "") - t.Setenv("PROJECTS_SYNC_TOKEN_COMMAND", "") - - cfg, err := Load() - if err != nil { - t.Fatalf("Load: %v", err) - } - if cfg.Sync.TokenCommand != "print-token --for api" { - t.Errorf("token command %q, want the file's", cfg.Sync.TokenCommand) - } -} - -// A dev .envrc points the URL at another API. The file's command was written for -// the file's URL, and must not hand its token to that one. -func TestAURLFromTheEnvironmentTakesNoTokenCommandFromTheFile(t *testing.T) { - writeConfig(t, syncedConfig) - t.Setenv("PROJECTS_SYNC_URL", "https://dev.example") - t.Setenv("PROJECTS_SYNC_TOKEN_COMMAND", "") - - cfg, err := Load() - if err != nil { - t.Fatalf("Load: %v", err) - } - if cfg.Sync.TokenCommand != "" { - t.Errorf("token command %q paired with the environment's URL, want none", cfg.Sync.TokenCommand) - } - - t.Setenv("PROJECTS_SYNC_TOKEN_COMMAND", "print-token --for dev") - cfg, err = Load() - if err != nil { - t.Fatalf("Load: %v", err) - } - if cfg.Sync.TokenCommand != "print-token --for dev" { - t.Errorf("token command %q, want the environment's", cfg.Sync.TokenCommand) - } -} - -// Split on spaces, a blank command is nothing to run, and silently sending no -// token would read as the API refusing a good one. -func TestABlankTokenCommandIsRefused(t *testing.T) { - writeConfig(t, "[sync]\nenabled = true\napi_url = \"https://api.example\"\ntoken_command = \" \"\n") - t.Setenv("PROJECTS_SYNC_URL", "") - t.Setenv("PROJECTS_SYNC_TOKEN_COMMAND", "") - - if _, err := Load(); err == nil { - t.Fatal("a blank token_command loaded") - } -} - -// time.NewTicker panics on a non-positive duration, so a typo here would take -// the TUI down on launch rather than degrading to a slower sync. -func TestLoad_SyncIntervalIsFloored(t *testing.T) { - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) - - for _, given := range []string{"0s", "-5m", "1s"} { - t.Setenv("PROJECTS_SYNC_INTERVAL", given) - cfg, err := Load() - if err != nil { - t.Fatalf("Load(%s): %v", given, err) - } - if cfg.Sync.Interval < minSyncInterval { - t.Errorf("interval %q left below the floor: %v", given, cfg.Sync.Interval) - } - } -} diff --git a/config/former_name.go b/config/former_name.go index a880fc1..0becc35 100644 --- a/config/former_name.go +++ b/config/former_name.go @@ -14,36 +14,28 @@ import ( // MIGRATION: the tool was called todoui. A machine that upgrades still holds data, a config // and variables under that name, and to this tool they read as if never written. Delete this -// file, its calls in main.go, former_name_test.go, and the passages on it in README and -// CLAUDE.md once no machine holds a todoui data or config directory and no checkout's .envrc +// file, its calls in cli/data_source.go, former_name_test.go, and the passages on it in README +// and CLAUDE.md once no machine holds a todoui data or config directory and no checkout's .envrc // exports a TODOUI_* variable. const formerName = "todoui" -var retiredVariables = []struct{ retired, replacement string }{ - {"TODOUI_DB", "PROJECTS_DB"}, - {"TODOUI_SYNC", "PROJECTS_SYNC"}, - {"TODOUI_SYNC_URL", "PROJECTS_SYNC_URL"}, - {"TODOUI_SYNC_TOKEN_COMMAND", "PROJECTS_SYNC_TOKEN_COMMAND"}, - {"TODOUI_SYNC_INTERVAL", "PROJECTS_SYNC_INTERVAL"}, - {"TODOUI_REPOS_REGISTRY", "PROJECTS_REPOS_REGISTRY"}, - {"TODOUI_NO_AUTO_UPDATE", "PROJECTS_NO_AUTO_UPDATE"}, -} - -// RetiredVariableRefusal names the first retired variable set to a value, or "" where none is. // Ignoring TODOUI_DB would open the default database where it chose another, and the others // are refused so a half-renamed environment never runs. -func RetiredVariableRefusal() string { - for _, variable := range retiredVariables { - if os.Getenv(variable.retired) != "" { - return fmt.Sprintf("$%s is set, and projects reads $%s. Move the value there, or unset it.", - variable.retired, variable.replacement) - } +func init() { + for _, renamed := range []struct{ retired, replacement string }{ + {"TODOUI_DB", "PROJECTS_DB"}, + {"TODOUI_REPOS_REGISTRY", "PROJECTS_REPOS_REGISTRY"}, + {"TODOUI_NO_AUTO_UPDATE", "PROJECTS_NO_AUTO_UPDATE"}, + } { + retiredVariables = append(retiredVariables, retiredVariable{ + name: renamed.retired, + remedy: fmt.Sprintf("and projects reads $%s. Move the value there, or unset it.", renamed.replacement), + }) } - return "" } // RefuseConfigUnderFormerName stops a run while the config sits only at the former path. -// Reading none would turn sync off, and the next pull deletes what was written meanwhile. +// Reading none would drop its api and send every command to a database on this machine. func RefuseConfigUnderFormerName() error { former := filepath.Join(userConfigDir(), formerName, "config.toml") current := filepath.Join(userConfigDir(), toolName, "config.toml") diff --git a/config/retired_sync.go b/config/retired_sync.go new file mode 100644 index 0000000..5f2a612 --- /dev/null +++ b/config/retired_sync.go @@ -0,0 +1,28 @@ +package config + +// MIGRATION: projects once kept a database as a synced copy of an API, set under [sync] and by +// the variables below. Delete this file, Config.RetiredSync, warnOfRetiredSyncSection in +// cli/data_source.go, dropSyncTables in store/sqlite/open.go, and the passages on them in README +// and CLAUDE.md once no machine's config holds [sync], no .envrc exports one of these, and no +// machine's database still holds pending_sync. + +// PROJECTS_SYNC_URL chose an API, and ignoring it would send the run wherever the config file +// points, which on a machine whose config names the production API is production. +func init() { + for _, name := range []string{ + "PROJECTS_SYNC", + "PROJECTS_SYNC_URL", + "PROJECTS_SYNC_TOKEN_COMMAND", + "PROJECTS_SYNC_INTERVAL", + "TODOUI_SYNC", + "TODOUI_SYNC_URL", + "TODOUI_SYNC_TOKEN_COMMAND", + "TODOUI_SYNC_INTERVAL", + } { + retiredVariables = append(retiredVariables, retiredVariable{ + name: name, + remedy: "and projects keeps no synced copy of an API. Unset it. api in the config file, " + + "written by `projects auth login `, sends every command to an API.", + }) + } +} diff --git a/config/retired_variables.go b/config/retired_variables.go new file mode 100644 index 0000000..022bcd4 --- /dev/null +++ b/config/retired_variables.go @@ -0,0 +1,29 @@ +package config + +import ( + "fmt" + "os" +) + +// retiredVariable is an environment variable projects refuses rather than reads. +// Each chose where a run read or wrote, so ignoring it would send the run +// somewhere else. +type retiredVariable struct { + name string + // remedy follows "$ is set, " in the refusal. + remedy string +} + +// retiredVariables holds every retirement's rows. Each retirement adds its own +// from the file that holds it, so deleting that file takes its rows with it. +var retiredVariables []retiredVariable + +// RetiredVariableRefusal names the first retired variable set to a value, or "" where none is. +func RetiredVariableRefusal() string { + for _, variable := range retiredVariables { + if os.Getenv(variable.name) != "" { + return fmt.Sprintf("$%s is set, %s", variable.name, variable.remedy) + } + } + return "" +} diff --git a/former_name_test.go b/former_name_test.go index 817e955..5616468 100644 --- a/former_name_test.go +++ b/former_name_test.go @@ -17,8 +17,8 @@ import ( ) // Each case runs the built binary under a scratch home, because the exit code and the -// order against opening the database belong to main. `sync` reaches the move and then -// stops at sync being off, so no case needs a terminal. +// order against opening the database belong to main. `list` reaches the move and opens +// the database, and needs no terminal. func TestFormerNameOnFirstRun(t *testing.T) { binary := tuidrive.Build(t, ".") @@ -28,8 +28,8 @@ func TestFormerNameOnFirstRun(t *testing.T) { seedFormerDatabaseWithUnflushedWAL(t, former) writeFile(t, filepath.Join(former, "dev.db"), "dev") - if output, code := home.run(t, binary); code != 1 { - t.Fatalf("exit %d, output %q, want it to reach the disabled sync", code, output) + if output, code := home.run(t, binary); code != 0 { + t.Fatalf("exit %d, output %q, want the list to answer", code, output) } if _, err := os.Lstat(former); !errors.Is(err, os.ErrNotExist) { @@ -81,11 +81,11 @@ func TestFormerNameOnFirstRun(t *testing.T) { } }) - t.Run("a config left at the former path refuses before sync silently turns off", func(t *testing.T) { + t.Run("a config left at the former path refuses before it is silently ignored", func(t *testing.T) { home := newScratchHome(t) formerConfig := filepath.Join(home.root, "config", "todoui", "config.toml") formerDB := filepath.Join(home.dataDir("todoui"), "todoui.db") - writeFile(t, formerConfig, "[sync]\nenabled = true\n") + writeFile(t, formerConfig, "api = \"https://projects.example\"\n") writeFile(t, formerDB, "former") output, code := home.run(t, binary) @@ -158,7 +158,7 @@ func (h scratchHome) dataDir(name string) string { func (h scratchHome) run(t *testing.T, binary string, env ...string) (string, int) { t.Helper() - cmd := exec.Command(binary, "sync") + cmd := exec.Command(binary, "list") cmd.Dir = h.root cmd.Env = append([]string{ "HOME=" + h.root, diff --git a/model/project.go b/model/project.go index 6c845ee..a3fa9e7 100644 --- a/model/project.go +++ b/model/project.go @@ -15,7 +15,7 @@ import ( const ( StatusActive = "active" // StatusCompleted and not "done". The API stores this value behind a foreign - // key, so a push carrying any other word is rejected. It also matches the + // key, so a write carrying any other word is rejected. It also matches the // `completed` boolean an item carries, so a project and its own items spell // one concept one way. StatusCompleted = "completed" diff --git a/model/project_item.go b/model/project_item.go index 76a6965..ac63097 100644 --- a/model/project_item.go +++ b/model/project_item.go @@ -16,9 +16,10 @@ const ( // ProjectItem is the base representation of an item in the system. type ProjectItem struct { ID string `json:"id"` - // Number is the handle — short, server-assigned, and the only one of the two - // identifiers meant to be read or typed. Nil for an item created while sync - // was unreachable, which has no number until its create is pushed. + // Number is the handle — short, assigned as the item is created, and the only + // one of the two identifiers meant to be read or typed. Nil on an item from + // before numbers existed, or one a database once kept as a copy of an API + // created before the API numbered it. Number *int `json:"number,omitempty"` Title string `json:"title"` Notes *string `json:"notes,omitempty"` @@ -47,11 +48,8 @@ func (i ProjectItem) Status() string { // ProjectItemDetail is a ProjectItem with everything filed against it. // -// DependencyIDs and Tasks decode to nil when a server omits the field and to an -// empty slice when it sends one, which is the only way a pull can tell a server -// predating the embed from an item that genuinely has neither. Keep them as plain -// slices and keep the API sending `[]`; anything that collapses nil and empty -// makes an old server look like an instruction to delete every task. +// DependencyIDs and Tasks hold an empty slice rather than nil where an item has +// none, so the API answers `[]` for them. type ProjectItemDetail struct { ProjectItem Projects []Project `json:"projects"` diff --git a/model/undo.go b/model/undo.go index f4dfc88..bc5b22c 100644 --- a/model/undo.go +++ b/model/undo.go @@ -33,56 +33,10 @@ type UndoEntry struct { CreatedAt time.Time `json:"created_at"` } -// UndoResult describes what an undo actually did. A sync layer needs more than -// a display string: the pending queue still holds operations describing the -// state the undo just reversed, and reconciling them requires knowing which -// entity changed and what it looks like now. +// UndoResult describes what an undo reversed. type UndoResult struct { Description string Action string EntityType string EntityID string - - // Restored is the local row after the undo, or nil when the undo removed it. - // Exactly one of Restored / RestoredProject can be set, matching EntityType. - Restored *ProjectItem - RestoredProject *Project - - // Rows that cascaded off a delete and came back with it. Sync has to push - // these, not just the row: a pull replaces memberships and dependencies - // wholesale from server state and drops tasks the server does not know - // about, so anything restored only locally is erased on the next pull. - RestoredProjectIDs []string // projects an item was restored into - RestoredItemIDs []string // items a restored project held - RestoredTasks []ProjectItemTask - RestoredDependencies []ItemDependency - - // Detail is set for entity types other than item and project. - Detail *UndoDetail -} - -// ItemDependency is one edge of the blocking graph: ItemID is blocked until -// DependsOnID is complete. -type ItemDependency struct { - ItemID string `json:"item_id"` - DependsOnID string `json:"depends_on_id"` -} - -// UndoDetail describes a reversal of anything other than an item or project -// row — a task, a membership, a dependency, or a reorder. Only the fields -// relevant to the result's EntityType are set. The sync layer needs it to -// push the same reversal rather than guessing from the entity ID alone. -type UndoDetail struct { - // Removed reports whether the undo took the row away rather than putting - // it back. It is what separates reversing an add from reversing a remove, - // which are otherwise indistinguishable at the sync layer. - Removed bool - - ItemID string // owning item: task, membership, dependency, item reorder - ProjectID string // membership and item reorder - DependsOnID string // dependency - Position int // reorder - - // Task is the row as it stands after the undo, nil when the undo removed it. - Task *ProjectItemTask } diff --git a/real_binary_flows_test.go b/real_binary_flows_test.go index 19db595..43adf91 100644 --- a/real_binary_flows_test.go +++ b/real_binary_flows_test.go @@ -108,7 +108,7 @@ func writeAPIConfig(t *testing.T, dir, api string) { } } -// seedFixtureDatabase writes through the backend the TUI writes through with sync off, +// seedFixtureDatabase writes through the backend the TUI writes through in database mode, // so the fixture holds what the tool's own rules allow. The export note runs long, // because a one-line note hides layout bugs. func seedFixtureDatabase(t *testing.T, path string) { diff --git a/service/number_test.go b/service/number_test.go index fd19f60..68ff988 100644 --- a/service/number_test.go +++ b/service/number_test.go @@ -19,10 +19,9 @@ func newNumberTestDB(t *testing.T) *sql.DB { return database } -// The work machine is never allowed to reach the API, so nothing upstream will -// ever number its items. Its database is the authority instead. -func TestCreateItem_AssignsNumbersWhenThisDatabaseIsTheAuthority(t *testing.T) { - b := service.New(sqlite.New(newNumberTestDB(t)), service.AssigningItemNumbers()) +// An item's number is the handle a person types, so a fresh database starts it at 1. +func TestCreateItem_NumbersAFreshDatabaseFromOne(t *testing.T) { + b := service.New(sqlite.New(newNumberTestDB(t))) project, err := b.CreateProject(model.CreateProject{Name: "Work"}) if err != nil { t.Fatalf("CreateProject: %v", err) @@ -35,7 +34,7 @@ func TestCreateItem_AssignsNumbersWhenThisDatabaseIsTheAuthority(t *testing.T) { t.Fatalf("CreateItem(%s): %v", title, err) } if item.Number == nil { - t.Fatalf("CreateItem(%s) left the number unset — nothing else will assign it here", title) + t.Fatalf("CreateItem(%s) left the number unset", title) } numbers = append(numbers, *item.Number) } @@ -44,22 +43,3 @@ func TestCreateItem_AssignsNumbersWhenThisDatabaseIsTheAuthority(t *testing.T) { t.Errorf("numbers = %v, want %v — the first item in a fresh database is 1", numbers, want) } } - -// With sync on the server owns the numbering. Guessing one locally would hand -// the user a handle that changes the moment the push comes back. -func TestCreateItem_LeavesTheNumberToTheServerWhenSyncIsOn(t *testing.T) { - b := service.New(sqlite.New(newNumberTestDB(t))) - project, err := b.CreateProject(model.CreateProject{Name: "Synced"}) - if err != nil { - t.Fatalf("CreateProject: %v", err) - } - - item, err := b.CreateItem(model.CreateProjectItem{Title: "Waits for a number", ProjectIDs: []string{project.ID}}) - if err != nil { - t.Fatalf("CreateItem: %v", err) - } - - if item.Number != nil { - t.Errorf("Number = %d, want nil — the server assigns it and the two would disagree", *item.Number) - } -} diff --git a/service/references.go b/service/references.go index f87e0ec..adeb673 100644 --- a/service/references.go +++ b/service/references.go @@ -159,8 +159,8 @@ func parseItemNumber(ref string) (int, bool) { return number, err == nil } -// itemHandle is what a refusal calls an item. An item created while sync was -// unreachable has no number yet, so it goes by its id. +// itemHandle is what a refusal calls an item: its number, or its id where it +// has none. func itemHandle(item model.ProjectItem) string { if item.Number != nil { return strconv.Itoa(*item.Number) diff --git a/service/service.go b/service/service.go index 4521393..a9822ac 100644 --- a/service/service.go +++ b/service/service.go @@ -23,9 +23,6 @@ const itemListSegment = "/items" // Service holds every rule about projects, items, memberships, dependencies, tasks and undo. type Service struct { store store.Store - // assignsItemNumbers is set when nothing upstream will hand one out, which - // is exactly the sync-disabled case. See AssigningItemNumbers. - assignsItemNumbers bool // repoRegistryPath enables the repo-named-project ban. Empty means no // registry and no ban. See RefusingRepoNames. repoRegistryPath string @@ -36,16 +33,6 @@ type Service struct { // Option configures a Service at construction. type Option func(*Service) -// AssigningItemNumbers makes this database the authority for item numbers. -// -// Pass it when sync is off, because that database is a disjoint universe no -// server will ever number. With sync on the server assigns instead, because a -// number guessed locally and a number assigned upstream would disagree and the -// handle would change under a user who had already written it down. -func AssigningItemNumbers() Option { - return func(s *Service) { s.assignsItemNumbers = true } -} - // RefusingRepoNames bans naming a project after a repo, reading the registry at // path on the create and rename paths only. // @@ -535,19 +522,15 @@ func (s *Service) CreateItem(input model.CreateProjectItem) (*model.ProjectItemD } } - var number *int - if s.assignsItemNumbers { - next, err := tx.NextItemNumber(ctx) - if err != nil { - return fmt.Errorf("allocating item number: %w", err) - } - number = &next + number, err := tx.NextItemNumber(ctx) + if err != nil { + return fmt.Errorf("allocating item number: %w", err) } id := idOrNew(input.ID) item, err := tx.CreateItem(ctx, model.ProjectItem{ ID: id, - Number: number, + Number: &number, Title: input.Title, Notes: input.Notes, Repo: emptyRepoUnlinks(input.Repo), @@ -997,8 +980,7 @@ func (s *Service) CreateTask(itemRef string, input model.CreateProjectItemTask) return fmt.Errorf("creating task: %w", err) } created = t - // The row, not nil: undoing a task create still has to know which item - // owned it so the reversal can be pushed. + // The row, not nil: undoTask refuses a task entry with no previous state. return logUndo(ctx, tx, model.UndoActionCreate, model.UndoEntityTask, id, taskRecordOf(t)) }) if err != nil { diff --git a/service/service_test.go b/service/service_test.go index 8096cd4..0dffcac 100644 --- a/service/service_test.go +++ b/service/service_test.go @@ -30,7 +30,7 @@ func runsOnPostgres() bool { func newTestBackend(t *testing.T) *Service { t.Helper() if runsOnPostgres() { - return New(postgres.New(postgrestest.Pool(t)), AssigningItemNumbers()) + return New(postgres.New(postgrestest.Pool(t))) } database, err := sqlite.Open(":memory:") if err != nil { @@ -404,10 +404,6 @@ func TestUndoCreateItem(t *testing.T) { if result.EntityID != item.ID { t.Errorf("expected undo result for %s, got %s", item.ID, result.EntityID) } - if result.Restored != nil { - t.Error("undoing a create removes the row, so nothing should be restored") - } - _, err = b.GetItem(item.ID) if !errors.Is(err, model.ErrNotFound) { t.Errorf("expected ErrNotFound after undo, got %v", err) @@ -446,15 +442,15 @@ func TestUndoRestoresDeletedProject(t *testing.T) { t.Fatalf("deleting project: %v", err) } - result, err := b.Undo() - if err != nil { + if _, err := b.Undo(); err != nil { t.Fatalf("undoing: %v", err) } - if result.RestoredProject == nil || result.RestoredProject.Name != "doomed" { - t.Fatalf("expected the deleted project restored, got %+v", result.RestoredProject) + restored, err := b.GetProject(p.ID) + if err != nil { + t.Fatalf("expected project back after undo: %v", err) } - if _, err := b.GetProject(p.ID); err != nil { - t.Errorf("expected project back after undo: %v", err) + if restored.Name != "doomed" { + t.Errorf("restored project is named %q, want doomed", restored.Name) } } @@ -879,8 +875,7 @@ func TestUndoPreservesDeletedItemTimestamps(t *testing.T) { } } -// An edit in the TUI reaches this while the sync engine's pull writes, and two -// running copies reach it on one file. Each rule reads and then writes, and SQLite +// Two running copies reach this on one file, such as the board and a command. Each rule reads and then writes, and SQLite // refuses that upgrade once another writer has committed unless the transaction // began with the write lock. func TestAnEditSurvivesASecondWriter(t *testing.T) { @@ -893,7 +888,7 @@ func TestAnEditSurvivesASecondWriter(t *testing.T) { t.Fatalf("connecting: %v", err) } t.Cleanup(pool.Close) - return New(postgres.New(pool), AssigningItemNumbers()) + return New(postgres.New(pool)) } } else { path := filepath.Join(t.TempDir(), "projects.db") @@ -963,12 +958,12 @@ func TestUndoAcceptsPreSnapshotDeleteEntries(t *testing.T) { `"Completed":0,"Archived":0,"CreatedAt":"2026-01-02 03:04:05","UpdatedAt":"2026-01-02 03:04:05"}`, item.ID)) - result, err := b.Undo() - if err != nil { + if _, err := b.Undo(); err != nil { t.Fatalf("undoing: %v", err) } - if result.Restored == nil || result.Restored.Title != "legacy" { - t.Fatalf("expected the item restored from a pre-snapshot entry, got %+v", result.Restored) + restored, err := b.GetItem(item.ID) + if err != nil || restored.Title != "legacy" { + t.Fatalf("expected the item restored from a pre-snapshot entry, got %+v, %v", restored, err) } } diff --git a/service/undo.go b/service/undo.go index ccfdc2d..3b25a33 100644 --- a/service/undo.go +++ b/service/undo.go @@ -36,19 +36,19 @@ func (s *Service) Undo() (*model.UndoResult, error) { var undoErr error switch entry.EntityType { case model.UndoEntityProject: - undoErr = undoProject(ctx, tx, entry, result) + undoErr = undoProject(ctx, tx, entry) case model.UndoEntityItem: - undoErr = undoItem(ctx, tx, entry, result) + undoErr = undoItem(ctx, tx, entry) case model.UndoEntityTask: - undoErr = undoTask(ctx, tx, entry, result) + undoErr = undoTask(ctx, tx, entry) case model.UndoEntityMembership: - undoErr = undoMembership(ctx, tx, entry, result) + undoErr = undoMembership(ctx, tx, entry) case model.UndoEntityDependency: - undoErr = undoDependency(ctx, tx, entry, result) + undoErr = undoDependency(ctx, tx, entry) case model.UndoEntityItemPosition: - undoErr = undoItemPosition(ctx, tx, entry, result) + undoErr = undoItemPosition(ctx, tx, entry) case model.UndoEntityProjectPosition: - undoErr = undoProjectPosition(ctx, tx, entry, result) + undoErr = undoProjectPosition(ctx, tx, entry) default: return fmt.Errorf("undoing unknown entity type %q", entry.EntityType) } @@ -68,7 +68,7 @@ func (s *Service) Undo() (*model.UndoResult, error) { return result, nil } -func undoItem(ctx context.Context, tx store.Operations, entry model.UndoEntry, result *model.UndoResult) error { +func undoItem(ctx context.Context, tx store.Operations, entry model.UndoEntry) error { switch entry.Action { case model.UndoActionCreate: if err := tx.DeleteItem(ctx, entry.EntityID); err != nil { @@ -82,11 +82,9 @@ func undoItem(ctx context.Context, tx store.Operations, entry model.UndoEntry, r } item := prev.toModel() item.ID = entry.EntityID - restored, err := tx.UpdateItem(ctx, item) - if err != nil { + if _, err := tx.UpdateItem(ctx, item); err != nil { return fmt.Errorf("restoring previous state: %w", err) } - result.Restored = &restored } case model.UndoActionDelete: if entry.PreviousState != nil { @@ -97,20 +95,6 @@ func undoItem(ctx context.Context, tx store.Operations, entry model.UndoEntry, r if err := restoreItemSnapshot(ctx, tx, snapshot); err != nil { return err } - item := snapshot.Item.toModel() - result.Restored = &item - for _, membership := range snapshot.Memberships { - result.RestoredProjectIDs = append(result.RestoredProjectIDs, membership.ProjectID) - } - for _, dependency := range snapshot.Dependencies { - result.RestoredDependencies = append(result.RestoredDependencies, model.ItemDependency{ - ItemID: dependency.ItemID, - DependsOnID: dependency.DependsOnID, - }) - } - for _, task := range snapshot.Tasks { - result.RestoredTasks = append(result.RestoredTasks, task.toModel()) - } } default: return fmt.Errorf("undoing unknown item action %q", entry.Action) @@ -118,7 +102,7 @@ func undoItem(ctx context.Context, tx store.Operations, entry model.UndoEntry, r return nil } -func undoTask(ctx context.Context, tx store.Operations, entry model.UndoEntry, result *model.UndoResult) error { +func undoTask(ctx context.Context, tx store.Operations, entry model.UndoEntry) error { if entry.PreviousState == nil { return fmt.Errorf("task undo entry %d has no previous state", entry.ID) } @@ -127,28 +111,22 @@ func undoTask(ctx context.Context, tx store.Operations, entry model.UndoEntry, r return fmt.Errorf("unmarshaling previous task state: %w", err) } - detail := &model.UndoDetail{ItemID: prev.ItemID} - result.Detail = detail - if entry.Action == model.UndoActionCreate { if err := tx.DeleteTask(ctx, entry.EntityID); err != nil { return fmt.Errorf("undoing task create: %w", err) } - detail.Removed = true return nil } // Update and delete both reverse to the recorded row. Restore covers both: // the row is still there after an update, and gone after a delete. - task := prev.toModel() - if err := tx.RestoreTask(ctx, task); err != nil { + if err := tx.RestoreTask(ctx, prev.toModel()); err != nil { return fmt.Errorf("restoring task: %w", err) } - detail.Task = &task return nil } -func undoMembership(ctx context.Context, tx store.Operations, entry model.UndoEntry, result *model.UndoResult) error { +func undoMembership(ctx context.Context, tx store.Operations, entry model.UndoEntry) error { if entry.PreviousState == nil { return fmt.Errorf("membership undo entry %d has no previous state", entry.ID) } @@ -157,12 +135,6 @@ func undoMembership(ctx context.Context, tx store.Operations, entry model.UndoEn return fmt.Errorf("unmarshaling previous membership state: %w", err) } - result.Detail = &model.UndoDetail{ - ItemID: prev.ItemID, - ProjectID: prev.ProjectID, - Removed: entry.Action == model.UndoActionCreate, - } - if entry.Action == model.UndoActionCreate { if err := tx.RemoveItemFromProject(ctx, prev.ItemID, prev.ProjectID); err != nil { return fmt.Errorf("undoing membership create: %w", err) @@ -176,7 +148,7 @@ func undoMembership(ctx context.Context, tx store.Operations, entry model.UndoEn return nil } -func undoDependency(ctx context.Context, tx store.Operations, entry model.UndoEntry, result *model.UndoResult) error { +func undoDependency(ctx context.Context, tx store.Operations, entry model.UndoEntry) error { if entry.PreviousState == nil { return fmt.Errorf("dependency undo entry %d has no previous state", entry.ID) } @@ -185,12 +157,6 @@ func undoDependency(ctx context.Context, tx store.Operations, entry model.UndoEn return fmt.Errorf("unmarshaling previous dependency state: %w", err) } - result.Detail = &model.UndoDetail{ - ItemID: prev.ItemID, - DependsOnID: prev.DependsOnID, - Removed: entry.Action == model.UndoActionCreate, - } - if entry.Action == model.UndoActionCreate { if err := tx.RemoveDependency(ctx, model.Dependency(prev)); err != nil { return fmt.Errorf("undoing dependency create: %w", err) @@ -204,7 +170,7 @@ func undoDependency(ctx context.Context, tx store.Operations, entry model.UndoEn return nil } -func undoItemPosition(ctx context.Context, tx store.Operations, entry model.UndoEntry, result *model.UndoResult) error { +func undoItemPosition(ctx context.Context, tx store.Operations, entry model.UndoEntry) error { if entry.PreviousState == nil { return fmt.Errorf("item reorder undo entry %d has no previous state", entry.ID) } @@ -221,16 +187,10 @@ func undoItemPosition(ctx context.Context, tx store.Operations, entry model.Undo return fmt.Errorf("restoring a sibling's position: %w", err) } } - - result.Detail = &model.UndoDetail{ - ItemID: prev.ItemID, - ProjectID: prev.ProjectID, - Position: int(prev.Position), - } return nil } -func undoProjectPosition(ctx context.Context, tx store.Operations, entry model.UndoEntry, result *model.UndoResult) error { +func undoProjectPosition(ctx context.Context, tx store.Operations, entry model.UndoEntry) error { if entry.PreviousState == nil { return fmt.Errorf("project reorder undo entry %d has no previous state", entry.ID) } @@ -242,13 +202,11 @@ func undoProjectPosition(ctx context.Context, tx store.Operations, entry model.U if err := tx.UpdateProjectPosition(ctx, prev.ID, int(prev.Position)); err != nil { return fmt.Errorf("restoring project position: %w", err) } - - result.Detail = &model.UndoDetail{Position: int(prev.Position)} return nil } // undoProject reverses a logged project mutation. -func undoProject(ctx context.Context, tx store.Operations, entry model.UndoEntry, result *model.UndoResult) error { +func undoProject(ctx context.Context, tx store.Operations, entry model.UndoEntry) error { switch entry.Action { case model.UndoActionCreate: if err := tx.DeleteProject(ctx, entry.EntityID); err != nil { @@ -266,11 +224,6 @@ func undoProject(ctx context.Context, tx store.Operations, entry model.UndoEntry if err := tx.RestoreProject(ctx, prev.toModel()); err != nil { return fmt.Errorf("restoring previous project state: %w", err) } - restored, err := tx.GetProject(ctx, entry.EntityID) - if err != nil { - return fmt.Errorf("reading back the restored project: %w", err) - } - result.RestoredProject = &restored } case model.UndoActionDelete: if entry.PreviousState != nil { @@ -281,11 +234,6 @@ func undoProject(ctx context.Context, tx store.Operations, entry model.UndoEntry if err := restoreProjectSnapshot(ctx, tx, snapshot); err != nil { return err } - project := snapshot.Project.toModel() - result.RestoredProject = &project - for _, membership := range snapshot.Memberships { - result.RestoredItemIDs = append(result.RestoredItemIDs, membership.ItemID) - } } default: return fmt.Errorf("undoing unknown project action %q", entry.Action) diff --git a/store/postgres/store.go b/store/postgres/store.go index afd58dd..7dedfe9 100644 --- a/store/postgres/store.go +++ b/store/postgres/store.go @@ -266,14 +266,11 @@ func (o operations) NextItemNumber(ctx context.Context) (int, error) { return int(next), nil } -// errItemNeedsNumber is a service built without AssigningItemNumbers, because this -// schema is its own authority for numbers and the column holds no null. -var errItemNeedsNumber = errors.New("an item on Postgres needs a number from NextItemNumber") +// errItemNeedsNumber is an undo record restoring an item with no number, which +// the column cannot hold. +var errItemNeedsNumber = errors.New("an item on Postgres needs a number") func (o operations) CreateItem(ctx context.Context, item model.ProjectItem) (model.ProjectItem, error) { - if item.Number == nil { - return model.ProjectItem{}, errItemNeedsNumber - } row, err := o.q.CreateItem(ctx, generated.CreateItemParams{ ID: item.ID, ItemNumber: int64(*item.Number), diff --git a/store/postgres/store_test.go b/store/postgres/store_test.go index 57fd63c..a184be4 100644 --- a/store/postgres/store_test.go +++ b/store/postgres/store_test.go @@ -20,7 +20,7 @@ func TestEveryWriteIsAuditedWithItsWriter(t *testing.T) { ctx := store.WithWriter(context.Background(), store.Writer{ Actor: "a-person", RequestID: "a-request", JobRunID: "a-run", Traceparent: "a-trace", Baggage: "a-baggage", }) - svc := service.New(postgres.New(pool), service.AssigningItemNumbers()).WithContext(ctx) + svc := service.New(postgres.New(pool)).WithContext(ctx) project, err := svc.CreateProject(model.CreateProject{Name: "audited"}) if err != nil { diff --git a/store/sqlite/convert.go b/store/sqlite/convert.go index dd7df27..9979282 100644 --- a/store/sqlite/convert.go +++ b/store/sqlite/convert.go @@ -5,7 +5,7 @@ import ( "time" "github.com/datapointchris/projects/model" - "github.com/datapointchris/projects/store/sqlite/generated" + "github.com/datapointchris/projects/store/sqlite/internal/generated" ) func parseTime(s string) time.Time { diff --git a/store/sqlite/generated/db.go b/store/sqlite/internal/generated/db.go similarity index 100% rename from store/sqlite/generated/db.go rename to store/sqlite/internal/generated/db.go diff --git a/store/sqlite/generated/models.go b/store/sqlite/internal/generated/models.go similarity index 77% rename from store/sqlite/generated/models.go rename to store/sqlite/internal/generated/models.go index 98361e1..52817fb 100644 --- a/store/sqlite/generated/models.go +++ b/store/sqlite/internal/generated/models.go @@ -8,17 +8,6 @@ import ( "database/sql" ) -type PendingSync struct { - ID int64 - Operation string - EntityType string - EntityID string - Payload string - CreatedAt string - Attempts int64 - LastError sql.NullString -} - type Project struct { ID string Name string @@ -73,18 +62,6 @@ type ProjectStatus struct { Name string } -type SyncOrigin struct { - ID int64 - ApiUrl string - AdoptedAt string -} - -type SyncState struct { - EntityType string - LastPullAt string - LastPushAt string -} - type UndoLog struct { ID int64 Action string diff --git a/store/sqlite/generated/queries.sql.go b/store/sqlite/internal/generated/queries.sql.go similarity index 73% rename from store/sqlite/generated/queries.sql.go rename to store/sqlite/internal/generated/queries.sql.go index 19d2e3d..ba8f98e 100644 --- a/store/sqlite/generated/queries.sql.go +++ b/store/sqlite/internal/generated/queries.sql.go @@ -40,43 +40,6 @@ func (q *Queries) AddItemToProject(ctx context.Context, arg AddItemToProjectPara return err } -const countItems = `-- name: CountItems :one -SELECT COUNT(*) FROM project_items -` - -func (q *Queries) CountItems(ctx context.Context) (int64, error) { - row := q.db.QueryRowContext(ctx, countItems) - var count int64 - err := row.Scan(&count) - return count, err -} - -const countPendingSync = `-- name: CountPendingSync :one -SELECT COUNT(*) FROM pending_sync -` - -func (q *Queries) CountPendingSync(ctx context.Context) (int64, error) { - row := q.db.QueryRowContext(ctx, countPendingSync) - var count int64 - err := row.Scan(&count) - return count, err -} - -const countProjects = `-- name: CountProjects :one - -SELECT COUNT(*) FROM projects -` - -// Sync: state tracking -// Counted rather than listed: the caller only needs to know whether this -// database has anything to lose. -func (q *Queries) CountProjects(ctx context.Context) (int64, error) { - row := q.db.QueryRowContext(ctx, countProjects) - var count int64 - err := row.Scan(&count) - return count, err -} - const countUndoLogs = `-- name: CountUndoLogs :one SELECT COUNT(*) FROM undo_log ` @@ -202,33 +165,6 @@ func (q *Queries) CreateTask(ctx context.Context, arg CreateTaskParams) (Project return i, err } -const deleteAllDependencies = `-- name: DeleteAllDependencies :exec -DELETE FROM project_item_dependencies -` - -func (q *Queries) DeleteAllDependencies(ctx context.Context) error { - _, err := q.db.ExecContext(ctx, deleteAllDependencies) - return err -} - -const deleteAllMemberships = `-- name: DeleteAllMemberships :exec -DELETE FROM project_item_memberships -` - -func (q *Queries) DeleteAllMemberships(ctx context.Context) error { - _, err := q.db.ExecContext(ctx, deleteAllMemberships) - return err -} - -const deleteAllPendingSync = `-- name: DeleteAllPendingSync :exec -DELETE FROM pending_sync -` - -func (q *Queries) DeleteAllPendingSync(ctx context.Context) error { - _, err := q.db.ExecContext(ctx, deleteAllPendingSync) - return err -} - const deleteItem = `-- name: DeleteItem :exec DELETE FROM project_items WHERE id = ? ` @@ -238,36 +174,6 @@ func (q *Queries) DeleteItem(ctx context.Context, id string) error { return err } -const deletePendingSync = `-- name: DeletePendingSync :exec -DELETE FROM pending_sync WHERE id = ? -` - -func (q *Queries) DeletePendingSync(ctx context.Context, id int64) error { - _, err := q.db.ExecContext(ctx, deletePendingSync, id) - return err -} - -const deletePendingSyncByEntity = `-- name: DeletePendingSyncByEntity :execrows -DELETE FROM pending_sync WHERE entity_id = ? -` - -func (q *Queries) DeletePendingSyncByEntity(ctx context.Context, entityID string) (int64, error) { - result, err := q.db.ExecContext(ctx, deletePendingSyncByEntity, entityID) - if err != nil { - return 0, err - } - return result.RowsAffected() -} - -const deletePendingSyncUpTo = `-- name: DeletePendingSyncUpTo :exec -DELETE FROM pending_sync WHERE id <= ? -` - -func (q *Queries) DeletePendingSyncUpTo(ctx context.Context, id int64) error { - _, err := q.db.ExecContext(ctx, deletePendingSyncUpTo, id) - return err -} - const deleteProject = `-- name: DeleteProject :exec DELETE FROM projects WHERE id = ? ` @@ -504,26 +410,6 @@ func (q *Queries) GetMembership(ctx context.Context, arg GetMembershipParams) (P return i, err } -const getOldestPendingSync = `-- name: GetOldestPendingSync :one -SELECT id, operation, entity_type, entity_id, payload, created_at, attempts, last_error FROM pending_sync ORDER BY id ASC LIMIT 1 -` - -func (q *Queries) GetOldestPendingSync(ctx context.Context) (PendingSync, error) { - row := q.db.QueryRowContext(ctx, getOldestPendingSync) - var i PendingSync - err := row.Scan( - &i.ID, - &i.Operation, - &i.EntityType, - &i.EntityID, - &i.Payload, - &i.CreatedAt, - &i.Attempts, - &i.LastError, - ) - return i, err -} - const getProject = `-- name: GetProject :one SELECT id, name, description, kind, status, status_reason, closed_at, position, created_at FROM projects WHERE id = ? ` @@ -572,17 +458,6 @@ func (q *Queries) GetProjectMemberships(ctx context.Context, projectID string) ( return items, nil } -const getSyncOrigin = `-- name: GetSyncOrigin :one -SELECT api_url FROM sync_origin WHERE id = 1 -` - -func (q *Queries) GetSyncOrigin(ctx context.Context) (string, error) { - row := q.db.QueryRowContext(ctx, getSyncOrigin) - var api_url string - err := row.Scan(&api_url) - return api_url, err -} - const getTask = `-- name: GetTask :one SELECT id, item_id, title, completed, completed_at, position, created_at FROM project_item_tasks WHERE id = ? ` @@ -602,43 +477,6 @@ func (q *Queries) GetTask(ctx context.Context, id string) (ProjectItemTask, erro return i, err } -const hasPulledBefore = `-- name: HasPulledBefore :one -SELECT COUNT(*) FROM sync_state WHERE last_pull_at > '1970-01-01T00:00:00.000Z' -` - -// Whether this database has ever reconciled with an API. A database that has -// pulled was bound to something, even if nothing recorded what. -func (q *Queries) HasPulledBefore(ctx context.Context) (int64, error) { - row := q.db.QueryRowContext(ctx, hasPulledBefore) - var count int64 - err := row.Scan(&count) - return count, err -} - -const insertPendingSync = `-- name: InsertPendingSync :exec - -INSERT INTO pending_sync (operation, entity_type, entity_id, payload) -VALUES (?, ?, ?, ?) -` - -type InsertPendingSyncParams struct { - Operation string - EntityType string - EntityID string - Payload string -} - -// Sync: pending operations -func (q *Queries) InsertPendingSync(ctx context.Context, arg InsertPendingSyncParams) error { - _, err := q.db.ExecContext(ctx, insertPendingSync, - arg.Operation, - arg.EntityType, - arg.EntityID, - arg.Payload, - ) - return err -} - const insertUndoLog = `-- name: InsertUndoLog :exec INSERT INTO undo_log (action, entity_type, entity_id, previous_state) @@ -947,69 +785,6 @@ func (q *Queries) ListItemsInProject(ctx context.Context, projectID string) ([]L return items, nil } -const listPendingSync = `-- name: ListPendingSync :many -SELECT id, operation, entity_type, entity_id, payload, created_at, attempts, last_error FROM pending_sync ORDER BY id ASC -` - -func (q *Queries) ListPendingSync(ctx context.Context) ([]PendingSync, error) { - rows, err := q.db.QueryContext(ctx, listPendingSync) - if err != nil { - return nil, err - } - defer rows.Close() - var items []PendingSync - for rows.Next() { - var i PendingSync - if err := rows.Scan( - &i.ID, - &i.Operation, - &i.EntityType, - &i.EntityID, - &i.Payload, - &i.CreatedAt, - &i.Attempts, - &i.LastError, - ); err != nil { - return nil, err - } - items = append(items, i) - } - if err := rows.Close(); err != nil { - return nil, err - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - -const listPendingSyncEntityIDsAfter = `-- name: ListPendingSyncEntityIDsAfter :many -SELECT DISTINCT entity_id FROM pending_sync WHERE id > ? -` - -func (q *Queries) ListPendingSyncEntityIDsAfter(ctx context.Context, id int64) ([]string, error) { - rows, err := q.db.QueryContext(ctx, listPendingSyncEntityIDsAfter, id) - if err != nil { - return nil, err - } - defer rows.Close() - var items []string - for rows.Next() { - var entity_id string - if err := rows.Scan(&entity_id); err != nil { - return nil, err - } - items = append(items, entity_id) - } - if err := rows.Close(); err != nil { - return nil, err - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - const listProjectItemFacts = `-- name: ListProjectItemFacts :many SELECT m.project_id, pi.completed, pi.archived, pi.repo FROM project_item_memberships m @@ -1143,24 +918,10 @@ func (q *Queries) ListTasksByItem(ctx context.Context, itemID string) ([]Project return items, nil } -const maxPendingSyncID = `-- name: MaxPendingSyncID :one -SELECT CAST(COALESCE(MAX(id), 0) AS INTEGER) AS max_id FROM pending_sync -` - -func (q *Queries) MaxPendingSyncID(ctx context.Context) (int64, error) { - row := q.db.QueryRowContext(ctx, maxPendingSyncID) - var max_id int64 - err := row.Scan(&max_id) - return max_id, err -} - const nextItemNumber = `-- name: NextItemNumber :one SELECT COALESCE(MAX(number), 0) + 1 FROM project_items ` -// Only used when sync is off and this database assigns its own numbers. With -// sync on the server is the authority and this would hand out a number it is -// about to disagree with. func (q *Queries) NextItemNumber(ctx context.Context) (int64, error) { row := q.db.QueryRowContext(ctx, nextItemNumber) var column_1 int64 @@ -1284,7 +1045,7 @@ type RestoreProjectParams struct { CreatedAt string } -// Undo: a restore carries each column a pull leaves alone. +// Undo: a restore inserts the whole row, or, where it is still there, rewrites what an edit can change. func (q *Queries) RestoreProject(ctx context.Context, arg RestoreProjectParams) error { _, err := q.db.ExecContext(ctx, restoreProject, arg.ID, @@ -1378,35 +1139,6 @@ func (q *Queries) SearchItems(ctx context.Context, arg SearchItemsParams) ([]Pro return items, nil } -const setItemNumber = `-- name: SetItemNumber :exec -UPDATE project_items SET number = ? WHERE id = ? -` - -type SetItemNumberParams struct { - Number sql.NullInt64 - ID string -} - -func (q *Queries) SetItemNumber(ctx context.Context, arg SetItemNumberParams) error { - _, err := q.db.ExecContext(ctx, setItemNumber, arg.Number, arg.ID) - return err -} - -const setSyncOrigin = `-- name: SetSyncOrigin :exec -INSERT INTO sync_origin (id, api_url, adopted_at) -VALUES (1, ?, strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) -ON CONFLICT(id) DO UPDATE SET - api_url = excluded.api_url, - adopted_at = excluded.adopted_at -` - -// Adoption is deliberate and always overwrites: the caller has already decided -// this database belongs to this API. -func (q *Queries) SetSyncOrigin(ctx context.Context, apiUrl string) error { - _, err := q.db.ExecContext(ctx, setSyncOrigin, apiUrl) - return err -} - const updateItem = `-- name: UpdateItem :one UPDATE project_items SET title = ?, @@ -1471,20 +1203,6 @@ func (q *Queries) UpdateItemPosition(ctx context.Context, arg UpdateItemPosition return err } -const updatePendingSyncError = `-- name: UpdatePendingSyncError :exec -UPDATE pending_sync SET attempts = attempts + 1, last_error = ? WHERE id = ? -` - -type UpdatePendingSyncErrorParams struct { - LastError sql.NullString - ID int64 -} - -func (q *Queries) UpdatePendingSyncError(ctx context.Context, arg UpdatePendingSyncErrorParams) error { - _, err := q.db.ExecContext(ctx, updatePendingSyncError, arg.LastError, arg.ID) - return err -} - const updateProject = `-- name: UpdateProject :one UPDATE projects SET name = ?, @@ -1602,149 +1320,3 @@ func (q *Queries) UpsertDependency(ctx context.Context, arg UpsertDependencyPara _, err := q.db.ExecContext(ctx, upsertDependency, arg.ItemID, arg.DependsOnID) return err } - -const upsertItem = `-- name: UpsertItem :exec -INSERT INTO project_items (id, number, title, notes, repo, completed, archived, created_at, updated_at) -VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) -ON CONFLICT(id) DO UPDATE SET - number = excluded.number, - title = excluded.title, - notes = excluded.notes, - repo = excluded.repo, - completed = excluded.completed, - archived = excluded.archived, - updated_at = excluded.updated_at -` - -type UpsertItemParams struct { - ID string - Number sql.NullInt64 - Title string - Notes sql.NullString - Repo sql.NullString - Completed int64 - Archived int64 - CreatedAt string - UpdatedAt string -} - -func (q *Queries) UpsertItem(ctx context.Context, arg UpsertItemParams) error { - _, err := q.db.ExecContext(ctx, upsertItem, - arg.ID, - arg.Number, - arg.Title, - arg.Notes, - arg.Repo, - arg.Completed, - arg.Archived, - arg.CreatedAt, - arg.UpdatedAt, - ) - return err -} - -const upsertMembership = `-- name: UpsertMembership :exec -INSERT INTO project_item_memberships (item_id, project_id, position) -VALUES (?, ?, ?) -ON CONFLICT(item_id, project_id) DO UPDATE SET - position = excluded.position -` - -type UpsertMembershipParams struct { - ItemID string - ProjectID string - Position int64 -} - -func (q *Queries) UpsertMembership(ctx context.Context, arg UpsertMembershipParams) error { - _, err := q.db.ExecContext(ctx, upsertMembership, arg.ItemID, arg.ProjectID, arg.Position) - return err -} - -const upsertProject = `-- name: UpsertProject :exec - -INSERT INTO projects (id, name, description, status, status_reason, closed_at, position, created_at) -VALUES (?, ?, ?, ?, ?, ?, ?, ?) -ON CONFLICT(id) DO UPDATE SET - name = excluded.name, - description = excluded.description, - status = excluded.status, - status_reason = excluded.status_reason, - closed_at = excluded.closed_at, - position = excluded.position -` - -type UpsertProjectParams struct { - ID string - Name string - Description sql.NullString - Status string - StatusReason sql.NullString - ClosedAt sql.NullString - Position int64 - CreatedAt string -} - -// Sync: pull reconciliation (upserts) -func (q *Queries) UpsertProject(ctx context.Context, arg UpsertProjectParams) error { - _, err := q.db.ExecContext(ctx, upsertProject, - arg.ID, - arg.Name, - arg.Description, - arg.Status, - arg.StatusReason, - arg.ClosedAt, - arg.Position, - arg.CreatedAt, - ) - return err -} - -const upsertSyncState = `-- name: UpsertSyncState :exec -INSERT INTO sync_state (entity_type, last_pull_at, last_push_at) -VALUES (?, ?, ?) -ON CONFLICT(entity_type) DO UPDATE SET - last_pull_at = CASE WHEN excluded.last_pull_at > sync_state.last_pull_at THEN excluded.last_pull_at ELSE sync_state.last_pull_at END, - last_push_at = CASE WHEN excluded.last_push_at > sync_state.last_push_at THEN excluded.last_push_at ELSE sync_state.last_push_at END -` - -type UpsertSyncStateParams struct { - EntityType string - LastPullAt string - LastPushAt string -} - -func (q *Queries) UpsertSyncState(ctx context.Context, arg UpsertSyncStateParams) error { - _, err := q.db.ExecContext(ctx, upsertSyncState, arg.EntityType, arg.LastPullAt, arg.LastPushAt) - return err -} - -const upsertTask = `-- name: UpsertTask :exec -INSERT INTO project_item_tasks (id, item_id, title, completed, position, created_at) -VALUES (?, ?, ?, ?, ?, ?) -ON CONFLICT(id) DO UPDATE SET - title = excluded.title, - completed = excluded.completed, - position = excluded.position -` - -type UpsertTaskParams struct { - ID string - ItemID string - Title string - Completed int64 - Position int64 - CreatedAt string -} - -func (q *Queries) UpsertTask(ctx context.Context, arg UpsertTaskParams) error { - _, err := q.db.ExecContext(ctx, upsertTask, - arg.ID, - arg.ItemID, - arg.Title, - arg.Completed, - arg.Position, - arg.CreatedAt, - ) - return err -} diff --git a/store/sqlite/open.go b/store/sqlite/open.go index bf1963d..a49da41 100644 --- a/store/sqlite/open.go +++ b/store/sqlite/open.go @@ -23,7 +23,7 @@ var lookupRows string // `PRAGMA foreign_keys=ON` through db.Exec sets it on whichever single // connection served that call and leaves every other one with SQLite's default, // which is off — so the ON DELETE CASCADE on every child table was inert, and a -// project deleted by a sync sweep left its memberships behind as orphans. +// deleted project left its memberships behind as orphans. // // A transaction takes the write lock when it begins, because a rule reads inside // it and then writes. SQLite refuses a deferred transaction's upgrade from read to @@ -67,7 +67,7 @@ func migrate(db *sql.DB) error { var name string err := db.QueryRow("SELECT name FROM sqlite_master WHERE type='table' AND name='project_items'").Scan(&name) if err == sql.ErrNoRows { - // Fresh database — apply full schema (includes sync tables) + // Fresh database — apply full schema if _, err := db.Exec(schema); err != nil { return fmt.Errorf("applying schema: %w", err) } @@ -77,10 +77,7 @@ func migrate(db *sql.DB) error { } // Existing database — apply incremental migrations - if err := migrateSyncTables(db); err != nil { - return err - } - if err := migrateSyncOrigin(db); err != nil { + if err := dropSyncTables(db); err != nil { return err } if err := migrateOrphanedChildRows(db); err != nil { @@ -137,40 +134,55 @@ CREATE TABLE IF NOT EXISTS project_statuses (name TEXT PRIMARY KEY);` return nil } -// migrateSyncOrigin adds the table that pairs a database with its API. Left -// empty for an existing database: which API it belongs to is not derivable from -// anything in it, so the first pull is what records it. -func migrateSyncOrigin(db *sql.DB) error { +// dropSyncTables removes what a database kept while it was a copy of an API: the +// queue of pushes, the pull times, and the API it was bound to. It refuses while +// the queue holds a row, because that edit never reached the API and the queue is +// the only record saying so. The other two hold nothing a person wrote. +// +// MIGRATION: retires with config/retired_sync.go. +func dropSyncTables(db *sql.DB) error { var name string - err := db.QueryRow("SELECT name FROM sqlite_master WHERE type='table' AND name='sync_origin'").Scan(&name) + err := db.QueryRow("SELECT name FROM sqlite_master WHERE type='table' AND name='pending_sync'").Scan(&name) + if err != nil && err != sql.ErrNoRows { + return fmt.Errorf("checking pending_sync: %w", err) + } if err == nil { - return nil - } else if err != sql.ErrNoRows { - return fmt.Errorf("checking sync_origin: %w", err) + var queued int + if err := db.QueryRow("SELECT COUNT(*) FROM pending_sync").Scan(&queued); err != nil { + return fmt.Errorf("counting pending_sync: %w", err) + } + if queued > 0 { + return fmt.Errorf("%d edits here were queued for %s and never reached it. Push them with `projects sync` "+ + "from v1.24.2, or discard them with DELETE FROM pending_sync", queued, boundAPI(db)) + } } - - const create = ` -CREATE TABLE sync_origin ( - id INTEGER PRIMARY KEY CHECK (id = 1), - api_url TEXT NOT NULL, - adopted_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) -);` - if _, err := db.Exec(create); err != nil { - return fmt.Errorf("creating sync_origin: %w", err) + const drop = ` +DROP TABLE IF EXISTS pending_sync; +DROP TABLE IF EXISTS sync_state; +DROP TABLE IF EXISTS sync_origin;` + if _, err := db.Exec(drop); err != nil { + return fmt.Errorf("dropping the sync tables: %w", err) } return nil } +// boundAPI is the API a synced database was bound to, as its refusal names it. +func boundAPI(db *sql.DB) string { + var url string + if err := db.QueryRow("SELECT api_url FROM sync_origin WHERE id = 1").Scan(&url); err != nil { + return "the API this database synced with" + } + return url +} + // migrateOrphanedChildRows deletes rows the cascades would never have allowed: // memberships, dependencies and tasks whose parent is gone. Every database -// written before foreign keys were actually enforced has them — a sync sweep -// that deleted a project left its memberships pointing at nothing, and a real -// database had accumulated 46 of them. +// written before foreign keys were actually enforced has them — a deleted +// project left its memberships pointing at nothing. // // Deleting is not a data loss decision. A membership whose project does not -// exist names no project, so nothing can reach it and nothing can render it; -// were the project to come back, the item pull rebuilds the membership from the -// server's own answer. It runs before migrateProjectStatus because that +// exist names no project, so nothing can reach it and nothing can render it. It +// runs before migrateProjectStatus because that // migration's foreign key check is what these rows fail. func migrateOrphanedChildRows(db *sql.DB) error { sweeps := []struct { @@ -209,9 +221,8 @@ func migrateOrphanedChildRows(db *sql.DB) error { // // The unique constraint is why this is a table rebuild rather than three ALTER // TABLE ADD COLUMNs. SQLite attaches it as an implicit index that no DDL can -// drop, and leaving it would fail the entire pull transaction the first time the -// server returned a completed project beside a live one sharing its name — -// exactly the case the partial index exists to allow. +// drop, and leaving it would refuse a completed project beside a live one +// sharing its name — exactly the case the partial index exists to allow. // // Foreign keys are off for the swap because project_item_memberships references // projects ON DELETE CASCADE, so dropping the old table with them on would take @@ -289,9 +300,8 @@ ALTER TABLE projects_rebuilt RENAME TO projects;` } // migrateItemNumber adds project_items.number to databases created before the -// column existed. Left null rather than backfilled: with sync on the server -// owns the numbering and the next pull fills every row in, and with sync off -// the first create allocates from an empty column, which starts at 1. +// column existed. Left null rather than backfilled, so the first create +// allocates from an empty column, which starts at 1. // // The UNIQUE constraint lives in indexes.sql because ALTER TABLE ADD COLUMN // cannot carry one. @@ -311,7 +321,7 @@ func migrateItemNumber(db *sql.DB) error { // migrateItemRepo adds project_items.repo to databases created before the // column existed. Detected by column presence rather than a version counter, -// matching how migrateSyncTables probes for its tables. +// as every migration here is. func migrateItemRepo(db *sql.DB) error { var name string err := db.QueryRow("SELECT name FROM pragma_table_info('project_items') WHERE name = 'repo'").Scan(&name) @@ -325,32 +335,3 @@ func migrateItemRepo(db *sql.DB) error { } return nil } - -func migrateSyncTables(db *sql.DB) error { - var name string - err := db.QueryRow("SELECT name FROM sqlite_master WHERE type='table' AND name='pending_sync'").Scan(&name) - if err == sql.ErrNoRows { - const syncMigration = ` -CREATE TABLE pending_sync ( - id INTEGER PRIMARY KEY AUTOINCREMENT, - operation TEXT NOT NULL, - entity_type TEXT NOT NULL, - entity_id TEXT NOT NULL, - payload TEXT NOT NULL, - created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), - attempts INTEGER NOT NULL DEFAULT 0, - last_error TEXT -); -CREATE TABLE sync_state ( - entity_type TEXT PRIMARY KEY, - last_pull_at TEXT NOT NULL DEFAULT '1970-01-01T00:00:00.000Z', - last_push_at TEXT NOT NULL DEFAULT '1970-01-01T00:00:00.000Z' -);` - if _, err := db.Exec(syncMigration); err != nil { - return fmt.Errorf("applying sync migration: %w", err) - } - } else if err != nil { - return fmt.Errorf("checking sync tables: %w", err) - } - return nil -} diff --git a/store/sqlite/open_test.go b/store/sqlite/open_test.go index 3b228f1..4b8ffd3 100644 --- a/store/sqlite/open_test.go +++ b/store/sqlite/open_test.go @@ -115,76 +115,6 @@ func mustExec(t *testing.T, database *sql.DB, query string) { } } -func TestOpen_IncrementalMigration(t *testing.T) { - // Simulate an existing database without sync tables - database, err := sql.Open("sqlite", ":memory:") - if err != nil { - t.Fatalf("opening raw db: %v", err) - } - database.SetMaxOpenConns(1) - - // Create only the core tables (no sync tables) - _, err = database.Exec(` - CREATE TABLE projects ( - id TEXT PRIMARY KEY, - name TEXT NOT NULL UNIQUE, - description TEXT, - position INTEGER NOT NULL DEFAULT 0, - created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) - ); - CREATE TABLE project_items ( - id TEXT PRIMARY KEY, - title TEXT NOT NULL, - completed INTEGER NOT NULL DEFAULT 0, - archived INTEGER NOT NULL DEFAULT 0, - created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), - updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) - ); - `) - if err != nil { - t.Fatalf("creating core tables: %v", err) - } - - // Insert test data - _, err = database.Exec("INSERT INTO projects (id, name) VALUES ('p1', 'existing')") - if err != nil { - t.Fatalf("inserting test data: %v", err) - } - - // Run migration — should add sync tables without losing data - if err := migrate(database); err != nil { - t.Fatalf("migrate: %v", err) - } - - // Verify sync tables exist - var name string - err = database.QueryRow( - "SELECT name FROM sqlite_master WHERE type='table' AND name='pending_sync'", - ).Scan(&name) - if err != nil { - t.Error("pending_sync table not created by migration") - } - - err = database.QueryRow( - "SELECT name FROM sqlite_master WHERE type='table' AND name='sync_state'", - ).Scan(&name) - if err != nil { - t.Error("sync_state table not created by migration") - } - - // Verify existing data survived - var projectName string - err = database.QueryRow("SELECT name FROM projects WHERE id='p1'").Scan(&projectName) - if err != nil { - t.Fatalf("existing data lost: %v", err) - } - if projectName != "existing" { - t.Errorf("expected project name 'existing', got %q", projectName) - } - - _ = database.Close() -} - func TestMigrate_AddsRepoColumnToExistingDatabase(t *testing.T) { database, err := sql.Open("sqlite", ":memory:") if err != nil { @@ -270,61 +200,6 @@ func TestOpen_ExistingDatabaseGainsTheUniquenessRules(t *testing.T) { } } -func TestMigrate_AddsNumberColumnToExistingDatabase(t *testing.T) { - database, err := sql.Open("sqlite", ":memory:") - if err != nil { - t.Fatalf("opening raw db: %v", err) - } - database.SetMaxOpenConns(1) - - _, err = database.Exec(` - CREATE TABLE projects ( - id TEXT PRIMARY KEY, - name TEXT NOT NULL UNIQUE, - description TEXT, - position INTEGER NOT NULL DEFAULT 0, - created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) - ); - CREATE TABLE project_items ( - id TEXT PRIMARY KEY, - title TEXT NOT NULL, - notes TEXT, - repo TEXT, - completed INTEGER NOT NULL DEFAULT 0, - archived INTEGER NOT NULL DEFAULT 0, - created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), - updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) - ); - INSERT INTO project_items (id, title) VALUES ('i1', 'pre-number item'); - `) - if err != nil { - t.Fatalf("creating pre-number table: %v", err) - } - - if err := migrate(database); err != nil { - t.Fatalf("migrate: %v", err) - } - - // Deliberately not backfilled: with sync on the pull supplies every number, - // and with sync off the first create allocates from an empty column. - var number sql.NullInt64 - var title string - row := database.QueryRow("SELECT title, number FROM project_items WHERE id = 'i1'") - if err := row.Scan(&title, &number); err != nil { - t.Fatalf("selecting migrated row: %v", err) - } - if title != "pre-number item" { - t.Errorf("existing row was disturbed: title = %q", title) - } - if number.Valid { - t.Errorf("number should be null for a pre-existing item, got %d", number.Int64) - } - - if err := migrate(database); err != nil { - t.Fatalf("second migrate: %v", err) - } -} - // TestMigrate_ReplacesTheGlobalNameConstraint is the one migration that rebuilds // a table rather than adding a column, because SQLite attaches a column-level // UNIQUE as an index no DDL can drop. Two things have to survive it: the rows, diff --git a/store/sqlite/queries.sql b/store/sqlite/queries.sql index 092e942..90c60ae 100644 --- a/store/sqlite/queries.sql +++ b/store/sqlite/queries.sql @@ -55,15 +55,9 @@ INSERT INTO project_items (id, number, title, notes, repo) VALUES (?, ?, ?, ?, ?) RETURNING *; --- Only used when sync is off and this database assigns its own numbers. With --- sync on the server is the authority and this would hand out a number it is --- about to disagree with. -- name: NextItemNumber :one SELECT COALESCE(MAX(number), 0) + 1 FROM project_items; --- name: SetItemNumber :exec -UPDATE project_items SET number = ? WHERE id = ?; - -- name: UpdateItem :one UPDATE project_items SET title = ?, @@ -175,77 +169,7 @@ RETURNING *; -- name: DeleteTask :exec DELETE FROM project_item_tasks WHERE id = ?; --- Sync: pending operations - --- name: InsertPendingSync :exec -INSERT INTO pending_sync (operation, entity_type, entity_id, payload) -VALUES (?, ?, ?, ?); - --- name: ListPendingSync :many -SELECT * FROM pending_sync ORDER BY id ASC; - --- name: GetOldestPendingSync :one -SELECT * FROM pending_sync ORDER BY id ASC LIMIT 1; - --- name: DeletePendingSync :exec -DELETE FROM pending_sync WHERE id = ?; - --- name: DeletePendingSyncByEntity :execrows -DELETE FROM pending_sync WHERE entity_id = ?; - --- name: UpdatePendingSyncError :exec -UPDATE pending_sync SET attempts = attempts + 1, last_error = ? WHERE id = ?; - --- name: CountPendingSync :one -SELECT COUNT(*) FROM pending_sync; - --- name: DeleteAllPendingSync :exec -DELETE FROM pending_sync; - --- name: MaxPendingSyncID :one -SELECT CAST(COALESCE(MAX(id), 0) AS INTEGER) AS max_id FROM pending_sync; - --- name: DeletePendingSyncUpTo :exec -DELETE FROM pending_sync WHERE id <= ?; - --- name: ListPendingSyncEntityIDsAfter :many -SELECT DISTINCT entity_id FROM pending_sync WHERE id > ?; - --- Sync: state tracking - --- Counted rather than listed: the caller only needs to know whether this --- database has anything to lose. --- name: CountProjects :one -SELECT COUNT(*) FROM projects; - --- name: CountItems :one -SELECT COUNT(*) FROM project_items; - --- Whether this database has ever reconciled with an API. A database that has --- pulled was bound to something, even if nothing recorded what. --- name: HasPulledBefore :one -SELECT COUNT(*) FROM sync_state WHERE last_pull_at > '1970-01-01T00:00:00.000Z'; - --- name: GetSyncOrigin :one -SELECT api_url FROM sync_origin WHERE id = 1; - --- Adoption is deliberate and always overwrites: the caller has already decided --- this database belongs to this API. --- name: SetSyncOrigin :exec -INSERT INTO sync_origin (id, api_url, adopted_at) -VALUES (1, ?, strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) -ON CONFLICT(id) DO UPDATE SET - api_url = excluded.api_url, - adopted_at = excluded.adopted_at; - --- name: UpsertSyncState :exec -INSERT INTO sync_state (entity_type, last_pull_at, last_push_at) -VALUES (?, ?, ?) -ON CONFLICT(entity_type) DO UPDATE SET - last_pull_at = CASE WHEN excluded.last_pull_at > sync_state.last_pull_at THEN excluded.last_pull_at ELSE sync_state.last_pull_at END, - last_push_at = CASE WHEN excluded.last_push_at > sync_state.last_push_at THEN excluded.last_push_at ELSE sync_state.last_push_at END; - --- Undo: a restore carries each column a pull leaves alone. +-- Undo: a restore inserts the whole row, or, where it is still there, rewrites what an edit can change. -- name: RestoreProject :exec INSERT INTO projects (id, name, description, kind, status, status_reason, closed_at, position, created_at) @@ -281,55 +205,10 @@ ON CONFLICT(id) DO UPDATE SET completed_at = excluded.completed_at, position = excluded.position; --- Sync: pull reconciliation (upserts) - --- name: UpsertProject :exec -INSERT INTO projects (id, name, description, status, status_reason, closed_at, position, created_at) -VALUES (?, ?, ?, ?, ?, ?, ?, ?) -ON CONFLICT(id) DO UPDATE SET - name = excluded.name, - description = excluded.description, - status = excluded.status, - status_reason = excluded.status_reason, - closed_at = excluded.closed_at, - position = excluded.position; - --- name: UpsertItem :exec -INSERT INTO project_items (id, number, title, notes, repo, completed, archived, created_at, updated_at) -VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) -ON CONFLICT(id) DO UPDATE SET - number = excluded.number, - title = excluded.title, - notes = excluded.notes, - repo = excluded.repo, - completed = excluded.completed, - archived = excluded.archived, - updated_at = excluded.updated_at; - --- name: UpsertTask :exec -INSERT INTO project_item_tasks (id, item_id, title, completed, position, created_at) -VALUES (?, ?, ?, ?, ?, ?) -ON CONFLICT(id) DO UPDATE SET - title = excluded.title, - completed = excluded.completed, - position = excluded.position; - --- name: UpsertMembership :exec -INSERT INTO project_item_memberships (item_id, project_id, position) -VALUES (?, ?, ?) -ON CONFLICT(item_id, project_id) DO UPDATE SET - position = excluded.position; - -- name: UpsertDependency :exec INSERT OR IGNORE INTO project_item_dependencies (item_id, depends_on_id) VALUES (?, ?); --- name: DeleteAllMemberships :exec -DELETE FROM project_item_memberships; - --- name: DeleteAllDependencies :exec -DELETE FROM project_item_dependencies; - -- name: ListAllProjectsRaw :many SELECT * FROM projects ORDER BY position, name; diff --git a/store/sqlite/schema.sql b/store/sqlite/schema.sql index f9277f1..c8120a8 100644 --- a/store/sqlite/schema.sql +++ b/store/sqlite/schema.sql @@ -13,9 +13,7 @@ CREATE TABLE project_statuses ( CREATE TABLE projects ( id TEXT PRIMARY KEY, -- Unique among ACTIVE projects only, via the partial index in indexes.sql. - -- A global UNIQUE would make a completed `clisteno` own the name forever, - -- and would fail the whole pull the moment the server returns two projects - -- that shared one. + -- A global UNIQUE would make a completed `clisteno` own the name forever. name TEXT NOT NULL, description TEXT, -- A name in project_kinds, checked by the rule that writes it. @@ -23,8 +21,7 @@ CREATE TABLE projects ( -- active | someday | completed | dropped. A project is a finite effort, so -- completing it is what hides it — there is no separate archived flag the way -- items have one. `dropped` requires a reason, which is what stops it reading - -- as deferred. A name in project_statuses, checked by the rule that writes it - -- rather than by a CHECK, so a pull mirrors what a server tells it. + -- as deferred. A name in project_statuses, checked by the rule that writes it. status TEXT NOT NULL DEFAULT 'active', status_reason TEXT, closed_at TEXT, @@ -34,10 +31,9 @@ CREATE TABLE projects ( CREATE TABLE project_items ( id TEXT PRIMARY KEY, - -- The handle: short enough to type, unlike the UUID key. Assigned by the API - -- when sync is on, and locally when it is off and this database is the only - -- authority. Null in the window between creating an item offline and the push - -- that earns it a number. + -- The handle: short enough to type, unlike the UUID key. Assigned as each + -- item is created. Null on an item from before the column, and on one a + -- database once kept as a copy of an API created before the API numbered it. number INTEGER, title TEXT NOT NULL, notes TEXT, @@ -45,8 +41,8 @@ CREATE TABLE project_items ( -- all — home projects, things to sell, errands. repo TEXT, completed INTEGER NOT NULL DEFAULT 0, - -- Null where the completion time is unknown: never finished, finished before - -- this column existed, or finished on a server a pull mirrors. + -- Null where the completion time is unknown: never finished, or finished + -- before this column existed. completed_at TEXT, archived INTEGER NOT NULL DEFAULT 0, created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), @@ -77,33 +73,6 @@ CREATE TABLE project_item_tasks ( created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) ); -CREATE TABLE pending_sync ( - id INTEGER PRIMARY KEY AUTOINCREMENT, - operation TEXT NOT NULL, - entity_type TEXT NOT NULL, - entity_id TEXT NOT NULL, - payload TEXT NOT NULL, - created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), - attempts INTEGER NOT NULL DEFAULT 0, - last_error TEXT -); - -CREATE TABLE sync_state ( - entity_type TEXT PRIMARY KEY, - last_pull_at TEXT NOT NULL DEFAULT '1970-01-01T00:00:00.000Z', - last_push_at TEXT NOT NULL DEFAULT '1970-01-01T00:00:00.000Z' -); - --- Which API this database belongs to. A pull deletes everything the server did --- not return, so a database and its API are a pair and pointing one at the other --- one is a wipe rather than a sync. Recording the pairing is what lets a pull --- refuse instead of obeying. Single row, enforced by the CHECK. -CREATE TABLE sync_origin ( - id INTEGER PRIMARY KEY CHECK (id = 1), - api_url TEXT NOT NULL, - adopted_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) -); - CREATE TABLE undo_log ( id INTEGER PRIMARY KEY AUTOINCREMENT, action TEXT NOT NULL, diff --git a/store/sqlite/sqlc.yaml b/store/sqlite/sqlc.yaml index 2c189f6..0b92fbb 100644 --- a/store/sqlite/sqlc.yaml +++ b/store/sqlite/sqlc.yaml @@ -6,4 +6,4 @@ sql: gen: go: package: "generated" - out: "generated" + out: "internal/generated" diff --git a/store/sqlite/store.go b/store/sqlite/store.go index dc01986..e0d41b5 100644 --- a/store/sqlite/store.go +++ b/store/sqlite/store.go @@ -11,7 +11,7 @@ import ( "github.com/datapointchris/projects/model" "github.com/datapointchris/projects/store" - "github.com/datapointchris/projects/store/sqlite/generated" + "github.com/datapointchris/projects/store/sqlite/internal/generated" ) // Store is store.Store over a database Open returned. diff --git a/sync/backend.go b/sync/backend.go deleted file mode 100644 index 3536f6f..0000000 --- a/sync/backend.go +++ /dev/null @@ -1,550 +0,0 @@ -package sync - -import ( - "fmt" - - "github.com/datapointchris/projects/backend" - "github.com/datapointchris/projects/model" - "github.com/datapointchris/projects/service" -) - -// SyncBackend wraps the service and queues sync operations after mutations. -// Read methods delegate directly; write methods delegate then queue + notify. -type SyncBackend struct { - local *service.Service - engine *Engine -} - -// Compile-time check that SyncBackend implements Backend. -var _ backend.Backend = (*SyncBackend)(nil) - -// NewSyncBackend creates a backend that writes locally and syncs to the remote API. -func NewSyncBackend(local *service.Service, engine *Engine) *SyncBackend { - return &SyncBackend{local: local, engine: engine} -} - -// --- Read methods: pass through to local --- - -func (s *SyncBackend) ListProjectsByStatus(status string) ([]model.ProjectWithItemCount, error) { - return s.local.ListProjectsByStatus(status) -} - -func (s *SyncBackend) GetProject(id string) (*model.ProjectWithItemCount, error) { - return s.local.GetProject(id) -} - -func (s *SyncBackend) ListAllItemsIncludingArchived() ([]model.ProjectItem, error) { - return s.local.ListAllItemsIncludingArchived() -} - -func (s *SyncBackend) ListItemsByProject(projectID string) ([]model.ProjectItemInProject, error) { - return s.local.ListItemsByProject(projectID) -} - -func (s *SyncBackend) GetItem(id string) (*model.ProjectItemDetail, error) { - return s.local.GetItem(id) -} - -func (s *SyncBackend) GetItemProjects(itemID string) ([]model.Project, error) { - return s.local.GetItemProjects(itemID) -} - -func (s *SyncBackend) GetBlockers(itemID string) ([]model.ProjectItem, error) { - return s.local.GetBlockers(itemID) -} - -func (s *SyncBackend) ListDependencies() ([]model.Dependency, error) { - return s.local.ListDependencies() -} - -func (s *SyncBackend) ListMemberships() ([]model.Membership, error) { - return s.local.ListMemberships() -} - -func (s *SyncBackend) ListTasks(itemID string) ([]model.ProjectItemTask, error) { - return s.local.ListTasks(itemID) -} - -func (s *SyncBackend) ListAllTasks() ([]model.ProjectItemTask, error) { - return s.local.ListAllTasks() -} - -func (s *SyncBackend) Search(query string) ([]model.ProjectItem, error) { - return s.local.Search(query) -} - -func (s *SyncBackend) ListBlocked() ([]model.ProjectItem, error) { - return s.local.ListBlocked() -} - -func (s *SyncBackend) ListArchived(projectID string) ([]model.ProjectItemInProject, error) { - return s.local.ListArchived(projectID) -} - -func (s *SyncBackend) CanUndo() (bool, error) { - return s.local.CanUndo() -} - -// --- Write methods: delegate to local, then queue + notify --- - -func (s *SyncBackend) CreateProject(input model.CreateProject) (*model.Project, error) { - result, err := s.local.CreateProject(input) - if err != nil { - return nil, err - } - _ = s.engine.QueueOp(OpCreateProject, result.ID, createProjectPayload{ - ID: result.ID, - Name: input.Name, - Description: input.Description, - Status: result.Status, - Reason: result.StatusReason, - }) - s.engine.Notify() - return result, nil -} - -func (s *SyncBackend) UpdateProject(id string, input model.UpdateProject) (*model.Project, error) { - result, err := s.local.UpdateProject(id, input) - if err != nil { - return nil, err - } - _ = s.engine.QueueOp(OpUpdateProject, id, input) - s.engine.Notify() - return result, nil -} - -// SetProjectStatus queues a PATCH rather than a complete/drop/reopen action -// endpoint, matching the API: one write path, so the transition is validated in -// one place. closed_at is left out of the payload because the server derives it -// from the status, exactly as the local statement does. -func (s *SyncBackend) SetProjectStatus(id, status string, reason *string) (*model.Project, error) { - result, err := s.local.SetProjectStatus(id, status, reason) - if err != nil { - return nil, err - } - _ = s.engine.QueueOp(OpUpdateProject, id, projectStatusPayload{ - Status: status, - Reason: result.StatusReason, - }) - s.engine.Notify() - return result, nil -} - -func (s *SyncBackend) DeleteProject(id string) error { - if err := s.local.DeleteProject(id); err != nil { - return err - } - _ = s.engine.QueueOp(OpDeleteProject, id, nil) - s.engine.Notify() - return nil -} - -func (s *SyncBackend) ReorderProject(projectID string, newPosition int) error { - if err := s.local.ReorderProject(projectID, newPosition); err != nil { - return err - } - _ = s.engine.QueueOp(OpReorderProject, projectID, projectReorderPayload{ - Position: newPosition, - }) - s.engine.Notify() - return nil -} - -func (s *SyncBackend) CreateItem(input model.CreateProjectItem) (*model.ProjectItemDetail, error) { - result, err := s.local.CreateItem(input) - if err != nil { - return nil, err - } - _ = s.engine.QueueOp(OpCreateItem, result.ID, createItemPayload{ - ID: result.ID, - Title: input.Title, - Notes: input.Notes, - Repo: input.Repo, - ProjectIDs: input.ProjectIDs, - }) - s.engine.Notify() - return result, nil -} - -func (s *SyncBackend) UpdateItem(id string, input model.UpdateProjectItem) (*model.ProjectItem, error) { - result, err := s.local.UpdateItem(id, input) - if err != nil { - return nil, err - } - _ = s.engine.QueueOp(OpUpdateItem, id, input) - s.engine.Notify() - return result, nil -} - -func (s *SyncBackend) DeleteItem(id string) error { - if err := s.local.DeleteItem(id); err != nil { - return err - } - _ = s.engine.QueueOp(OpDeleteItem, id, nil) - s.engine.Notify() - return nil -} - -func (s *SyncBackend) ReorderItem(itemID, projectID string, newPosition int) error { - if err := s.local.ReorderItem(itemID, projectID, newPosition); err != nil { - return err - } - _ = s.engine.QueueOp(OpReorderItem, itemID, reorderPayload{ - ProjectID: projectID, - Position: newPosition, - }) - s.engine.Notify() - return nil -} - -func (s *SyncBackend) AddToProject(itemID, projectID string) error { - if err := s.local.AddToProject(itemID, projectID); err != nil { - return err - } - _ = s.engine.QueueOp(OpAddToProject, itemID, projectIDPayload{ProjectID: projectID}) - s.engine.Notify() - return nil -} - -func (s *SyncBackend) RemoveFromProject(itemID, projectID string) error { - if err := s.local.RemoveFromProject(itemID, projectID); err != nil { - return err - } - _ = s.engine.QueueOp(OpRemoveFromProject, itemID, projectIDPayload{ProjectID: projectID}) - s.engine.Notify() - return nil -} - -func (s *SyncBackend) AddDependency(itemID, dependsOn string) error { - if err := s.local.AddDependency(itemID, dependsOn); err != nil { - return err - } - _ = s.engine.QueueOp(OpAddDependency, itemID, depPayload{DependsOnID: dependsOn}) - s.engine.Notify() - return nil -} - -func (s *SyncBackend) RemoveDependency(itemID, dependsOn string) error { - if err := s.local.RemoveDependency(itemID, dependsOn); err != nil { - return err - } - _ = s.engine.QueueOp(OpRemoveDependency, itemID, depPayload{DependsOnID: dependsOn}) - s.engine.Notify() - return nil -} - -func (s *SyncBackend) CreateTask(itemID string, input model.CreateProjectItemTask) (*model.ProjectItemTask, error) { - result, err := s.local.CreateTask(itemID, input) - if err != nil { - return nil, err - } - _ = s.engine.QueueOp(OpCreateTask, result.ID, taskPayload{ - ID: result.ID, - ItemID: itemID, - Title: input.Title, - }) - s.engine.Notify() - return result, nil -} - -func (s *SyncBackend) UpdateTask(itemID, taskID string, input model.UpdateProjectItemTask) (*model.ProjectItemTask, error) { - result, err := s.local.UpdateTask(itemID, taskID, input) - if err != nil { - return nil, err - } - _ = s.engine.QueueOp(OpUpdateTask, taskID, taskUpdatePayload{ - ItemID: itemID, - Title: input.Title, - Completed: input.Completed, - Position: input.Position, - }) - s.engine.Notify() - return result, nil -} - -func (s *SyncBackend) DeleteTask(itemID, taskID string) error { - if err := s.local.DeleteTask(itemID, taskID); err != nil { - return err - } - _ = s.engine.QueueOp(OpDeleteTask, taskID, taskRefPayload{ItemID: itemID}) - s.engine.Notify() - return nil -} - -// Undo reverses the local change and then reconciles the sync queue with it. -// Treating undo as local-only leaves the queued operations for that entity in -// place, so the next push recreates on the server exactly what was just undone. -func (s *SyncBackend) Undo() (*model.UndoResult, error) { - result, err := s.local.Undo() - if err != nil { - return nil, err - } - - // Memberships, dependencies, and item reorders all record the item as their - // entity ID, so dropping by entity ID would take unrelated queued edits to - // that item with them. Queue the inverse operation instead — a push that - // finds nothing to reverse gets a 404 or 409, which the push loop already - // treats as settled. - switch result.EntityType { - case "membership", "dependency", "item_position", "project_position": - s.queueUndoInverse(result) - s.engine.Notify() - return result, nil - } - - // Anything still queued describes the state the undo just reversed. - dropped, dropErr := s.engine.DropOpsForEntity(result.EntityID) - if dropErr != nil { - return result, fmt.Errorf("reconciling sync queue: %w", dropErr) - } - - if result.EntityType == "task" { - s.queueTaskUndo(result, dropped) - s.engine.Notify() - return result, nil - } - - // Undoing a delete when the delete had already been pushed means the row - // is gone from the server, so it has to be recreated rather than updated — - // and recreated with everything that cascaded off it, since a pull rebuilds - // memberships and dependencies from server state alone. - deleteWasPushed := result.Action == "delete" && dropped == 0 - - if result.EntityType == "project" { - switch { - case result.RestoredProject != nil && deleteWasPushed: - restored := result.RestoredProject - _ = s.engine.QueueOp(OpCreateProject, result.EntityID, createProjectPayload{ - ID: restored.ID, - Name: restored.Name, - Description: restored.Description, - Status: restored.Status, - Reason: restored.StatusReason, - }) - for _, itemID := range result.RestoredItemIDs { - _ = s.engine.QueueOp(OpAddToProject, itemID, projectIDPayload{ProjectID: result.EntityID}) - } - case result.RestoredProject != nil: - restored := result.RestoredProject - // One PATCH carrying the whole restored row, status included: the - // undo may have reversed a complete or a drop, and a body that - // omitted status would leave the server showing the project as - // closed after it came back locally. - _ = s.engine.QueueOp(OpUpdateProject, result.EntityID, projectRestorePayload{ - Name: restored.Name, - Description: restored.Description, - Position: restored.Position, - Status: restored.Status, - Reason: restored.StatusReason, - }) - case dropped == 0: - _ = s.engine.QueueOp(OpDeleteProject, result.EntityID, nil) - } - s.engine.Notify() - return result, nil - } - - switch { - case result.Restored != nil && deleteWasPushed: - restored := result.Restored - _ = s.engine.QueueOp(OpCreateItem, result.EntityID, createItemPayload{ - ID: restored.ID, - Title: restored.Title, - Notes: restored.Notes, - Repo: restored.Repo, - ProjectIDs: result.RestoredProjectIDs, - }) - s.queueRestoredChildren(result) - case result.Restored != nil: - // The row is back; push its restored state. Idempotent whether or not - // the reversed change had already been pushed. - restored := result.Restored - unlinked := "" - repo := restored.Repo - if repo == nil { - // A nil Repo is omitted from the payload and would leave a link the - // undo removed. Empty string is how this API unlinks one. - repo = &unlinked - } - _ = s.engine.QueueOp(OpUpdateItem, result.EntityID, model.UpdateProjectItem{ - Title: &restored.Title, - Notes: restored.Notes, - Repo: repo, - Completed: &restored.Completed, - Archived: &restored.Archived, - }) - case dropped == 0: - // The row is gone locally and its create had already been pushed, so - // the server still has it. With a queued create there is nothing on the - // server to delete. - _ = s.engine.QueueOp(OpDeleteItem, result.EntityID, nil) - } - - s.engine.Notify() - return result, nil -} - -// queueUndoInverse pushes the reversal of a membership, dependency, or reorder -// undo. Detail carries which way the reversal went; Removed means the undo took -// the row away rather than putting it back. -func (s *SyncBackend) queueUndoInverse(result *model.UndoResult) { - detail := result.Detail - if detail == nil { - return - } - - switch result.EntityType { - case "membership": - if detail.Removed { - _ = s.engine.QueueOp(OpRemoveFromProject, detail.ItemID, projectIDPayload{ProjectID: detail.ProjectID}) - return - } - _ = s.engine.QueueOp(OpAddToProject, detail.ItemID, projectIDPayload{ProjectID: detail.ProjectID}) - - case "dependency": - if detail.Removed { - _ = s.engine.QueueOp(OpRemoveDependency, detail.ItemID, depPayload{DependsOnID: detail.DependsOnID}) - return - } - _ = s.engine.QueueOp(OpAddDependency, detail.ItemID, depPayload{DependsOnID: detail.DependsOnID}) - - case "item_position": - _ = s.engine.QueueOp(OpReorderItem, detail.ItemID, reorderPayload{ - ProjectID: detail.ProjectID, - Position: detail.Position, - }) - - case "project_position": - _ = s.engine.QueueOp(OpReorderProject, result.EntityID, projectReorderPayload{Position: detail.Position}) - } -} - -// queueTaskUndo pushes the reversal of a task undo. dropped counts operations -// still queued for the task, so a non-zero count means the change being undone -// never reached the server and needs no reversal there. -func (s *SyncBackend) queueTaskUndo(result *model.UndoResult, dropped int64) { - detail := result.Detail - if detail == nil { - return - } - - switch { - case detail.Removed: - if dropped == 0 { - _ = s.engine.QueueOp(OpDeleteTask, result.EntityID, taskRefPayload{ItemID: detail.ItemID}) - } - - case result.Action == "delete": - if dropped > 0 || detail.Task == nil { - return - } - task := detail.Task - _ = s.engine.QueueOp(OpCreateTask, task.ID, taskPayload{ID: task.ID, ItemID: task.ItemID, Title: task.Title}) - if task.Completed { - _ = s.engine.QueueOp(OpCompleteTask, task.ID, taskRefPayload{ItemID: task.ItemID}) - } - - case detail.Task != nil: - task := detail.Task - position := task.Position - _ = s.engine.QueueOp(OpUpdateTask, task.ID, taskUpdatePayload{ - ItemID: task.ItemID, - Title: &task.Title, - Completed: &task.Completed, - Position: &position, - }) - } -} - -// queueRestoredChildren pushes the tasks and dependencies that came back with -// a restored item. The item's own create carries its memberships. -func (s *SyncBackend) queueRestoredChildren(result *model.UndoResult) { - for _, task := range result.RestoredTasks { - _ = s.engine.QueueOp(OpCreateTask, task.ID, taskPayload{ - ID: task.ID, - ItemID: task.ItemID, - Title: task.Title, - }) - if task.Completed { - _ = s.engine.QueueOp(OpCompleteTask, task.ID, taskRefPayload{ItemID: task.ItemID}) - } - } - for _, dependency := range result.RestoredDependencies { - _ = s.engine.QueueOp(OpAddDependency, dependency.ItemID, depPayload{ - DependsOnID: dependency.DependsOnID, - }) - } -} - -// --- Payload types for JSON serialization into pending_sync --- - -type createProjectPayload struct { - ID string `json:"id"` - Name string `json:"name"` - Description *string `json:"description,omitempty"` - // Sent even when active so a project completed offline does not come back - // alive on the push that first tells the server it exists. - Status string `json:"status"` - Reason *string `json:"status_reason,omitempty"` -} - -// projectStatusPayload is the PATCH body for a transition. Reason is whatever -// the local statement settled on — the value sent when dropping, and null after -// a reopen cleared it — so the two databases agree without a second round trip. -type projectStatusPayload struct { - Status string `json:"status"` - Reason *string `json:"status_reason"` -} - -// projectRestorePayload is an undo's PATCH: the whole row as it was, so one -// request reverses whichever field the undone mutation had changed. -type projectRestorePayload struct { - Name string `json:"name"` - Description *string `json:"description"` - Position int `json:"position"` - Status string `json:"status"` - Reason *string `json:"status_reason"` -} - -type createItemPayload struct { - ID string `json:"id"` - Title string `json:"title"` - Notes *string `json:"notes,omitempty"` - Repo *string `json:"repo,omitempty"` - ProjectIDs []string `json:"project_ids"` -} - -type reorderPayload struct { - ProjectID string `json:"project_id"` - Position int `json:"position"` -} - -type projectReorderPayload struct { - Position int `json:"position"` -} - -type projectIDPayload struct { - ProjectID string `json:"project_id"` -} - -type depPayload struct { - DependsOnID string `json:"depends_on_id"` -} - -// taskPayload carries the local id, because a later update names the task by it. -type taskPayload struct { - ID string `json:"id"` - ItemID string `json:"item_id"` - Title string `json:"title"` -} - -type taskUpdatePayload struct { - ItemID string `json:"item_id"` - Title *string `json:"title,omitempty"` - Completed *bool `json:"completed,omitempty"` - Position *int `json:"position,omitempty"` -} - -type taskRefPayload struct { - ItemID string `json:"item_id"` -} diff --git a/sync/engine.go b/sync/engine.go deleted file mode 100644 index f8a2869..0000000 --- a/sync/engine.go +++ /dev/null @@ -1,276 +0,0 @@ -package sync - -import ( - "context" - "database/sql" - "encoding/json" - "errors" - "fmt" - "log" - "net/http" - "strings" - gosync "sync" - "time" - - "github.com/datapointchris/projects/store/sqlite/generated" -) - -// apiPrefix is the API version every path and decoder here is written against. -const apiPrefix = "/api/v1" - -// Engine orchestrates push and pull synchronization with the remote API. -type Engine struct { - db *sql.DB - q *generated.Queries - client *http.Client - apiURL string - base string - - mu gosync.Mutex - status SyncStatus - syncDepth int - - // drainMu serializes drainPendingOps. It reads the oldest queued op, pushes - // it, and only then deletes it, so two drains at once both read the same op - // and both push it — a 201 followed by a 409 for every create, with the - // server burning an item number on the insert that lost. Two can overlap - // easily: Notify wakes the push loop on the same mutation that a caller then - // flushes for, and the retry ticker fires regardless of either. - drainMu gosync.Mutex - - pushCh chan struct{} - retryInterval time.Duration - ctx context.Context - cancel context.CancelFunc - done chan struct{} -} - -// Option customizes an Engine at construction. -type Option func(*Engine) - -// WithPushRetryInterval overrides how often the push loop re-attempts a queue it -// failed to drain. Tests use it to avoid waiting out the production interval. -func WithPushRetryInterval(d time.Duration) Option { - return func(e *Engine) { e.retryInterval = d } -} - -// New creates a sync engine. Call Start() to launch the background push loop. -// A non-empty tokenCommand is split on spaces and run without a shell, and every -// request carries the bearer token it prints (tokenTransport). Empty sends no -// Authorization header. -func New(db *sql.DB, apiURL, tokenCommand string, opts ...Option) *Engine { - ctx, cancel := context.WithCancel(context.Background()) - client := &http.Client{Timeout: 10 * time.Second} - if argv := strings.Fields(tokenCommand); len(argv) > 0 { - client.Transport = newTokenTransport(argv, http.DefaultTransport) - } - e := &Engine{ - db: db, - q: generated.New(db), - client: client, - apiURL: apiURL, - base: strings.TrimRight(apiURL, "/") + apiPrefix, - pushCh: make(chan struct{}, 1), - retryInterval: defaultPushRetryInterval, - ctx: ctx, - cancel: cancel, - done: make(chan struct{}), - } - for _, opt := range opts { - opt(e) - } - return e -} - -// APIURL returns the configured address of the server this engine syncs with. -func (e *Engine) APIURL() string { return e.apiURL } - -// Start launches the background push loop goroutine. -func (e *Engine) Start() { - go func() { - defer close(e.done) - e.pushLoop() - }() -} - -// Stop cancels the background loop and waits for it to finish. Callers close the -// database right after this returns, so returning early leaves the loop mid-query -// against a closed handle, which fails with "sql: database is closed" on exit. -// The timeout keeps an in-flight HTTP push from hanging exit. -func (e *Engine) Stop() { - e.cancel() - select { - case <-e.done: - case <-time.After(2 * time.Second): - } -} - -// QueueOp inserts a pending sync operation into the database. -func (e *Engine) QueueOp(op OpType, entityID string, payload any) error { - data, err := json.Marshal(payload) - if err != nil { - return fmt.Errorf("marshaling sync payload: %w", err) - } - return e.q.InsertPendingSync(e.ctx, generated.InsertPendingSyncParams{ - Operation: string(op), - EntityType: op.entityType(), - EntityID: entityID, - Payload: string(data), - }) -} - -// DropOpsForEntity removes every queued operation for one entity and reports how -// many were dropped. Used when a local change is reversed: the queued operations -// describe a state that no longer exists, and pushing them would recreate it on -// the server. A non-zero count also means nothing for that entity reached the -// server yet, since operations are deleted once pushed. -func (e *Engine) DropOpsForEntity(entityID string) (int64, error) { - return e.q.DeletePendingSyncByEntity(e.ctx, entityID) -} - -// Notify signals the push loop to wake up and process pending operations. -// Non-blocking: if the push loop is already signaled, this is a no-op. -func (e *Engine) Notify() { - select { - case e.pushCh <- struct{}{}: - default: - } -} - -// Status returns the current sync status (thread-safe). -func (e *Engine) Status() SyncStatus { - e.mu.Lock() - defer e.mu.Unlock() - - count, err := e.q.CountPendingSync(e.ctx) - if err != nil { - return e.status - } - e.status.PendingCount = int(count) - return e.status -} - -func (e *Engine) setStatus(fn func(s *SyncStatus)) { - e.mu.Lock() - defer e.mu.Unlock() - fn(&e.status) -} - -// beginSync/endSync track sync depth rather than a bare flag: Pull calls -// drainPendingOps inside its own window, and a bool would clear on the inner -// call's return, reporting SYNCED while a full pull was still in flight. -func (e *Engine) beginSync() { - e.mu.Lock() - defer e.mu.Unlock() - e.syncDepth++ - e.status.Syncing = true -} - -func (e *Engine) endSync() { - e.mu.Lock() - defer e.mu.Unlock() - e.syncDepth-- - if e.syncDepth <= 0 { - e.syncDepth = 0 - e.status.Syncing = false - } -} - -// defaultPushRetryInterval is how often the push loop re-attempts a queue it -// failed to drain. Notify only fires on a local mutation, so without this a push -// that failed while the API was unreachable stayed queued until the user -// happened to edit something else — the queue never healed on its own. -const defaultPushRetryInterval = 30 * time.Second - -func (e *Engine) pushLoop() { - retry := time.NewTicker(e.retryInterval) - defer retry.Stop() - - for { - select { - case <-e.ctx.Done(): - return - case <-e.pushCh: - e.drainPendingOps() - case <-retry.C: - e.retryPendingOps() - } - } -} - -// retryPendingOps drains only when something is actually queued. An unconditional -// drain would report Connected on an empty queue without having reached the -// network, clearing a pull failure the status bar should still be showing. -func (e *Engine) retryPendingOps() { - count, err := e.q.CountPendingSync(e.ctx) - if err != nil || count == 0 { - return - } - e.drainPendingOps() -} - -func (e *Engine) drainPendingOps() { - e.drainMu.Lock() - defer e.drainMu.Unlock() - - e.beginSync() - defer e.endSync() - - // The push loop runs unattended, so this is the only thing standing between - // a mispointed engine and one database's work landing in another's API. - // Nothing is lost by stopping: the ops stay queued for a correctly pointed - // engine to drain. - if err := e.checkOriginMatch(e.ctx); err != nil { - e.setStatus(func(s *SyncStatus) { - s.Connected = false - s.LastError = err.Error() - }) - return - } - - for { - if e.ctx.Err() != nil { - return - } - - op, err := e.q.GetOldestPendingSync(e.ctx) - if err == sql.ErrNoRows { - e.setStatus(func(s *SyncStatus) { - s.Connected = true - s.LastError = "" - }) - return - } - if err != nil { - // Shutdown cancels the context mid-query. That is the normal exit - // path, not a fault worth printing on every invocation. - if !errors.Is(err, context.Canceled) { - log.Printf("sync: reading pending op: %v", err) - } - return - } - - if err := e.executePush(op); err != nil { - e.setStatus(func(s *SyncStatus) { - s.Connected = false - s.LastError = err.Error() - }) - _ = e.q.UpdatePendingSyncError(e.ctx, generated.UpdatePendingSyncErrorParams{ - LastError: sql.NullString{String: err.Error(), Valid: true}, - ID: op.ID, - }) - // Back off briefly, then exit loop. Will retry on next Notify. - select { - case <-time.After(2 * time.Second): - case <-e.ctx.Done(): - } - return - } - - _ = e.q.DeletePendingSync(e.ctx, op.ID) - e.setStatus(func(s *SyncStatus) { - s.Connected = true - s.LastError = "" - }) - } -} diff --git a/sync/engine_test.go b/sync/engine_test.go deleted file mode 100644 index 6eb0357..0000000 --- a/sync/engine_test.go +++ /dev/null @@ -1,1074 +0,0 @@ -package sync_test - -import ( - "encoding/json" - "net/http" - "net/http/httptest" - "strings" - goSync "sync" - "testing" - "time" - - "github.com/datapointchris/projects/model" - "github.com/datapointchris/projects/service" - "github.com/datapointchris/projects/store/sqlite" - "github.com/datapointchris/projects/sync" -) - -func setupSync(t *testing.T, handler http.Handler) (*sync.SyncBackend, *sync.Engine) { - t.Helper() - database, err := sqlite.Open(":memory:") - if err != nil { - t.Fatalf("opening database: %v", err) - } - t.Cleanup(func() { _ = database.Close() }) - - ts := httptest.NewServer(handler) - t.Cleanup(ts.Close) - - local := service.New(sqlite.New(database)) - engine := sync.New(database, ts.URL, "") - engine.Start() - t.Cleanup(engine.Stop) - - sb := sync.NewSyncBackend(local, engine) - return sb, engine -} - -func TestSyncBackend_ReadsDelegateToLocal(t *testing.T) { - handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) - }) - sb, _ := setupSync(t, handler) - - // Create a project - _, err := sb.CreateProject(model.CreateProject{Name: "LocalRead"}) - if err != nil { - t.Fatalf("CreateProject: %v", err) - } - - // Read should come from local DB immediately - projects, err := sb.ListProjectsByStatus(model.StatusActive) - if err != nil { - t.Fatalf("ListProjectsByStatus: %v", err) - } - if len(projects) != 1 { - t.Fatalf("got %d projects, want 1", len(projects)) - } - if projects[0].Name != "LocalRead" { - t.Errorf("got name %q, want LocalRead", projects[0].Name) - } -} - -func TestSyncBackend_PushRetryOnNetworkError(t *testing.T) { - attempts := 0 - handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - attempts++ - if attempts <= 1 { - w.WriteHeader(http.StatusInternalServerError) - return - } - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) - }) - sb, engine := setupSync(t, handler) - - _, err := sb.CreateProject(model.CreateProject{Name: "Retry"}) - if err != nil { - t.Fatalf("CreateProject: %v", err) - } - - // First push will fail (500), wait for backoff + retry - time.Sleep(4 * time.Second) - - // Manually trigger a retry - engine.Notify() - time.Sleep(500 * time.Millisecond) - - status := engine.Status() - if status.PendingCount != 0 { - t.Errorf("expected 0 pending after retry, got %d", status.PendingCount) - } -} - -func TestSyncBackend_PushDropsOn409(t *testing.T) { - handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusConflict) - _, _ = w.Write([]byte(`{"detail":"duplicate"}`)) - }) - sb, engine := setupSync(t, handler) - - _, err := sb.CreateProject(model.CreateProject{Name: "Conflict"}) - if err != nil { - t.Fatalf("CreateProject: %v", err) - } - - // 409 should be treated as "drop" — pending count goes to 0 - time.Sleep(200 * time.Millisecond) - - status := engine.Status() - if status.PendingCount != 0 { - t.Errorf("expected 0 pending (409 should drop), got %d", status.PendingCount) - } -} - -// callRecorder captures what a test's mock server received. The push loop -// runs on its own goroutine, so every read and write of the log needs the -// lock — asserting on an unguarded slice is a data race. -type callRecorder struct { - mu goSync.Mutex - calls []string -} - -func (r *callRecorder) handler() http.HandlerFunc { - return func(w http.ResponseWriter, req *http.Request) { - r.mu.Lock() - r.calls = append(r.calls, req.Method+" "+req.URL.Path) - r.mu.Unlock() - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) - } -} - -func (r *callRecorder) snapshot() []string { - r.mu.Lock() - defer r.mu.Unlock() - return append([]string(nil), r.calls...) -} - -func (r *callRecorder) count() int { - r.mu.Lock() - defer r.mu.Unlock() - return len(r.calls) -} - -func TestSyncBackend_ItemLifecycle(t *testing.T) { - recorder := &callRecorder{} - sb, _ := setupSync(t, recorder.handler()) - - p, _ := sb.CreateProject(model.CreateProject{Name: "Items"}) - item, err := sb.CreateItem(model.CreateProjectItem{ - Title: "Test", - ProjectIDs: []string{p.ID}, - }) - if err != nil { - t.Fatalf("CreateItem: %v", err) - } - - done := true - _, err = sb.UpdateItem(item.ID, model.UpdateProjectItem{Completed: &done}) - if err != nil { - t.Fatalf("UpdateItem: %v", err) - } - - if err := sb.DeleteItem(item.ID); err != nil { - t.Fatalf("DeleteItem: %v", err) - } - - // Wait for push loop to drain - time.Sleep(500 * time.Millisecond) - - // Verify the HTTP calls were made in order - if recorder.count() < 4 { - t.Fatalf("expected at least 4 HTTP calls, got %d: %v", recorder.count(), recorder.snapshot()) - } -} - -func TestSyncBackend_TaskOperations(t *testing.T) { - recorder := &callRecorder{} - sb, _ := setupSync(t, recorder.handler()) - - p, _ := sb.CreateProject(model.CreateProject{Name: "Tasks"}) - item, _ := sb.CreateItem(model.CreateProjectItem{ - Title: "WithTasks", - ProjectIDs: []string{p.ID}, - }) - - task, err := sb.CreateTask(item.ID, model.CreateProjectItemTask{Title: "Sub"}) - if err != nil { - t.Fatalf("CreateTask: %v", err) - } - - done := true - if _, err := sb.UpdateTask(item.ID, task.ID, model.UpdateProjectItemTask{Completed: &done}); err != nil { - t.Fatalf("UpdateTask: %v", err) - } - - if err := sb.DeleteTask(item.ID, task.ID); err != nil { - t.Fatalf("DeleteTask: %v", err) - } - - time.Sleep(500 * time.Millisecond) - - if recorder.count() < 5 { - t.Fatalf("expected at least 5 HTTP calls, got %d: %v", recorder.count(), recorder.snapshot()) - } -} - -// A person adds a task and checks it off before the next pull. The check names the -// local id, so the server has to hold the task under it, or a 404 drops the check and -// the pull brings the task back unchecked. -func TestATaskCheckedOffBeforeThePullReachesTheServerUnderItsOwnID(t *testing.T) { - createdAs := make(chan string, 8) - recorder := &callRecorder{} - sb, _ := setupSync(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/tasks") { - var body struct { - ID string `json:"id"` - } - _ = json.NewDecoder(r.Body).Decode(&body) - createdAs <- body.ID - } - recorder.handler()(w, r) - })) - - p, _ := sb.CreateProject(model.CreateProject{Name: "Tasks"}) - item, _ := sb.CreateItem(model.CreateProjectItem{Title: "WithTasks", ProjectIDs: []string{p.ID}}) - task, err := sb.CreateTask(item.ID, model.CreateProjectItemTask{Title: "Sub"}) - if err != nil { - t.Fatalf("CreateTask: %v", err) - } - - select { - case got := <-createdAs: - if got != task.ID { - t.Errorf("the server was asked to create task %q, and the TUI holds it as %q", got, task.ID) - } - case <-time.After(5 * time.Second): - t.Fatal("the task's create never reached the server") - } -} - -func TestSyncBackend_UndoAfterPushDeletesRemotely(t *testing.T) { - recorder := &callRecorder{} - sb, _ := setupSync(t, recorder.handler()) - - p, _ := sb.CreateProject(model.CreateProject{Name: "Undo"}) - item, _ := sb.CreateItem(model.CreateProjectItem{ - Title: "Undoable", - ProjectIDs: []string{p.ID}, - }) - - // Let the create reach the server, so the server holds a row the undo removes. - time.Sleep(300 * time.Millisecond) - - if _, err := sb.Undo(); err != nil { - t.Fatalf("Undo: %v", err) - } - time.Sleep(300 * time.Millisecond) - - want := "DELETE /api/v1/project-items/" + item.ID - calls := recorder.snapshot() - for _, call := range calls { - if call == want { - return - } - } - t.Errorf("expected %q after undoing an already-pushed create, got %v", want, calls) -} - -func TestSyncBackend_UndoBeforePushDropsQueuedCreate(t *testing.T) { - // Every push fails, so operations stay queued and nothing reaches the server. - handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - }) - sb, engine := setupSync(t, handler) - - p, _ := sb.CreateProject(model.CreateProject{Name: "Undo"}) - item, _ := sb.CreateItem(model.CreateProjectItem{ - Title: "Undoable", - ProjectIDs: []string{p.ID}, - }) - - if _, err := sb.Undo(); err != nil { - t.Fatalf("Undo: %v", err) - } - - // The queued create described a row that no longer exists. Nothing should be - // left for this item — not the create, and not a delete for a row the server - // never received. - remaining, err := engine.DropOpsForEntity(item.ID) - if err != nil { - t.Fatalf("checking queue: %v", err) - } - if remaining != 0 { - t.Errorf("expected no queued operations for the undone item, found %d", remaining) - } -} - -func TestPull_Reconciles(t *testing.T) { - // Mock API server that returns test data - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/api/v1/projects": - _ = json.NewEncoder(w).Encode([]model.Project{ - {ID: "proj-1", Name: "Pulled", Position: 0, CreatedAt: time.Now()}, - }) - case "/api/v1/project-items": - _ = json.NewEncoder(w).Encode([]model.ProjectItem{ - {ID: "01900000-0000-7000-8000-00000000a001", Title: "Pulled Item", CreatedAt: time.Now(), UpdatedAt: time.Now()}, - }) - case "/api/v1/project-items/01900000-0000-7000-8000-00000000a001": - _ = json.NewEncoder(w).Encode(model.ProjectItemDetail{ - ProjectItem: model.ProjectItem{ID: "01900000-0000-7000-8000-00000000a001", Title: "Pulled Item", CreatedAt: time.Now(), UpdatedAt: time.Now()}, - Projects: []model.Project{{ID: "proj-1", Name: "Pulled"}}, - }) - case "/api/v1/project-items/01900000-0000-7000-8000-00000000a001/tasks": - _ = json.NewEncoder(w).Encode([]model.ProjectItemTask{ - {ID: "task-1", ItemID: "01900000-0000-7000-8000-00000000a001", Title: "Pulled Task", CreatedAt: time.Now()}, - }) - default: - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) - } - }) - - database, err := sqlite.Open(":memory:") - if err != nil { - t.Fatalf("opening database: %v", err) - } - defer func() { _ = database.Close() }() - - ts := httptest.NewServer(handler) - defer ts.Close() - - engine := sync.New(database, ts.URL, "") - - // Pull should reconcile server data into local DB - if err := engine.Pull(t.Context()); err != nil { - t.Fatalf("Pull: %v", err) - } - - // Verify data was pulled into local DB - local := service.New(sqlite.New(database)) - - projects, err := local.ListProjectsByStatus(model.StatusActive) - if err != nil { - t.Fatalf("ListProjectsByStatus: %v", err) - } - if len(projects) != 1 || projects[0].Name != "Pulled" { - t.Errorf("expected 1 project 'Pulled', got %v", projects) - } - - item, err := local.GetItem("01900000-0000-7000-8000-00000000a001") - if err != nil { - t.Fatalf("GetItem: %v", err) - } - if item.Title != "Pulled Item" { - t.Errorf("got title %q, want 'Pulled Item'", item.Title) - } - - tasks, err := local.ListTasks("01900000-0000-7000-8000-00000000a001") - if err != nil { - t.Fatalf("ListTasks: %v", err) - } - if len(tasks) != 1 || tasks[0].Title != "Pulled Task" { - t.Errorf("expected 1 task 'Pulled Task', got %v", tasks) - } -} - -// Undoing an already-pushed delete has to recreate the row on the server, not -// update one the server no longer has — and recreate its memberships, tasks, -// and dependencies with it, because a pull rebuilds those from server state -// and would otherwise erase the restore. -func TestSyncBackend_UndoOfPushedDeleteRecreatesRemotely(t *testing.T) { - recorder := &callRecorder{} - sb, _ := setupSync(t, recorder.handler()) - - p, _ := sb.CreateProject(model.CreateProject{Name: "Undo"}) - item, _ := sb.CreateItem(model.CreateProjectItem{ - Title: "Doomed", - ProjectIDs: []string{p.ID}, - }) - if _, err := sb.CreateTask(item.ID, model.CreateProjectItemTask{Title: "step"}); err != nil { - t.Fatalf("CreateTask: %v", err) - } - if err := sb.DeleteItem(item.ID); err != nil { - t.Fatalf("DeleteItem: %v", err) - } - - // Let the delete reach the server before undoing it, then look only at - // what the undo itself pushed — the original create hit the same paths. - time.Sleep(400 * time.Millisecond) - beforeUndo := recorder.count() - - if _, err := sb.Undo(); err != nil { - t.Fatalf("Undo: %v", err) - } - time.Sleep(400 * time.Millisecond) - - calls := recorder.snapshot()[beforeUndo:] - - var sawItemCreate, sawTaskCreate bool - for _, call := range calls { - switch call { - case "POST /api/v1/project-items": - sawItemCreate = true - case "POST /api/v1/project-items/" + item.ID + "/tasks": - sawTaskCreate = true - } - } - if !sawItemCreate { - t.Errorf("expected the restored item recreated on the server, got %v", calls) - } - if !sawTaskCreate { - t.Errorf("expected the restored sub-task recreated on the server, got %v", calls) - } -} - -// Membership, dependency, and reorder undos all record the item as their -// entity ID. Reconciling them by dropping queued ops for that ID would take -// unrelated edits with it, and falling through to the item branch would push -// a delete for an item that is still there. -func TestSyncBackend_UndoOfMembershipDoesNotDeleteTheItem(t *testing.T) { - recorder := &callRecorder{} - sb, _ := setupSync(t, recorder.handler()) - - work, _ := sb.CreateProject(model.CreateProject{Name: "work"}) - personal, _ := sb.CreateProject(model.CreateProject{Name: "personal"}) - item, _ := sb.CreateItem(model.CreateProjectItem{ - Title: "shared", - ProjectIDs: []string{work.ID}, - }) - if err := sb.AddToProject(item.ID, personal.ID); err != nil { - t.Fatalf("AddToProject: %v", err) - } - - time.Sleep(400 * time.Millisecond) - beforeUndo := recorder.count() - - if _, err := sb.Undo(); err != nil { - t.Fatalf("Undo: %v", err) - } - time.Sleep(400 * time.Millisecond) - - calls := recorder.snapshot()[beforeUndo:] - wantRemove := "DELETE /api/v1/project-items/" + item.ID + "/projects/" + personal.ID - itemDelete := "DELETE /api/v1/project-items/" + item.ID - - var sawRemove bool - for _, call := range calls { - if call == itemDelete { - t.Fatalf("undoing a membership add deleted the item on the server: %v", calls) - } - if call == wantRemove { - sawRemove = true - } - } - if !sawRemove { - t.Errorf("expected %q after undoing the membership add, got %v", wantRemove, calls) - } -} - -func TestSyncBackend_UndoOfTaskCompletionPushesTheReversal(t *testing.T) { - recorder := &callRecorder{} - sb, _ := setupSync(t, recorder.handler()) - - p, _ := sb.CreateProject(model.CreateProject{Name: "work"}) - item, _ := sb.CreateItem(model.CreateProjectItem{Title: "item", ProjectIDs: []string{p.ID}}) - task, err := sb.CreateTask(item.ID, model.CreateProjectItemTask{Title: "step"}) - if err != nil { - t.Fatalf("CreateTask: %v", err) - } - done := true - if _, err := sb.UpdateTask(item.ID, task.ID, model.UpdateProjectItemTask{Completed: &done}); err != nil { - t.Fatalf("UpdateTask: %v", err) - } - - time.Sleep(400 * time.Millisecond) - beforeUndo := recorder.count() - - if _, err := sb.Undo(); err != nil { - t.Fatalf("Undo: %v", err) - } - time.Sleep(400 * time.Millisecond) - - calls := recorder.snapshot()[beforeUndo:] - want := "PATCH /api/v1/project-items/" + item.ID + "/tasks/" + task.ID - for _, call := range calls { - if call == want { - return - } - } - t.Errorf("expected %q after undoing the completion, got %v", want, calls) -} - -// logging wraps a handler so a test can count requests without reimplementing -// the routing. -func logging(recorder *callRecorder, next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - recorder.mu.Lock() - recorder.calls = append(recorder.calls, r.Method+" "+r.URL.Path) - recorder.mu.Unlock() - next.ServeHTTP(w, r) - }) -} - -// A pull takes seconds — 2+2N requests — and the user keeps working through it. -// Anything created in that window postdates the server response, so the -// reconciliation must not read it as "deleted upstream" and erase it, nor drop -// the queued operation that is the only record of it. -func TestPull_KeepsWorkDoneWhileItWasRunning(t *testing.T) { - database, err := sqlite.Open(":memory:") - if err != nil { - t.Fatalf("opening database: %v", err) - } - t.Cleanup(func() { _ = database.Close() }) - - local := service.New(sqlite.New(database)) - - // The mid-pull create needs a project to attach to, and the server has to - // report that same project or the pull deletes it and cascades the item away. - project, err := local.CreateProject(model.CreateProject{Name: "Server"}) - if err != nil { - t.Fatalf("seeding project: %v", err) - } - - var sb *sync.SyncBackend - var once goSync.Once - raced := make(chan string, 1) - - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Every write fails, so the op queued below is guaranteed to still be - // pending when the assertion runs. - if r.Method != http.MethodGet { - w.WriteHeader(http.StatusInternalServerError) - return - } - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/api/v1/projects": - _ = json.NewEncoder(w).Encode([]model.Project{ - {ID: project.ID, Name: "Server", Position: 0, CreatedAt: time.Now()}, - }) - case "/api/v1/project-items": - _ = json.NewEncoder(w).Encode([]model.ProjectItem{ - {ID: "01900000-0000-7000-8000-00000000a001", Title: "Server Item", CreatedAt: time.Now(), UpdatedAt: time.Now()}, - }) - case "/api/v1/project-items/01900000-0000-7000-8000-00000000a001": - // Mid-pull: the user adds an item the server response cannot know about. - once.Do(func() { - created, err := sb.CreateItem(model.CreateProjectItem{ - Title: "Typed During Pull", - ProjectIDs: []string{project.ID}, - }) - if err != nil { - t.Errorf("creating item mid-pull: %v", err) - return - } - raced <- created.ID - }) - _ = json.NewEncoder(w).Encode(model.ProjectItemDetail{ - ProjectItem: model.ProjectItem{ID: "01900000-0000-7000-8000-00000000a001", Title: "Server Item", CreatedAt: time.Now(), UpdatedAt: time.Now()}, - Projects: []model.Project{{ID: project.ID, Name: "Server"}}, - }) - default: - _ = json.NewEncoder(w).Encode([]model.ProjectItemTask{}) - } - }) - - ts := httptest.NewServer(handler) - t.Cleanup(ts.Close) - - engine := sync.New(database, ts.URL, "") - t.Cleanup(engine.Stop) - sb = sync.NewSyncBackend(local, engine) - - if err := engine.Pull(t.Context()); err != nil { - t.Fatalf("Pull: %v", err) - } - - racedID := <-raced - if _, err := local.GetItem(racedID); err != nil { - t.Fatalf("item created during the pull was erased by it: %v", err) - } - if got := engine.Status().PendingCount; got == 0 { - t.Error("the queued create was dropped, so nothing will ever push it") - } -} - -// Notify only fires on a local mutation. Without a retry ticker a push that -// failed while the API was down stayed queued until the user happened to edit -// something else, which is indistinguishable from sync being broken. -func TestPush_RetriesWithoutAFurtherMutation(t *testing.T) { - var mu goSync.Mutex - failing := true - handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - mu.Lock() - down := failing - mu.Unlock() - if down { - w.WriteHeader(http.StatusInternalServerError) - return - } - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) - }) - - database, err := sqlite.Open(":memory:") - if err != nil { - t.Fatalf("opening database: %v", err) - } - t.Cleanup(func() { _ = database.Close() }) - - ts := httptest.NewServer(handler) - t.Cleanup(ts.Close) - - engine := sync.New(database, ts.URL, "", sync.WithPushRetryInterval(100*time.Millisecond)) - engine.Start() - t.Cleanup(engine.Stop) - sb := sync.NewSyncBackend(service.New(sqlite.New(database)), engine) - - if _, err := sb.CreateProject(model.CreateProject{Name: "Queued"}); err != nil { - t.Fatalf("CreateProject: %v", err) - } - - // The first push fails and backs off; the op must still be waiting. - time.Sleep(500 * time.Millisecond) - if engine.Status().PendingCount == 0 { - t.Fatal("op vanished while the API was failing") - } - - mu.Lock() - failing = false - mu.Unlock() - - // No Notify, no further mutation — only the ticker can clear this. - deadline := time.Now().Add(5 * time.Second) - for time.Now().Before(deadline) { - if engine.Status().PendingCount == 0 { - return - } - time.Sleep(50 * time.Millisecond) - } - t.Error("queue never healed on its own after the API recovered") -} - -// pullFixture serves a two-item dataset. When embedded is true the list carries -// dependency_ids and tasks the way the current API does; when false it omits -// them the way a server predating the embed does, which is what forces the -// per-item fallback. -func pullFixture(embedded bool) http.HandlerFunc { - now := time.Now() - base := func(id, title string) map[string]any { - return map[string]any{ - "id": id, "title": title, "notes": nil, "repo": nil, - "completed": false, "archived": false, - "created_at": now.Format(time.RFC3339Nano), - "updated_at": now.Format(time.RFC3339Nano), - "projects": []map[string]any{{"id": "proj-1", "name": "Remote", "position": 0, "created_at": now.Format(time.RFC3339Nano)}}, - } - } - task := map[string]any{ - "id": "task-1", "item_id": "01900000-0000-7000-8000-00000000a002", "title": "Embedded Task", - "completed": false, "position": 0, "created_at": now.Format(time.RFC3339Nano), - } - - return func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/api/v1/projects": - _ = json.NewEncoder(w).Encode([]model.Project{{ID: "proj-1", Name: "Remote", Position: 0, CreatedAt: now}}) - case "/api/v1/project-items": - blocker := base("01900000-0000-7000-8000-00000000a001", "Blocker") - blocked := base("01900000-0000-7000-8000-00000000a002", "Blocked") - if embedded { - blocker["dependency_ids"] = []string{} - blocker["tasks"] = []map[string]any{} - blocked["dependency_ids"] = []string{"01900000-0000-7000-8000-00000000a001"} - blocked["tasks"] = []map[string]any{task} - } - _ = json.NewEncoder(w).Encode([]map[string]any{blocker, blocked}) - case "/api/v1/project-items/01900000-0000-7000-8000-00000000a001": - _ = json.NewEncoder(w).Encode(base("01900000-0000-7000-8000-00000000a001", "Blocker")) - case "/api/v1/project-items/01900000-0000-7000-8000-00000000a002": - detail := base("01900000-0000-7000-8000-00000000a002", "Blocked") - detail["dependency_ids"] = []string{"01900000-0000-7000-8000-00000000a001"} - _ = json.NewEncoder(w).Encode(detail) - case "/api/v1/project-items/01900000-0000-7000-8000-00000000a002/tasks": - _ = json.NewEncoder(w).Encode([]map[string]any{task}) - default: - _ = json.NewEncoder(w).Encode([]map[string]any{}) - } - } -} - -func pulledInto(t *testing.T, handler http.HandlerFunc) (*callRecorder, *service.Service) { - t.Helper() - database, err := sqlite.Open(":memory:") - if err != nil { - t.Fatalf("opening database: %v", err) - } - t.Cleanup(func() { _ = database.Close() }) - - recorder := &callRecorder{} - ts := httptest.NewServer(logging(recorder, handler)) - t.Cleanup(ts.Close) - - engine := sync.New(database, ts.URL, "") - t.Cleanup(engine.Stop) - if err := engine.Pull(t.Context()); err != nil { - t.Fatalf("Pull: %v", err) - } - return recorder, service.New(sqlite.New(database)) -} - -// The whole point of embedding: a reconcile is two requests regardless of how -// many items exist, instead of two more per item. -func TestPull_UsesEmbeddedDetailAndStopsAtTwoRequests(t *testing.T) { - recorder, local := pulledInto(t, pullFixture(true)) - - if got := recorder.snapshot(); len(got) != 2 { - t.Errorf("expected 2 requests, got %d: %v", len(got), got) - } - - tasks, err := local.ListTasks("01900000-0000-7000-8000-00000000a002") - if err != nil { - t.Fatalf("ListTasks: %v", err) - } - if len(tasks) != 1 || tasks[0].Title != "Embedded Task" { - t.Errorf("embedded task did not reconcile: %v", tasks) - } - blockers, err := local.GetBlockers("01900000-0000-7000-8000-00000000a002") - if err != nil { - t.Fatalf("GetBlockers: %v", err) - } - if len(blockers) != 1 || blockers[0].ID != "01900000-0000-7000-8000-00000000a001" { - t.Errorf("embedded dependency did not reconcile: %v", blockers) - } -} - -// Deploy order between this tool and its server must not matter. Against a server -// that omits the fields, treating absent as empty would delete every task and -// dependency locally, so the pull has to fall back to the per-item endpoints. -func TestPull_FallsBackWhenTheServerOmitsEmbeddedFields(t *testing.T) { - recorder, local := pulledInto(t, pullFixture(false)) - - if got := recorder.count(); got <= 2 { - t.Errorf("expected per-item fallback requests, got only %d", got) - } - - tasks, err := local.ListTasks("01900000-0000-7000-8000-00000000a002") - if err != nil { - t.Fatalf("ListTasks: %v", err) - } - if len(tasks) != 1 || tasks[0].Title != "Embedded Task" { - t.Errorf("fallback lost the item's tasks: %v", tasks) - } - blockers, err := local.GetBlockers("01900000-0000-7000-8000-00000000a002") - if err != nil { - t.Fatalf("GetBlockers: %v", err) - } - if len(blockers) != 1 || blockers[0].ID != "01900000-0000-7000-8000-00000000a001" { - t.Errorf("fallback lost the item's dependencies: %v", blockers) - } -} - -// pendingAfterPush waits for the push loop a mutation's Notify woke, and returns -// how many ops are still queued once the queue empties or the wait runs out. -func pendingAfterPush(engine *sync.Engine) int { - deadline := time.Now().Add(2 * time.Second) - for { - pending := engine.Status().PendingCount - if pending == 0 || time.Now().After(deadline) { - return pending - } - time.Sleep(10 * time.Millisecond) - } -} - -// The number is the item's handle in the TUI, and the create response is the -// first place it exists. Waiting for the next pull would leave the item unnamed -// for up to a full sync interval after it was made. -func TestPush_RecordsTheNumberTheServerAssigned(t *testing.T) { - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodPost && r.URL.Path == "/api/v1/project-items" { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"number": 231}`)) - return - } - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) - }) - sb, engine := setupSync(t, handler) - - p, _ := sb.CreateProject(model.CreateProject{Name: "Numbered"}) - item, err := sb.CreateItem(model.CreateProjectItem{Title: "Earns a number", ProjectIDs: []string{p.ID}}) - if err != nil { - t.Fatalf("CreateItem: %v", err) - } - if item.Number != nil { - t.Fatalf("Number = %d before the push, want nil", *item.Number) - } - - if pending := pendingAfterPush(engine); pending != 0 { - t.Fatalf("pending ops = %d, want the create pushed", pending) - } - - pushed, err := sb.GetItem(item.ID) - if err != nil { - t.Fatalf("GetItem: %v", err) - } - if pushed.Number == nil { - t.Fatal("Number is still nil after the push — the create response was discarded") - } - if *pushed.Number != 231 { - t.Errorf("Number = %d, want 231", *pushed.Number) - } -} - -// A create that reached the server is applied whether or not its response -// decodes. Failing here would re-queue it and file the item twice. -func TestPush_AnUndecodableCreateResponseIsNotAFailure(t *testing.T) { - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodPost && r.URL.Path == "/api/v1/project-items" { - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte("not json")) - return - } - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) - }) - sb, engine := setupSync(t, handler) - - p, _ := sb.CreateProject(model.CreateProject{Name: "Garbled"}) - item, err := sb.CreateItem(model.CreateProjectItem{Title: "Still filed once", ProjectIDs: []string{p.ID}}) - if err != nil { - t.Fatalf("CreateItem: %v", err) - } - - if pending := pendingAfterPush(engine); pending != 0 { - t.Errorf("pending ops = %d, want 0 — the create landed and must not be retried", pending) - } - stored, err := sb.GetItem(item.ID) - if err != nil { - t.Fatalf("GetItem: %v", err) - } - if stored.Number != nil { - t.Errorf("Number = %d, want nil — the next pull supplies it", *stored.Number) - } -} - -// Two drains overlapping pushed the same queued op twice — a 201 followed by a -// 409, and a server-side item number burned on the insert that lost the race. -// Notify wakes the push loop on a create, and a pull drains the queue before it -// fetches, so a create landing just before a pull is drained from both sides. -func TestPush_AQueuedCreateIsPushedExactlyOnce(t *testing.T) { - var mu goSync.Mutex - creates := 0 - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodPost && r.URL.Path == "/api/v1/project-items" { - mu.Lock() - creates++ - mu.Unlock() - time.Sleep(20 * time.Millisecond) - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte(`{"number": 9}`)) - return - } - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) - }) - sb, engine := setupSync(t, handler) - - p, _ := sb.CreateProject(model.CreateProject{Name: "Once"}) - if _, err := sb.CreateItem(model.CreateProjectItem{Title: "Pushed once", ProjectIDs: []string{p.ID}}); err != nil { - t.Fatalf("CreateItem: %v", err) - } - - // The create already woke the push loop. The pull's own drain races one - // that is very likely still in flight, and its fetch then fails on the - // handler's "{}", which this test does not care about. - _ = engine.Pull(t.Context()) - time.Sleep(300 * time.Millisecond) - - mu.Lock() - defer mu.Unlock() - if creates != 1 { - t.Errorf("POST /api/v1/project-items called %d times, want exactly 1", creates) - } -} - -// The server assembles /projects/ and each item's embedded Projects list -// separately, and nothing makes the two agree, so an item can name a project -// the projects endpoint omitted. The pull drops that link and carries on. -// Inserting it would fail the pull on the foreign key, and one inconsistent -// link would cost every other change in the reconcile. -func TestPull_DropsLinksToEntitiesTheServerDidNotSend(t *testing.T) { - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/api/v1/projects": - _ = json.NewEncoder(w).Encode([]model.Project{ - {ID: "proj-1", Name: "Real", Position: 0, CreatedAt: time.Now()}, - }) - case "/api/v1/project-items": - _ = json.NewEncoder(w).Encode([]model.ProjectItemDetail{{ - ProjectItem: model.ProjectItem{ID: "01900000-0000-7000-8000-00000000a001", Title: "Kept", CreatedAt: time.Now(), UpdatedAt: time.Now()}, - Projects: []model.Project{ - {ID: "proj-1", Name: "Real"}, - {ID: "proj-missing", Name: "Never sent by /projects/"}, - }, - DependencyIDs: []string{"item-missing"}, - Tasks: []model.ProjectItemTask{ - {ID: "task-1", ItemID: "01900000-0000-7000-8000-00000000a001", Title: "Kept task", CreatedAt: time.Now()}, - }, - }}) - default: - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) - } - }) - - database, err := sqlite.Open(":memory:") - if err != nil { - t.Fatalf("opening database: %v", err) - } - defer func() { _ = database.Close() }() - - ts := httptest.NewServer(handler) - defer ts.Close() - - engine := sync.New(database, ts.URL, "") - if err := engine.Pull(t.Context()); err != nil { - t.Fatalf("one unlinkable membership failed the whole pull: %v", err) - } - - local := service.New(sqlite.New(database)) - projects, err := local.GetItemProjects("01900000-0000-7000-8000-00000000a001") - if err != nil { - t.Fatalf("GetItemProjects: %v", err) - } - if len(projects) != 1 || projects[0].ID != "proj-1" { - t.Errorf("expected only the membership that has a project, got %v", projects) - } - - var violations int - if err := database.QueryRow("SELECT COUNT(*) FROM pragma_foreign_key_check").Scan(&violations); err != nil { - t.Fatalf("checking foreign keys: %v", err) - } - if violations != 0 { - t.Errorf("the pull left %d dangling rows behind", violations) - } - - tasks, err := local.ListTasks("01900000-0000-7000-8000-00000000a001") - if err != nil { - t.Fatalf("ListTasks: %v", err) - } - if len(tasks) != 1 { - t.Errorf("the good task went with the bad link: %v", tasks) - } -} - -// idx_items_number is unique and the upsert writes row by row, so two items -// swapping numbers upstream means one of them briefly needs a number the other -// still holds. Writing the numbers after every row has been cleared makes the -// pass order-independent — otherwise any renumbering upstream fails the entire -// pull and the tool never syncs again. -func TestPull_SurvivesItemsSwappingNumbers(t *testing.T) { - numbers := map[string]int{"01900000-0000-7000-8000-00000000a00a": 1, "01900000-0000-7000-8000-00000000a00b": 2} - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - if r.URL.Path != "/api/v1/project-items" { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("[]")) - return - } - out := make([]model.ProjectItemDetail, 0, 2) - for _, id := range []string{"01900000-0000-7000-8000-00000000a00a", "01900000-0000-7000-8000-00000000a00b"} { - n := numbers[id] - out = append(out, model.ProjectItemDetail{ - ProjectItem: model.ProjectItem{ - ID: id, Number: &n, Title: id, - CreatedAt: time.Now(), UpdatedAt: time.Now(), - }, - DependencyIDs: []string{}, - Tasks: []model.ProjectItemTask{}, - }) - } - _ = json.NewEncoder(w).Encode(out) - }) - - database, err := sqlite.Open(":memory:") - if err != nil { - t.Fatalf("opening database: %v", err) - } - defer func() { _ = database.Close() }() - - ts := httptest.NewServer(handler) - defer ts.Close() - - engine := sync.New(database, ts.URL, "") - if err := engine.Pull(t.Context()); err != nil { - t.Fatalf("first pull: %v", err) - } - - numbers["01900000-0000-7000-8000-00000000a00a"], numbers["01900000-0000-7000-8000-00000000a00b"] = 2, 1 - if err := engine.Pull(t.Context()); err != nil { - t.Fatalf("a swap upstream failed the pull: %v", err) - } - - local := service.New(sqlite.New(database)) - item, err := local.GetItem("01900000-0000-7000-8000-00000000a00a") - if err != nil { - t.Fatalf("GetItem: %v", err) - } - if item.Number == nil || *item.Number != 2 { - t.Errorf("01900000-0000-7000-8000-00000000a00a number = %v, want 2", item.Number) - } -} - -// Both list fetches ask for every status, because the sweep in Pull deletes any -// local row the server did not return. An unqualified fetch is therefore not a -// narrower read — it is a delete. /project-items/ defaults to open items, so an -// unqualified fetch would delete every completed item here. -func TestPull_RequestsEveryStatusFromBothListEndpoints(t *testing.T) { - var mu goSync.Mutex - asked := map[string]string{} - record := func(r *http.Request) { - mu.Lock() - defer mu.Unlock() - asked[r.URL.Path] = r.URL.Query().Get("status") - } - - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/api/v1/projects": - record(r) - _ = json.NewEncoder(w).Encode([]model.Project{}) - case "/api/v1/project-items": - record(r) - _ = json.NewEncoder(w).Encode([]model.ProjectItemDetail{}) - default: - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("{}")) - } - }) - - database, err := sqlite.Open(":memory:") - if err != nil { - t.Fatalf("opening database: %v", err) - } - defer func() { _ = database.Close() }() - - ts := httptest.NewServer(handler) - defer ts.Close() - - engine := sync.New(database, ts.URL, "") - if err := engine.Pull(t.Context()); err != nil { - t.Fatalf("pull: %v", err) - } - - mu.Lock() - defer mu.Unlock() - for _, path := range []string{"/api/v1/projects", "/api/v1/project-items"} { - if asked[path] != "all" { - t.Errorf("%s fetched with status=%q, want all — the sweep deletes whatever the server omits", path, asked[path]) - } - } -} diff --git a/sync/operations.go b/sync/operations.go deleted file mode 100644 index 46d9d92..0000000 --- a/sync/operations.go +++ /dev/null @@ -1,41 +0,0 @@ -package sync - -// OpType identifies the kind of sync operation queued in pending_sync. -type OpType string - -const ( - OpCreateProject OpType = "create_project" - OpUpdateProject OpType = "update_project" - OpDeleteProject OpType = "delete_project" - OpReorderProject OpType = "reorder_project" - OpCreateItem OpType = "create_item" - OpUpdateItem OpType = "update_item" - OpDeleteItem OpType = "delete_item" - OpReorderItem OpType = "reorder_item" - OpAddToProject OpType = "add_to_project" - OpRemoveFromProject OpType = "remove_from_project" - OpCreateTask OpType = "create_task" - OpUpdateTask OpType = "update_task" - OpDeleteTask OpType = "delete_task" - OpCompleteTask OpType = "complete_task" - OpAddDependency OpType = "add_dependency" - OpRemoveDependency OpType = "remove_dependency" -) - -// entityType returns the entity category for the pending_sync table. -func (o OpType) entityType() string { - switch o { - case OpCreateProject, OpUpdateProject, OpDeleteProject, OpReorderProject: - return "project" - case OpCreateItem, OpUpdateItem, OpDeleteItem, OpReorderItem: - return "item" - case OpAddToProject, OpRemoveFromProject: - return "membership" - case OpCreateTask, OpUpdateTask, OpDeleteTask, OpCompleteTask: - return "task" - case OpAddDependency, OpRemoveDependency: - return "dependency" - default: - return "unknown" - } -} diff --git a/sync/origin.go b/sync/origin.go deleted file mode 100644 index 351858e..0000000 --- a/sync/origin.go +++ /dev/null @@ -1,171 +0,0 @@ -package sync - -import ( - "context" - "database/sql" - "errors" - "fmt" - "strings" -) - -// A pull deletes every local row the server did not return, which makes a -// database and the API it reconciles against a pair. Point one at the other -// one and the pull is not a sync but a wipe: it faithfully deletes everything -// the other API has never heard of. That is not a hypothetical — a production -// database reconciled against a development API lost 42 projects and 286 items -// in a single command that reported success. -// -// Nothing in a database says which API it belongs to, so nothing could refuse. -// The pairing is recorded on first pull and checked before every push and pull -// after that. - -// OriginMismatch is returned when a database is asked to reconcile against an -// API other than the one it was adopted by. -type OriginMismatch struct { - Recorded string - Configured string -} - -func (e *OriginMismatch) Error() string { - return fmt.Sprintf( - "this database belongs to %s but sync is configured for %s — "+ - "reconciling would delete everything the other API has never seen. "+ - "Run `projects sync --adopt` if the move is deliberate", - e.Recorded, e.Configured) -} - -// normalizeAPIURL makes the comparison indifferent to the things that do not -// change which server is being addressed. -func normalizeAPIURL(url string) string { - return strings.TrimRight(strings.TrimSpace(url), "/") -} - -// OriginUnclaimed is returned when a database that has reconciled before has no -// record of which API it did that with. It was bound to something; adopting it -// silently to whatever is configured now is exactly the step that turns a -// misconfiguration into a wipe, so it is the user who says. -type OriginUnclaimed struct { - Configured string - Projects int - Items int -} - -func (e *OriginUnclaimed) Error() string { - return fmt.Sprintf( - "this database holds %d projects and %d items and has synced before, but has no record of "+ - "which API it belongs to — reconciling against %s would delete everything that API has "+ - "never seen. Run `projects sync --adopt` to bind it", - e.Projects, e.Items, e.Configured) -} - -// checkOriginMatch refuses only an outright mismatch. It is what the push path -// asks, because a push does not destroy local data: it only has to be stopped -// from delivering one database's work to another's API. -func (e *Engine) checkOriginMatch(ctx context.Context) error { - recorded, err := e.q.GetSyncOrigin(ctx) - if errors.Is(err, sql.ErrNoRows) { - return nil - } - if err != nil { - return fmt.Errorf("reading the database's sync origin: %w", err) - } - if configured := normalizeAPIURL(e.apiURL); recorded != configured { - return &OriginMismatch{Recorded: recorded, Configured: configured} - } - return nil -} - -// checkOrigin is the pull's stricter question: may this reconcile happen, and -// should it be the one to record the pairing. -// -// A database that has never pulled adopts on this one — that is every new -// install, whatever it has typed in locally, since a pull spares rows queued for -// push anyway. A database that has pulled before was bound to some API already, -// and if nothing recorded which, that has to be said out loud rather than -// inferred from whichever environment happens to be loaded. -func (e *Engine) checkOrigin(ctx context.Context) (adopt bool, err error) { - configured := normalizeAPIURL(e.apiURL) - - recorded, err := e.q.GetSyncOrigin(ctx) - switch { - case err == nil: - if recorded != configured { - return false, &OriginMismatch{Recorded: recorded, Configured: configured} - } - return false, nil - case !errors.Is(err, sql.ErrNoRows): - return false, fmt.Errorf("reading the database's sync origin: %w", err) - } - - pulledBefore, err := e.q.HasPulledBefore(ctx) - if err != nil { - return false, fmt.Errorf("checking whether this database has synced before: %w", err) - } - if pulledBefore == 0 { - return true, nil - } - - projects, err := e.q.CountProjects(ctx) - if err != nil { - return false, fmt.Errorf("counting projects: %w", err) - } - items, err := e.q.CountItems(ctx) - if err != nil { - return false, fmt.Errorf("counting items: %w", err) - } - return false, &OriginUnclaimed{ - Configured: configured, - Projects: int(projects), - Items: int(items), - } -} - -// AdoptOrigin binds this database to the engine's API, overwriting whatever it -// was bound to. It is what `projects sync --adopt` calls: the one place the -// pairing can change, and only because someone said so. -func (e *Engine) AdoptOrigin(ctx context.Context) error { - return e.q.SetSyncOrigin(ctx, normalizeAPIURL(e.apiURL)) -} - -// A pull deletes whatever the server did not return, so a truncated response -// and a genuine mass deletion are the same event seen from here. Scale is the -// only thing that separates them, and the cost of guessing wrong is asymmetric: -// a refused pull costs one flag, an obeyed one costs the database. The floor -// keeps the guard out of the way of small databases, where losing "most of it" -// is a handful of rows and routine. -const ( - sweepFloor = 10 - sweepShare = 0.5 -) - -// SweepRefused is returned when a pull would delete implausibly much. -type SweepRefused struct { - Entity string - Deleting int - Local int -} - -func (e *SweepRefused) Error() string { - return fmt.Sprintf( - "refusing to delete %d of %d %s in one pull — the API returned almost nothing this database has, "+ - "which is a truncated response or the wrong API far more often than it is a real deletion. "+ - "Run `projects sync --force` if it is genuinely what happened", - e.Deleting, e.Local, e.Entity) -} - -func guardSweep(entity string, deleting, local int, allowed bool) error { - if allowed || deleting < sweepFloor || float64(deleting) <= float64(local)*sweepShare { - return nil - } - return &SweepRefused{Entity: entity, Deleting: deleting, Local: local} -} - -// Origin returns the API this database is bound to, or "" if it has never -// pulled. -func (e *Engine) Origin(ctx context.Context) string { - recorded, err := e.q.GetSyncOrigin(ctx) - if err != nil { - return "" - } - return recorded -} diff --git a/sync/origin_test.go b/sync/origin_test.go deleted file mode 100644 index ac26a54..0000000 --- a/sync/origin_test.go +++ /dev/null @@ -1,266 +0,0 @@ -package sync_test - -import ( - "database/sql" - "encoding/json" - "errors" - "net/http" - "net/http/httptest" - "testing" - "time" - - "github.com/datapointchris/projects/model" - "github.com/datapointchris/projects/service" - "github.com/datapointchris/projects/store/sqlite" - "github.com/datapointchris/projects/sync" -) - -// apiWith serves a fixed set of projects and items, which is all a pull reads. -func apiWith(t *testing.T, projects []model.Project, items []model.ProjectItemDetail) *httptest.Server { - t.Helper() - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/api/v1/projects": - _ = json.NewEncoder(w).Encode(projects) - case "/api/v1/project-items": - _ = json.NewEncoder(w).Encode(items) - default: - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("[]")) - } - }) - ts := httptest.NewServer(handler) - t.Cleanup(ts.Close) - return ts -} - -func openTestDB(t *testing.T) *sql.DB { - t.Helper() - database, err := sqlite.Open(":memory:") - if err != nil { - t.Fatalf("opening database: %v", err) - } - t.Cleanup(func() { _ = database.Close() }) - return database -} - -func seededAPI(t *testing.T, projectCount, itemCount int) *httptest.Server { - t.Helper() - projects := make([]model.Project, 0, projectCount) - for i := range projectCount { - projects = append(projects, model.Project{ - ID: "proj-" + itoa(i), Name: "Project " + itoa(i), CreatedAt: time.Now(), - }) - } - items := make([]model.ProjectItemDetail, 0, itemCount) - for i := range itemCount { - items = append(items, model.ProjectItemDetail{ - ProjectItem: model.ProjectItem{ - ID: "item-" + itoa(i), Title: "Item " + itoa(i), - CreatedAt: time.Now(), UpdatedAt: time.Now(), - }, - Projects: []model.Project{{ID: "proj-0"}}, - DependencyIDs: []string{}, - Tasks: []model.ProjectItemTask{}, - }) - } - return apiWith(t, projects, items) -} - -func itoa(i int) string { - if i == 0 { - return "0" - } - var digits []byte - for i > 0 { - digits = append([]byte{byte('0' + i%10)}, digits...) - i /= 10 - } - return string(digits) -} - -// A fresh install must not need a ceremony: its first pull records which API it -// belongs to and gets on with it. -func TestFirstPullAdoptsTheAPI(t *testing.T) { - database := openTestDB(t) - ts := seededAPI(t, 2, 3) - - engine := sync.New(database, ts.URL, "") - if err := engine.Pull(t.Context()); err != nil { - t.Fatalf("first pull: %v", err) - } - if got := engine.Origin(t.Context()); got != ts.URL { - t.Errorf("origin = %q, want %q", got, ts.URL) - } -} - -// The incident this exists to prevent: a database that belongs to one API is -// pointed at another, and the pull faithfully deletes everything the second API -// has never heard of. A production database lost 42 projects and 286 items this -// way, and the command printed "Synced." -func TestPullRefusesADifferentAPI(t *testing.T) { - database := openTestDB(t) - real := seededAPI(t, 20, 40) - - if err := sync.New(database, real.URL, "").Pull(t.Context()); err != nil { - t.Fatalf("first pull: %v", err) - } - - other := seededAPI(t, 1, 2) - err := sync.New(database, other.URL, "").Pull(t.Context()) - - var mismatch *sync.OriginMismatch - if !errors.As(err, &mismatch) { - t.Fatalf("pull against a different API returned %v, want an OriginMismatch", err) - } - if mismatch.Recorded != real.URL || mismatch.Configured != other.URL { - t.Errorf("mismatch names %s → %s, want %s → %s", - mismatch.Recorded, mismatch.Configured, real.URL, other.URL) - } - - local := service.New(sqlite.New(database)) - projects, err := local.ListProjectsByStatus(model.StatusActive) - if err != nil { - t.Fatalf("ListProjectsByStatus: %v", err) - } - if len(projects) != 20 { - t.Errorf("the refused pull still took data: %d projects left of 20", len(projects)) - } -} - -// The refusal has to survive the trailing slash, or it fires on a difference -// that addresses the same server. -func TestOriginIgnoresATrailingSlash(t *testing.T) { - database := openTestDB(t) - ts := seededAPI(t, 2, 3) - - if err := sync.New(database, ts.URL, "").Pull(t.Context()); err != nil { - t.Fatalf("first pull: %v", err) - } - if err := sync.New(database, ts.URL+"/", "").Pull(t.Context()); err != nil { - t.Errorf("a trailing slash was treated as a different API: %v", err) - } -} - -// --adopt is the deliberate override, for an API that genuinely moved. -func TestAdoptRebindsTheDatabase(t *testing.T) { - database := openTestDB(t) - first := seededAPI(t, 2, 3) - if err := sync.New(database, first.URL, "").Pull(t.Context()); err != nil { - t.Fatalf("first pull: %v", err) - } - - moved := seededAPI(t, 2, 3) - engine := sync.New(database, moved.URL, "") - if err := engine.PullWith(t.Context(), sync.PullOptions{Adopt: true, AllowLargeSweep: true}); err != nil { - t.Fatalf("adopting pull: %v", err) - } - if got := engine.Origin(t.Context()); got != moved.URL { - t.Errorf("origin = %q, want %q", got, moved.URL) - } -} - -// A database carried over from before the binding existed has reconciled with -// something, and nothing recorded what. Guessing from whichever environment -// happens to be loaded is the mistake itself. -func TestPullRefusesToGuessForADatabaseThatSyncedBeforeTheBinding(t *testing.T) { - database := openTestDB(t) - ts := seededAPI(t, 20, 40) - if err := sync.New(database, ts.URL, "").Pull(t.Context()); err != nil { - t.Fatalf("first pull: %v", err) - } - if _, err := database.Exec("DELETE FROM sync_origin"); err != nil { - t.Fatalf("clearing the origin: %v", err) - } - - err := sync.New(database, ts.URL, "").Pull(t.Context()) - - var unclaimed *sync.OriginUnclaimed - if !errors.As(err, &unclaimed) { - t.Fatalf("pull returned %v, want an OriginUnclaimed", err) - } - if unclaimed.Projects != 20 || unclaimed.Items != 40 { - t.Errorf("the refusal reports %d projects and %d items, want 20 and 40", - unclaimed.Projects, unclaimed.Items) - } - - if err := sync.New(database, ts.URL, "").PullWith(t.Context(), sync.PullOptions{Adopt: true}); err != nil { - t.Errorf("--adopt did not resolve it: %v", err) - } -} - -// A truncated response and a real mass deletion are the same event from here, -// and only one of them should be obeyed without being asked. -func TestPullRefusesToDeleteMostOfTheDatabase(t *testing.T) { - database := openTestDB(t) - full := seededAPI(t, 20, 40) - if err := sync.New(database, full.URL, "").Pull(t.Context()); err != nil { - t.Fatalf("first pull: %v", err) - } - - // Same API, now answering with almost nothing — a bad deploy, an auth - // failure that still returns 200, a paginated endpoint read as complete. - truncated := apiWith(t, []model.Project{{ID: "proj-0", Name: "Project 0", CreatedAt: time.Now()}}, nil) - engine := sync.New(database, truncated.URL, "") - if err := engine.PullWith(t.Context(), sync.PullOptions{Adopt: true}); err == nil { - t.Fatal("a pull deleting 19 of 20 projects was allowed") - } else { - var refused *sync.SweepRefused - if !errors.As(err, &refused) { - t.Fatalf("got %v, want a SweepRefused", err) - } - } - - local := service.New(sqlite.New(database)) - projects, err := local.ListProjectsByStatus(model.StatusActive) - if err != nil { - t.Fatalf("ListProjectsByStatus: %v", err) - } - if len(projects) != 20 { - t.Errorf("the refused pull still deleted: %d projects left of 20", len(projects)) - } - - if err := engine.PullWith(t.Context(), sync.PullOptions{Adopt: true, AllowLargeSweep: true}); err != nil { - t.Fatalf("--force did not allow it: %v", err) - } - projects, err = local.ListProjectsByStatus(model.StatusActive) - if err != nil { - t.Fatalf("ListProjectsByStatus: %v", err) - } - if len(projects) != 1 { - t.Errorf("--force did not apply the sweep: %d projects left", len(projects)) - } -} - -// The guard must not fire on ordinary tidying, or it trains you to pass --force -// by reflex and stops guarding anything. -func TestPullAllowsAnOrdinaryDeletion(t *testing.T) { - database := openTestDB(t) - full := seededAPI(t, 20, 40) - if err := sync.New(database, full.URL, "").Pull(t.Context()); err != nil { - t.Fatalf("first pull: %v", err) - } - - fewer := seededAPI(t, 18, 32) - engine := sync.New(database, fewer.URL, "") - if err := engine.PullWith(t.Context(), sync.PullOptions{Adopt: true}); err != nil { - t.Fatalf("deleting 2 of 20 projects and 8 of 40 items was refused: %v", err) - } -} - -// A small database loses "most of itself" on any ordinary day, so the guard has -// a floor — otherwise it fires constantly and gets forced past on reflex. -func TestSweepGuardHasAFloorForSmallDatabases(t *testing.T) { - database := openTestDB(t) - full := seededAPI(t, 4, 6) - if err := sync.New(database, full.URL, "").Pull(t.Context()); err != nil { - t.Fatalf("first pull: %v", err) - } - - empty := apiWith(t, nil, nil) - engine := sync.New(database, empty.URL, "") - if err := engine.PullWith(t.Context(), sync.PullOptions{Adopt: true}); err != nil { - t.Errorf("a 4-project database was guarded: %v", err) - } -} diff --git a/sync/pull.go b/sync/pull.go deleted file mode 100644 index 67587f5..0000000 --- a/sync/pull.go +++ /dev/null @@ -1,446 +0,0 @@ -package sync - -import ( - "context" - "database/sql" - "encoding/json" - "fmt" - "io" - "net/http" - "time" - - "github.com/datapointchris/projects/model" - "github.com/datapointchris/projects/store/sqlite/generated" -) - -// Pull fetches all data from the remote API and reconciles it with the local database. -// It attempts to push pending ops first (so the server has our changes), then does a -// full pull. Server state wins on conflicts. -// -// A failure is recorded on the status so the TUI's indicator reflects it. Pull runs -// unattended on a timer, and an error only a foreground caller can see would leave -// the bar reading SYNCED while nothing had reconciled for hours. -func (e *Engine) Pull(ctx context.Context) error { - return e.PullWith(ctx, PullOptions{}) -} - -// PullOptions carries the deliberate overrides of the two guards that stand -// between a misconfigured reconcile and an emptied database. Neither is -// reachable from the background timers — a guard something automatic can waive -// is not a guard. -type PullOptions struct { - // Adopt binds the database to this API before reconciling, which is the - // only way the pairing ever changes. `projects sync --adopt`. - Adopt bool - // AllowLargeSweep permits a pull that would delete most of what is here. - // `projects sync --force`. - AllowLargeSweep bool -} - -func (e *Engine) PullWith(ctx context.Context, opts PullOptions) error { - e.beginSync() - defer e.endSync() - - err := e.pull(ctx, opts) - e.setStatus(func(s *SyncStatus) { - s.Connected = err == nil - if err != nil { - s.LastError = err.Error() - } else { - s.LastError = "" - } - }) - return err -} - -func (e *Engine) pull(ctx context.Context, opts PullOptions) error { - if opts.Adopt { - if err := e.AdoptOrigin(ctx); err != nil { - return fmt.Errorf("adopting %s: %w", e.apiURL, err) - } - } - // Before the push, not after: draining first would send this database's - // queued work to an API that is not its own. - adopt, err := e.checkOrigin(ctx) - if err != nil { - return err - } - if adopt { - if err := e.AdoptOrigin(ctx); err != nil { - return fmt.Errorf("recording %s as this database's origin: %w", e.apiURL, err) - } - } - - // Push first: try to drain pending ops so server has our changes - e.drainPendingOps() - - // Everything queued from here on describes a local change the fetch below - // cannot know about. Recording the high-water mark lets the reconciliation - // clear only what it actually superseded — see the delete calls at the end. - pushedThrough, err := e.q.MaxPendingSyncID(ctx) - if err != nil { - return fmt.Errorf("reading pending sync high-water mark: %w", err) - } - - // Fetch all data from the server. - // - // status=all because the sweep below deletes any local project the server - // did not return, cascading through to its memberships. Once /projects - // defaults to active-only, an omitted-because-completed project reads as - // deleted-upstream and the row goes. Terminal projects are filtered for - // display locally instead. The param is ignored by an API predating it, - // which is what lets this ship ahead of the server change. - projects, err := fetchJSON[[]model.Project](ctx, e.client, e.base, "/projects?status=all") - if err != nil { - return fmt.Errorf("pulling projects: %w", err) - } - - // status=all for the same reason as projects. /project-items defaults to - // open items, so without it every completed item is absent from the response - // and the sweep below deletes it. - items, err := fetchJSON[[]model.ProjectItemDetail](ctx, e.client, e.base, "/project-items?status=all") - if err != nil { - return fmt.Errorf("pulling items: %w", err) - } - - type itemExtra struct { - detail *model.ProjectItemDetail - tasks []model.ProjectItemTask - } - extras := make(map[string]itemExtra, len(items)) - - for _, item := range items { - // The list embeds memberships, dependencies, and tasks, so the whole - // reconcile is two requests. The fallback below costs two more per item: - // 122 serial round trips and 6.15s at 60 items. - if item.DependencyIDs != nil && item.Tasks != nil { - extras[item.ID] = itemExtra{detail: &item, tasks: item.Tasks} - continue - } - - // A server predating the embed omits both fields. Falling back rather - // than trusting the zero value matters: treating absent as empty would - // erase every task and dependency locally on the first pull against an - // older API, so deploy order between this and ichrisbirch cannot lose data. - detail, err := fetchJSON[*model.ProjectItemDetail](ctx, e.client, e.base, fmt.Sprintf("/project-items/%s", item.ID)) - if err != nil { - return fmt.Errorf("pulling item %s detail: %w", item.ID, err) - } - - tasks, err := fetchJSON[[]model.ProjectItemTask](ctx, e.client, e.base, fmt.Sprintf("/project-items/%s/tasks", item.ID)) - if err != nil { - return fmt.Errorf("pulling item %s tasks: %w", item.ID, err) - } - - extras[item.ID] = itemExtra{detail: detail, tasks: tasks} - } - - // Reconcile in a transaction - tx, err := e.db.Begin() - if err != nil { - return fmt.Errorf("beginning sync transaction: %w", err) - } - defer func() { _ = tx.Rollback() }() - qtx := e.q.WithTx(tx) - - // Entities changed locally while the fetch was in flight. The server response - // predates them, so the sweeps below would read them as "deleted upstream" and - // erase a row the user created seconds ago. Ids are UUIDv7 and unique across - // tables, so one set covers projects, items, and tasks alike. - pendingIDs, err := qtx.ListPendingSyncEntityIDsAfter(ctx, pushedThrough) - if err != nil { - return fmt.Errorf("listing in-flight sync entities: %w", err) - } - queuedLocally := make(map[string]bool, len(pendingIDs)) - for _, id := range pendingIDs { - queuedLocally[id] = true - } - - // Upsert projects - serverProjectIDs := make(map[string]bool, len(projects)) - for _, p := range projects { - serverProjectIDs[p.ID] = true - // A server predating the status column sends none, and an empty status - // would violate NOT NULL and fail the whole pull. Absent means active, - // which is what every project was before the column existed. - status := p.Status - if status == "" { - status = model.StatusActive - } - if err := qtx.UpsertProject(ctx, generated.UpsertProjectParams{ - ID: p.ID, - Name: p.Name, - Description: nullStr(p.Description), - Status: status, - StatusReason: nullStr(p.StatusReason), - ClosedAt: nullTime(p.ClosedAt), - Position: int64(p.Position), - CreatedAt: p.CreatedAt.Format(time.RFC3339Nano), - }); err != nil { - return fmt.Errorf("upserting project %s: %w", p.ID, err) - } - } - - // Delete local projects not on server - localProjects, err := qtx.ListAllProjectsRaw(ctx) - if err != nil { - return fmt.Errorf("listing local projects: %w", err) - } - // Which projects a membership may legally point at once this loop is done: - // what the server sent, plus anything created locally that the sweep spares. - projectExists := make(map[string]bool, len(serverProjectIDs)) - for id := range serverProjectIDs { - projectExists[id] = true - } - var staleProjects []string - for _, lp := range localProjects { - if !serverProjectIDs[lp.ID] && !queuedLocally[lp.ID] { - staleProjects = append(staleProjects, lp.ID) - continue - } - projectExists[lp.ID] = true - } - if err := guardSweep("projects", len(staleProjects), len(localProjects), opts.AllowLargeSweep); err != nil { - return err - } - for _, id := range staleProjects { - if err := qtx.DeleteProject(ctx, id); err != nil { - return fmt.Errorf("deleting stale project %s: %w", id, err) - } - } - - // Upsert items, unnumbered. - // - // idx_items_number is unique and the upsert writes row by row, so an item - // taking a number another row has not given up yet fails the whole pull — - // which is what any renumbering upstream looks like from here. Clearing - // every server item to unnumbered first makes the pass order-independent; - // the index is partial over non-null numbers, so any number of rows may sit - // unnumbered in between. - serverItemIDs := make(map[string]bool, len(items)) - for _, item := range items { - serverItemIDs[item.ID] = true - if err := qtx.UpsertItem(ctx, generated.UpsertItemParams{ - ID: item.ID, - Title: item.Title, - Notes: nullStr(item.Notes), - Repo: nullStr(item.Repo), - Completed: boolToInt(item.Completed), - Archived: boolToInt(item.Archived), - CreatedAt: item.CreatedAt.Format(time.RFC3339Nano), - UpdatedAt: item.UpdatedAt.Format(time.RFC3339Nano), - }); err != nil { - return fmt.Errorf("upserting item %s: %w", item.ID, err) - } - } - // Delete local items not on server - localItems, err := qtx.ListAllItemsRaw(ctx) - if err != nil { - return fmt.Errorf("listing local items: %w", err) - } - itemExists := make(map[string]bool, len(serverItemIDs)) - for id := range serverItemIDs { - itemExists[id] = true - } - var staleItems []string - for _, li := range localItems { - if !serverItemIDs[li.ID] && !queuedLocally[li.ID] { - staleItems = append(staleItems, li.ID) - continue - } - itemExists[li.ID] = true - } - if err := guardSweep("items", len(staleItems), len(localItems), opts.AllowLargeSweep); err != nil { - return err - } - for _, id := range staleItems { - if err := qtx.DeleteItem(ctx, id); err != nil { - return fmt.Errorf("deleting stale item %s: %w", id, err) - } - } - - // Numbers go on last of all, once the rows that are leaving have left: an - // item deleted upstream still holds its number until the sweep above runs, - // and the server reissuing that number to something else is exactly the - // collision this ordering avoids. - for _, item := range items { - if item.Number == nil { - continue - } - if err := qtx.SetItemNumber(ctx, generated.SetItemNumberParams{ - ID: item.ID, - Number: nullInt(item.Number), - }); err != nil { - return fmt.Errorf("numbering item %s: %w", item.ID, err) - } - } - - // Replace all memberships with server state - if err := qtx.DeleteAllMemberships(ctx); err != nil { - return fmt.Errorf("clearing memberships: %w", err) - } - for _, extra := range extras { - if extra.detail == nil { - continue - } - for _, proj := range extra.detail.Projects { - // An item detail can name a project the projects endpoint did not - // return — the two answers are assembled separately server-side and - // nothing makes them agree. The link is dropped rather than - // inserted: it has no project to point at, and failing the pull over - // it would take the whole reconcile down with it, which is the - // opposite of local-first. - if !projectExists[proj.ID] || !itemExists[extra.detail.ID] { - continue - } - if err := qtx.UpsertMembership(ctx, generated.UpsertMembershipParams{ - ItemID: extra.detail.ID, - ProjectID: proj.ID, - Position: 0, // server doesn't return per-project position in detail - }); err != nil { - return fmt.Errorf("upserting membership: %w", err) - } - } - } - - // Replace all dependencies with server state - if err := qtx.DeleteAllDependencies(ctx); err != nil { - return fmt.Errorf("clearing dependencies: %w", err) - } - for _, extra := range extras { - if extra.detail == nil { - continue - } - for _, depID := range extra.detail.DependencyIDs { - // Same reasoning as memberships: a blocker the server no longer - // lists has no row to reference. - if !itemExists[depID] || !itemExists[extra.detail.ID] { - continue - } - if err := qtx.UpsertDependency(ctx, generated.UpsertDependencyParams{ - ItemID: extra.detail.ID, - DependsOnID: depID, - }); err != nil { - return fmt.Errorf("upserting dependency: %w", err) - } - } - } - - // Upsert tasks and delete stale ones - serverTaskIDs := make(map[string]bool) - for _, extra := range extras { - for _, task := range extra.tasks { - if !itemExists[task.ItemID] { - continue - } - serverTaskIDs[task.ID] = true - if err := qtx.UpsertTask(ctx, generated.UpsertTaskParams{ - ID: task.ID, - ItemID: task.ItemID, - Title: task.Title, - Completed: boolToInt(task.Completed), - Position: int64(task.Position), - CreatedAt: task.CreatedAt.Format(time.RFC3339Nano), - }); err != nil { - return fmt.Errorf("upserting task %s: %w", task.ID, err) - } - } - } - - localTasks, err := qtx.ListAllTasks(ctx) - if err != nil { - return fmt.Errorf("listing local tasks: %w", err) - } - for _, lt := range localTasks { - if !serverTaskIDs[lt.ID] && !queuedLocally[lt.ID] { - if err := qtx.DeleteTask(ctx, lt.ID); err != nil { - return fmt.Errorf("deleting stale task %s: %w", lt.ID, err) - } - } - } - - // Clear only the ops this reconciliation superseded. Anything queued after the - // high-water mark postdates the server response and still has to be pushed. - if err := qtx.DeletePendingSyncUpTo(ctx, pushedThrough); err != nil { - return fmt.Errorf("clearing pending sync: %w", err) - } - - // Update sync state - now := time.Now().UTC().Format(time.RFC3339Nano) - for _, entityType := range []string{"project", "item", "task"} { - if err := qtx.UpsertSyncState(ctx, generated.UpsertSyncStateParams{ - EntityType: entityType, - LastPullAt: now, - LastPushAt: now, - }); err != nil { - return fmt.Errorf("updating sync state: %w", err) - } - } - - if err := tx.Commit(); err != nil { - return fmt.Errorf("committing sync: %w", err) - } - - // Ops that survived the truncation were queued mid-pull and are still unsent. - if len(queuedLocally) > 0 { - e.Notify() - } - return nil -} - -// fetchJSON performs a GET request and decodes the JSON response into T. -func fetchJSON[T any](ctx context.Context, client *http.Client, baseURL, path string) (T, error) { - var zero T - - req, err := http.NewRequestWithContext(ctx, http.MethodGet, baseURL+path, nil) - if err != nil { - return zero, err - } - - resp, err := client.Do(req) - if err != nil { - return zero, friendlyNetErr(err) - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode >= 400 { - body, _ := io.ReadAll(resp.Body) - return zero, refusal("API error", resp.StatusCode, body) - } - - var result T - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { - return zero, fmt.Errorf("decoding response: %w", err) - } - return result, nil -} - -func nullStr(s *string) sql.NullString { - if s == nil { - return sql.NullString{} - } - return sql.NullString{String: *s, Valid: true} -} - -// nullTime stores a nullable timestamp as RFC3339Nano, which trims trailing zeros. -// These columns sort as text, so a pulled .34Z sorts after a local .343Z. -func nullTime(t *time.Time) sql.NullString { - if t == nil { - return sql.NullString{} - } - return sql.NullString{String: t.Format(time.RFC3339Nano), Valid: true} -} - -func nullInt(n *int) sql.NullInt64 { - if n == nil { - return sql.NullInt64{} - } - return sql.NullInt64{Int64: int64(*n), Valid: true} -} - -func boolToInt(b bool) int64 { - if b { - return 1 - } - return 0 -} diff --git a/sync/push.go b/sync/push.go deleted file mode 100644 index 4cf4ff8..0000000 --- a/sync/push.go +++ /dev/null @@ -1,221 +0,0 @@ -package sync - -import ( - "bytes" - "database/sql" - "encoding/json" - "errors" - "fmt" - "io" - "net/http" - "strings" - - "github.com/datapointchris/projects/store/sqlite/generated" -) - -// executePush translates a pending sync operation to an HTTP request. -// Returns nil on success, or on server rejection (404/409) which means -// the operation should be dropped (server wins). -// Returns non-nil error on network/transient failures that should be retried. -func (e *Engine) executePush(op generated.PendingSync) error { - opType := OpType(op.Operation) - - switch opType { - case OpCreateProject: - return e.pushJSON(http.MethodPost, "/projects", op.Payload) - case OpUpdateProject: - return e.pushJSON(http.MethodPatch, fmt.Sprintf("/projects/%s", op.EntityID), op.Payload) - case OpDeleteProject: - return e.pushDelete(fmt.Sprintf("/projects/%s", op.EntityID)) - case OpReorderProject: - // The payload is {"position": n}, which a project's own PATCH takes. - return e.pushJSON(http.MethodPatch, fmt.Sprintf("/projects/%s", op.EntityID), op.Payload) - - case OpCreateItem: - return e.pushCreatedItem(op.EntityID, op.Payload) - case OpUpdateItem: - return e.pushJSON(http.MethodPatch, fmt.Sprintf("/project-items/%s", op.EntityID), op.Payload) - case OpDeleteItem: - return e.pushDelete(fmt.Sprintf("/project-items/%s", op.EntityID)) - - case OpReorderItem: - return e.pushJSON(http.MethodPatch, fmt.Sprintf("/project-items/%s/reorder", op.EntityID), op.Payload) - - case OpAddToProject: - return e.pushJSON(http.MethodPost, fmt.Sprintf("/project-items/%s/projects", op.EntityID), op.Payload) - case OpRemoveFromProject: - var p struct { - ProjectID string `json:"project_id"` - } - _ = json.Unmarshal([]byte(op.Payload), &p) - return e.pushDelete(fmt.Sprintf("/project-items/%s/projects/%s", op.EntityID, p.ProjectID)) - - case OpCreateTask: - var p struct { - ItemID string `json:"item_id"` - } - _ = json.Unmarshal([]byte(op.Payload), &p) - return e.pushJSON(http.MethodPost, fmt.Sprintf("/project-items/%s/tasks", p.ItemID), op.Payload) - case OpUpdateTask: - var p struct { - ItemID string `json:"item_id"` - } - _ = json.Unmarshal([]byte(op.Payload), &p) - return e.pushJSON(http.MethodPatch, fmt.Sprintf("/project-items/%s/tasks/%s", p.ItemID, op.EntityID), op.Payload) - case OpDeleteTask: - var p struct { - ItemID string `json:"item_id"` - } - _ = json.Unmarshal([]byte(op.Payload), &p) - return e.pushDelete(fmt.Sprintf("/project-items/%s/tasks/%s", p.ItemID, op.EntityID)) - case OpCompleteTask: - var p struct { - ItemID string `json:"item_id"` - } - _ = json.Unmarshal([]byte(op.Payload), &p) - return e.pushJSON(http.MethodPatch, fmt.Sprintf("/project-items/%s/tasks/%s", p.ItemID, op.EntityID), `{"completed":true}`) - - case OpAddDependency: - return e.pushJSON(http.MethodPost, fmt.Sprintf("/project-items/%s/dependencies", op.EntityID), op.Payload) - case OpRemoveDependency: - var p struct { - DependsOnID string `json:"depends_on_id"` - } - _ = json.Unmarshal([]byte(op.Payload), &p) - return e.pushDelete(fmt.Sprintf("/project-items/%s/dependencies/%s", op.EntityID, p.DependsOnID)) - - default: - return fmt.Errorf("unknown sync operation: %s", op.Operation) - } -} - -// pushCreatedItem posts a queued item create and records the number the server -// assigned to it. -// -// The number is the item's handle, and until the server answers there is none — -// so without this the item would stay unnamed until the next pull, up to a full -// sync interval after the command that created it returned. Long enough that -// the thing you just filed cannot be referred to by the handle it already has. -func (e *Engine) pushCreatedItem(itemID, payload string) error { - var created struct { - Number *int `json:"number"` - } - if err := e.pushJSONInto(http.MethodPost, "/project-items", payload, &created); err != nil { - return err - } - if created.Number == nil { - return nil - } - return e.q.SetItemNumber(e.ctx, generated.SetItemNumberParams{ - Number: sql.NullInt64{Int64: int64(*created.Number), Valid: true}, - ID: itemID, - }) -} - -// pushJSON sends a JSON request and handles the response. -// 404/409 are treated as "drop this op" (returns nil). -// Network errors and 5xx are retryable (returns error). -func (e *Engine) pushJSON(method, path string, payload string) error { - return e.pushJSONInto(method, path, payload, nil) -} - -// pushJSONInto is pushJSON with the response decoded into out when the server -// accepted the request. A body that will not decode is not an error: the write -// itself landed, and failing here would re-queue a create the server has -// already applied. The missing field arrives on the next pull instead. -func (e *Engine) pushJSONInto(method, path string, payload string, out any) error { - var body io.Reader - if payload != "" { - body = bytes.NewReader([]byte(payload)) - } - - req, err := http.NewRequestWithContext(e.ctx, method, e.base+path, body) - if err != nil { - return err - } - if body != nil { - req.Header.Set("Content-Type", "application/json") - } - - resp, err := e.client.Do(req) - if err != nil { - return friendlyNetErr(err) - } - defer func() { _ = resp.Body.Close() }() - responseBody, _ := io.ReadAll(resp.Body) - - if err := classifyResponse(resp.StatusCode, responseBody); err != nil { - return err - } - if out != nil && resp.StatusCode < 400 { - _ = json.Unmarshal(responseBody, out) - } - return nil -} - -// pushDelete sends a DELETE request. -func (e *Engine) pushDelete(path string) error { - req, err := http.NewRequestWithContext(e.ctx, http.MethodDelete, e.base+path, nil) - if err != nil { - return err - } - - resp, err := e.client.Do(req) - if err != nil { - return friendlyNetErr(err) - } - defer func() { _ = resp.Body.Close() }() - responseBody, _ := io.ReadAll(resp.Body) - - return classifyResponse(resp.StatusCode, responseBody) -} - -// classifyResponse decides whether a status code means success, drop, or retry. -func classifyResponse(status int, body []byte) error { - switch { - case status < 400: - return nil // success - case status == http.StatusNotFound, status == http.StatusConflict: - return nil // drop: server wins, next pull reconciles - case status >= 500: - return refusal("server error", status, body) - default: - return refusal("push rejected", status, body) - } -} - -// refusal names the status and the reason the API gave in its error body's -// `message`, or the body as it arrived when it carries none. -func refusal(prefix string, status int, body []byte) error { - reason := strings.TrimSpace(string(body)) - var answer struct { - Message string `json:"message"` - } - if json.Unmarshal(body, &answer) == nil && answer.Message != "" { - reason = answer.Message - } - return fmt.Errorf("%s (%d %s): %s", prefix, status, http.StatusText(status), reason) -} - -// friendlyNetErr translates raw network errors into user-readable messages. -// -// A token command's failure arrives through the same call, wrapped as a network -// error. Its own message says what to run, and its timeout is not the API's, so -// it is returned as it is. -func friendlyNetErr(err error) error { - var tokenErr *TokenCommandError - if errors.As(err, &tokenErr) { - return tokenErr - } - msg := err.Error() - switch { - case strings.Contains(msg, "connection refused"): - return fmt.Errorf("cannot reach API server (connection refused)") - case strings.Contains(msg, "no such host"): - return fmt.Errorf("cannot reach API server (DNS lookup failed)") - case strings.Contains(msg, "Timeout") || strings.Contains(msg, "deadline exceeded"): - return fmt.Errorf("API server did not respond (timeout)") - default: - return fmt.Errorf("network error: %w", err) - } -} diff --git a/sync/status.go b/sync/status.go deleted file mode 100644 index dfd3338..0000000 --- a/sync/status.go +++ /dev/null @@ -1,9 +0,0 @@ -package sync - -// SyncStatus represents the current state of the sync engine for TUI display. -type SyncStatus struct { - Connected bool - PendingCount int - LastError string - Syncing bool -} diff --git a/sync/token.go b/sync/token.go deleted file mode 100644 index 73796dd..0000000 --- a/sync/token.go +++ /dev/null @@ -1,153 +0,0 @@ -package sync - -import ( - "bytes" - "context" - "errors" - "fmt" - "io" - "net/http" - "os/exec" - "strings" - gosync "sync" - "time" -) - -// tokenCommandTimeout bounds one run of sync.token_command. A command that -// refreshes its own login reaches an identity provider, which answers well -// inside this when it answers at all. -const tokenCommandTimeout = 10 * time.Second - -// TokenCommandError is sync.token_command failing to print a token. Reason is -// the first line the command wrote to stderr where it wrote one, because that is -// where a CLI says its login has lapsed and what to run. -type TokenCommandError struct { - Command string - Reason string -} - -func (e *TokenCommandError) Error() string { - return fmt.Sprintf("sync.token_command %q: %s", e.Command, e.Reason) -} - -// tokenTransport sends the bearer token sync.token_command prints. -// -// The command runs on the first request and again only after the API refuses -// the token held, so a pull of many requests costs one run. A refused token is -// replaced once and the request sent once more; a second refusal is the API's -// answer, and so is a refusal of a token printed for this request. -type tokenTransport struct { - argv []string - base http.RoundTripper - - mu gosync.Mutex - token string -} - -func newTokenTransport(argv []string, base http.RoundTripper) *tokenTransport { - return &tokenTransport{argv: argv, base: base} -} - -func (t *tokenTransport) RoundTrip(req *http.Request) (*http.Response, error) { - token, printedNow, err := t.held(req.Context()) - if err != nil { - closeBody(req) - return nil, err - } - - resp, err := t.send(req, req.Body, token) - if err != nil || resp.StatusCode != http.StatusUnauthorized || printedNow { - return resp, err - } - // A body cannot be sent twice unless the request can produce it again, which - // http.NewRequest arranges for every in-memory body. - if req.Body != nil && req.GetBody == nil { - return resp, nil - } - - replacement, err := t.replace(req.Context(), token) - if err != nil { - _ = resp.Body.Close() - return nil, err - } - var body io.ReadCloser - if req.GetBody != nil { - if body, err = req.GetBody(); err != nil { - return resp, nil - } - } - _ = resp.Body.Close() - return t.send(req, body, replacement) -} - -func (t *tokenTransport) send(req *http.Request, body io.ReadCloser, token string) (*http.Response, error) { - out := req.Clone(req.Context()) - out.Body = body - out.Header.Set("Authorization", "Bearer "+token) - return t.base.RoundTrip(out) -} - -// held returns the token to send, running the command when none is held. -// printedNow reports that the command printed it for this request. -func (t *tokenTransport) held(ctx context.Context) (token string, printedNow bool, err error) { - t.mu.Lock() - defer t.mu.Unlock() - if t.token != "" { - return t.token, false, nil - } - if t.token, err = runTokenCommand(ctx, t.argv); err != nil { - return "", false, err - } - return t.token, true, nil -} - -// replace runs the command again for a refused token. Requests in flight -// together are refused together, so the first to arrive replaces it and the -// rest take its replacement rather than each running the command. -func (t *tokenTransport) replace(ctx context.Context, refused string) (string, error) { - t.mu.Lock() - defer t.mu.Unlock() - if t.token != "" && t.token != refused { - return t.token, nil - } - token, err := runTokenCommand(ctx, t.argv) - t.token = token - return token, err -} - -// runTokenCommand runs argv without a shell and with no stdin, so a command -// that would prompt fails rather than waiting on a terminal the TUI owns. -func runTokenCommand(ctx context.Context, argv []string) (string, error) { - command := strings.Join(argv, " ") - ctx, cancel := context.WithTimeout(ctx, tokenCommandTimeout) - defer cancel() - - cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) - var stderr bytes.Buffer - cmd.Stderr = &stderr - out, err := cmd.Output() - if err != nil { - return "", &TokenCommandError{Command: command, Reason: failureReason(ctx, err, stderr.String())} - } - token := strings.TrimSpace(string(out)) - if token == "" { - return "", &TokenCommandError{Command: command, Reason: "printed no token"} - } - return token, nil -} - -func failureReason(ctx context.Context, err error, stderr string) string { - if errors.Is(ctx.Err(), context.DeadlineExceeded) { - return fmt.Sprintf("did not finish within %s", tokenCommandTimeout) - } - if line, _, _ := strings.Cut(strings.TrimSpace(stderr), "\n"); line != "" { - return line - } - return err.Error() -} - -func closeBody(req *http.Request) { - if req.Body != nil { - _ = req.Body.Close() - } -} diff --git a/sync/token_test.go b/sync/token_test.go deleted file mode 100644 index ad0f59a..0000000 --- a/sync/token_test.go +++ /dev/null @@ -1,257 +0,0 @@ -package sync_test - -import ( - "errors" - "io" - "net/http" - "net/http/httptest" - "os" - "path/filepath" - "strconv" - "strings" - goSync "sync" - "testing" - - "github.com/datapointchris/projects/sync" -) - -// seenRequest is one request as the API received it. -type seenRequest struct { - Authorization, Method, Path, Body string -} - -// bearerAPI answers a pull with empty lists for a request whose Authorization -// header is the accepted one, and 401 for any other. It records every request. -type bearerAPI struct { - *httptest.Server - mu goSync.Mutex - accepted string - seen []seenRequest -} - -func newBearerAPI(t *testing.T, accepted string) *bearerAPI { - t.Helper() - api := &bearerAPI{accepted: accepted} - api.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - body, _ := io.ReadAll(r.Body) - api.mu.Lock() - authorization := r.Header.Get("Authorization") - api.seen = append(api.seen, seenRequest{authorization, r.Method, r.URL.Path, string(body)}) - ok := authorization == api.accepted - api.mu.Unlock() - if !ok { - w.WriteHeader(http.StatusUnauthorized) - return - } - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte("[]")) - })) - t.Cleanup(api.Close) - return api -} - -func (a *bearerAPI) accept(authorization string) { - a.mu.Lock() - defer a.mu.Unlock() - a.accepted = authorization -} - -func (a *bearerAPI) requests() []seenRequest { - a.mu.Lock() - defer a.mu.Unlock() - return append([]seenRequest(nil), a.seen...) -} - -// writeTokenScript writes body as an executable shell script and returns its -// path, which holds no space, so it survives being split into a command. -func writeTokenScript(t *testing.T, dir, body string) string { - t.Helper() - path := filepath.Join(dir, "print-token") - if err := os.WriteFile(path, []byte("#!/bin/sh\n"+body), 0o700); err != nil { - t.Fatalf("writing token script: %v", err) - } - return path -} - -// tokenCommand returns a command printing tokens[n-1] on its nth run, and the -// last token on every run after, with a count of its runs so far. -func tokenCommand(t *testing.T, tokens ...string) (string, func() int) { - t.Helper() - dir := t.TempDir() - if err := os.WriteFile(filepath.Join(dir, "tokens"), []byte(strings.Join(tokens, "\n")+"\n"), 0o600); err != nil { - t.Fatalf("writing tokens: %v", err) - } - script := writeTokenScript(t, dir, `dir=$(dirname "$0") -runs=$(( $(cat "$dir/runs" 2>/dev/null || echo 0) + 1 )) -echo "$runs" > "$dir/runs" -total=$(wc -l < "$dir/tokens") -[ "$runs" -gt "$total" ] && line=$total || line=$runs -sed -n "${line}p" "$dir/tokens" -`) - runs := func() int { - data, err := os.ReadFile(filepath.Join(dir, "runs")) - if err != nil { - return 0 - } - n, _ := strconv.Atoi(strings.TrimSpace(string(data))) - return n - } - return script, runs -} - -func mustPull(t *testing.T, engine *sync.Engine) { - t.Helper() - if err := engine.Pull(t.Context()); err != nil { - t.Fatalf("pull: %v", err) - } -} - -// A pull is several requests, and the command runs for the first alone. -func TestEveryRequestCarriesTheTokenTheCommandPrints(t *testing.T) { - command, runs := tokenCommand(t, "tok-1") - api := newBearerAPI(t, "Bearer tok-1") - engine := sync.New(openTestDB(t), api.URL, command) - - mustPull(t, engine) - mustPull(t, engine) - - seen := api.requests() - if len(seen) < 4 { - t.Fatalf("two pulls made %d requests, want at least 4", len(seen)) - } - for _, r := range seen { - if r.Authorization != "Bearer tok-1" { - t.Errorf("%s %s carried %q, want the printed token", r.Method, r.Path, r.Authorization) - } - } - if got := runs(); got != 1 { - t.Errorf("the command ran %d times for %d requests, want once", got, len(seen)) - } -} - -// An access token expires while the TUI is open. The next request is refused, -// and the command runs once more for a token the API takes. -func TestAnExpiredTokenIsReplacedAndTheRequestSentAgain(t *testing.T) { - command, runs := tokenCommand(t, "tok-1", "tok-2") - api := newBearerAPI(t, "Bearer tok-1") - engine := sync.New(openTestDB(t), api.URL, command) - mustPull(t, engine) - - api.accept("Bearer tok-2") - mustPull(t, engine) - - if got := runs(); got != 2 { - t.Errorf("the command ran %d times, want 2", got) - } - seen := api.requests() - if last := seen[len(seen)-1]; last.Authorization != "Bearer tok-2" { - t.Errorf("the last request carried %q, want the replacement", last.Authorization) - } -} - -// Running the command again for a token it printed a moment ago would print the -// same refusal, so the refusal is the answer. -func TestATokenRefusedAsSoonAsItIsPrintedIsTheAnswer(t *testing.T) { - command, runs := tokenCommand(t, "tok-1", "tok-2") - api := newBearerAPI(t, "Bearer nothing-it-prints") - - if err := sync.New(openTestDB(t), api.URL, command).Pull(t.Context()); err == nil { - t.Fatal("a pull whose every request is refused succeeded") - } - if got := runs(); got != 1 { - t.Errorf("the command ran %d times, want once", got) - } -} - -func TestARefusedReplacementIsTheAnswer(t *testing.T) { - command, runs := tokenCommand(t, "tok-1", "tok-2", "tok-3") - api := newBearerAPI(t, "Bearer tok-1") - engine := sync.New(openTestDB(t), api.URL, command) - mustPull(t, engine) - - api.accept("Bearer nothing-it-prints") - if err := engine.Pull(t.Context()); err == nil { - t.Fatal("a pull whose every request is refused succeeded") - } - if got := runs(); got != 2 { - t.Errorf("the command ran %d times, want 2: the first token and one replacement", got) - } -} - -// A create refused for an expired token has not reached the server, so it is -// sent again whole. -func TestAPushRefusedForAnExpiredTokenIsResentWithItsBody(t *testing.T) { - command, _ := tokenCommand(t, "tok-1", "tok-2") - api := newBearerAPI(t, "Bearer tok-1") - engine := sync.New(openTestDB(t), api.URL, command) - mustPull(t, engine) - - api.accept("Bearer tok-2") - if err := engine.QueueOp(sync.OpCreateProject, "proj-1", map[string]string{"id": "proj-1", "name": "Boxes"}); err != nil { - t.Fatalf("queueing a create: %v", err) - } - mustPull(t, engine) - - var posts []seenRequest - for _, r := range api.requests() { - if r.Method == http.MethodPost && r.Path == "/api/v1/projects" { - posts = append(posts, r) - } - } - if len(posts) != 2 { - t.Fatalf("the create was posted %d times, want 2: refused, then resent", len(posts)) - } - if posts[1].Authorization != "Bearer tok-2" { - t.Errorf("the resend carried %q, want the replacement token", posts[1].Authorization) - } - if posts[1].Body == "" || posts[1].Body != posts[0].Body { - t.Errorf("the resend's body %q, want the original %q", posts[1].Body, posts[0].Body) - } -} - -// A CLI whose login has lapsed says so on stderr, and that line is what the -// status bar needs, rather than a network error. -func TestAFailingCommandSaysWhatItPrinted(t *testing.T) { - script := writeTokenScript(t, t.TempDir(), "echo 'Not logged in. Run the login command.' >&2\nexit 1\n") - api := newBearerAPI(t, "Bearer tok-1") - - err := sync.New(openTestDB(t), api.URL, script).Pull(t.Context()) - - var tokenErr *sync.TokenCommandError - if !errors.As(err, &tokenErr) { - t.Fatalf("pull error %v, want a TokenCommandError", err) - } - if tokenErr.Reason != "Not logged in. Run the login command." { - t.Errorf("reason %q, want the command's stderr", tokenErr.Reason) - } - if strings.Contains(err.Error(), "network error") { - t.Errorf("pull error %q calls the command's failure a network error", err) - } - if seen := api.requests(); len(seen) != 0 { - t.Errorf("%d requests reached the API without a token", len(seen)) - } -} - -func TestACommandPrintingNothingIsAFailure(t *testing.T) { - script := writeTokenScript(t, t.TempDir(), "exit 0\n") - api := newBearerAPI(t, "Bearer tok-1") - - err := sync.New(openTestDB(t), api.URL, script).Pull(t.Context()) - - var tokenErr *sync.TokenCommandError - if !errors.As(err, &tokenErr) || tokenErr.Reason != "printed no token" { - t.Fatalf("pull error %v, want a TokenCommandError saying it printed no token", err) - } -} - -// A dev API sets its own identity and needs no header. -func TestNoTokenCommandSendsNoAuthorization(t *testing.T) { - api := newBearerAPI(t, "") - mustPull(t, sync.New(openTestDB(t), api.URL, "")) - - for _, r := range api.requests() { - if r.Authorization != "" { - t.Errorf("%s %s carried %q, want no Authorization header", r.Method, r.Path, r.Authorization) - } - } -} diff --git a/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-item-pane.style b/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-item-pane.style index 3cdd220..262a194 100644 --- a/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-item-pane.style +++ b/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-item-pane.style @@ -8,25 +8,25 @@ 4: 76-76 fg=4 5: 25-25 fg=4 5: 76-76 fg=4 -6: 25-76 fg=4 -7: 25-25 fg=4 -7: 76-76 fg=4 +6: 25-25 fg=4 +6: 76-76 fg=4 +7: 25-76 fg=4 8: 25-25 fg=4 8: 76-76 fg=4 9: 25-25 fg=4 9: 76-76 fg=4 -10: 25-76 fg=4 -11: 25-25 fg=4 -11: 76-76 fg=4 +10: 25-25 fg=4 +10: 76-76 fg=4 +11: 25-76 fg=4 12: 25-25 fg=4 12: 76-76 fg=4 13: 25-25 fg=4 13: 76-76 fg=4 14: 25-25 fg=4 14: 76-76 fg=4 -15: 25-76 fg=4 -16: 25-25 fg=4 -16: 76-76 fg=4 +15: 25-25 fg=4 +15: 76-76 fg=4 +16: 25-76 fg=4 17: 25-25 fg=4 17: 76-76 fg=4 18: 25-25 fg=4 diff --git a/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-item-pane.txt b/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-item-pane.txt index 826cee9..ce36295 100644 --- a/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-item-pane.txt +++ b/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-item-pane.txt @@ -1,3 +1,4 @@ + │ p Manage project membership │ │ b Link dependency (blocked by) │ │ B Unlink dependency │ │ T Dependency browser (map + │ @@ -20,8 +21,7 @@ │ 2 Filter: all + archived │ │ (toggle) │ │ 0 Filter: reset │ - │ R Sync now (runs automatically │ - │ too) │ + │ R Read the data again now │ │ q Ctrl+C Quit │ │ ? Toggle this help │ │ │ diff --git a/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-project-pane.style b/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-project-pane.style index 406fbc2..4ead506 100644 --- a/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-project-pane.style +++ b/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-project-pane.style @@ -8,9 +8,9 @@ 4: 76-76 fg=4 5: 25-25 fg=4 5: 76-76 fg=4 -6: 25-76 fg=4 -7: 25-25 fg=4 -7: 76-76 fg=4 +6: 25-25 fg=4 +6: 76-76 fg=4 +7: 25-76 fg=4 8: 25-25 fg=4 8: 76-76 fg=4 9: 25-25 fg=4 @@ -25,9 +25,9 @@ 13: 76-76 fg=4 14: 25-25 fg=4 14: 76-76 fg=4 -15: 25-76 fg=4 -16: 25-25 fg=4 -16: 76-76 fg=4 +15: 25-25 fg=4 +15: 76-76 fg=4 +16: 25-76 fg=4 17: 25-25 fg=4 17: 76-76 fg=4 18: 25-25 fg=4 diff --git a/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-project-pane.txt b/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-project-pane.txt index f3ac5a1..f3552a4 100644 --- a/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-project-pane.txt +++ b/testdata/TestRealBinaryFlows/help-taller-than-the-window/from-the-project-pane.txt @@ -1,3 +1,4 @@ + │ a Add project │ │ m Move/reorder project │ │ C Show/hide shelved and closed │ │ projects │ @@ -20,8 +21,7 @@ │ 2 Filter: all + archived │ │ (toggle) │ │ 0 Filter: reset │ - │ R Sync now (runs automatically │ - │ too) │ + │ R Read the data again now │ │ q Ctrl+C Quit │ │ ? Toggle this help │ │ │ diff --git a/tui/app.go b/tui/app.go index c644b0b..e6b9e3a 100644 --- a/tui/app.go +++ b/tui/app.go @@ -2,7 +2,6 @@ package tui import ( "cmp" - "context" "fmt" "slices" "strconv" @@ -15,7 +14,6 @@ import ( "github.com/datapointchris/projects/backend" "github.com/datapointchris/projects/model" - "github.com/datapointchris/projects/sync" ) // App is the top-level Bubble Tea model for the TUI. @@ -121,25 +119,18 @@ type App struct { errorMsg string // transient error shown in status bar loading bool // true while an async operation is in-flight - // Sync - syncEngine *sync.Engine - syncStatus sync.SyncStatus - syncInterval time.Duration - where Where } // Where names the data the board shows, for its status line: a database on this -// machine, or the API at APIHost. A database with sync on is named by its engine. +// machine, or the API at APIHost. type Where struct { DatabasePath string APIHost string } -// NewApp creates a new TUI application backed by the given Backend. syncEngine -// is nil unless the database syncs. syncInterval is how often the board rereads -// what it shows: a pull with sync on, and a read of the API in API mode. -func NewApp(b backend.Backend, syncEngine *sync.Engine, where Where, syncInterval time.Duration) *App { +// NewApp creates a new TUI application backed by the given Backend. +func NewApp(b backend.Backend, where Where) *App { ti := textinput.New() ti.CharLimit = 200 ti.SetStyles(inputStyles()) @@ -155,8 +146,6 @@ func NewApp(b backend.Backend, syncEngine *sync.Engine, where Where, syncInterva itemBlockers: make(map[string][]model.ProjectItem), titleInput: ti, depFilter: df, - syncEngine: syncEngine, - syncInterval: syncInterval, where: where, } } @@ -270,18 +259,11 @@ type errMsg struct{ error } // moved flashSeq on, so an older flash's timer leaves it on screen. type clearStatusMsg struct{ seq int } -// Sync messages. A pull carries whether the user asked for it: an automatic one -// runs every syncInterval, so announcing it would flash the status bar forever -// and a transient failure would pin an error banner over a working app. -type ( - syncStatusMsg sync.SyncStatus - syncPullTickMsg struct{} - syncPullDoneMsg struct{ manual bool } - syncPullErrMsg struct { - error - manual bool - } -) +// rereadTickMsg reads the API again, since another client may have written. +type rereadTickMsg struct{} + +// apiRereadInterval is how stale the board may grow through an API. +const apiRereadInterval = 2 * time.Minute const statusFlashDuration = 3 * time.Second @@ -295,9 +277,8 @@ func (m *App) flash(msg string) tea.Cmd { } // itemHandle names an item the way it is typed on the command line: its number, -// or the UUID tail while it is still waiting for one. An item created here -// while the API was unreachable has no number until its create is pushed, and -// showing nothing in that window would leave the row unnameable. +// or the UUID tail for an item that has none, which a database once kept as a +// copy of an API may still hold. func itemHandle(item model.ProjectItem) string { if item.Number != nil { return strconv.Itoa(*item.Number) @@ -327,34 +308,12 @@ func truncate(s string, maxWidth int) string { return string(runes[:maxWidth-1]) + "…" } -// --- Sync commands --- - -func syncPullCmd(e *sync.Engine, manual bool) tea.Cmd { - return func() tea.Msg { - if err := e.Pull(context.Background()); err != nil { - return syncPullErrMsg{error: err, manual: manual} - } - return syncPullDoneMsg{manual: manual} - } -} - -func syncStatusTickCmd(e *sync.Engine) tea.Cmd { - return tea.Tick(2*time.Second, func(_ time.Time) tea.Msg { - return syncStatusMsg(e.Status()) - }) -} - -func syncPullTickCmd(d time.Duration) tea.Cmd { - return tea.Tick(d, func(_ time.Time) tea.Msg { - return syncPullTickMsg{} +func rereadTickCmd() tea.Cmd { + return tea.Tick(apiRereadInterval, func(_ time.Time) tea.Msg { + return rereadTickMsg{} }) } -// safeToAutoPull reports whether a background reconcile can run without pulling -// the ground out from under the user. A pull rewrites items, memberships and -// ordering wholesale, which would corrupt a grab in progress or refresh a -// reader into a snapshot of something else. Every other mode defers to -// the next tick rather than skipping the pull outright. // projectStatusFilter is what the project pane is currently asking for. Closing // a project is what takes it out of the list, so the default hides the terminal // ones and `C` is how you get at them. @@ -365,7 +324,11 @@ func (m *App) projectStatusFilter() string { return model.StatusActive } -func (m *App) safeToAutoPull() bool { +// safeToReread reports whether a background read can land without pulling the +// ground out from under the user. A read replaces the lists and their order, +// which would corrupt a grab in progress or refresh a reader into a snapshot of +// something else. Every other mode defers to the next tick. +func (m *App) safeToReread() bool { return m.appMode == modeNormal } @@ -993,16 +956,8 @@ func (m *App) groupHeaderAt(idx int) string { } func (m *App) Init() tea.Cmd { - if m.syncEngine != nil { - return tea.Batch( - fetchProjectsCmd(m.backend, m.projectStatusFilter()), - syncPullCmd(m.syncEngine, false), - syncStatusTickCmd(m.syncEngine), - syncPullTickCmd(m.syncInterval), - ) - } if m.where.APIHost != "" { - return tea.Batch(fetchProjectsCmd(m.backend, m.projectStatusFilter()), syncPullTickCmd(m.syncInterval)) + return tea.Batch(fetchProjectsCmd(m.backend, m.projectStatusFilter()), rereadTickCmd()) } return fetchProjectsCmd(m.backend, m.projectStatusFilter()) } @@ -1252,40 +1207,14 @@ func (m *App) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.errorMsg = msg.Error() return m, nil - case syncPullTickMsg: - next := syncPullTickCmd(m.syncInterval) - switch { - case !m.safeToAutoPull(): + case rereadTickMsg: + // A tick re-arms even when it skips the read. One that returned nothing + // would end the rereads for the rest of the session. + next := rereadTickCmd() + if !m.safeToReread() { return m, next - case m.syncEngine != nil: - return m, tea.Batch(syncPullCmd(m.syncEngine, false), next) - case m.where.APIHost != "": - // Another client may have written since the last read. - return m, tea.Batch(fetchProjectsCmd(m.backend, m.projectStatusFilter()), next) - } - return m, next - - case syncPullDoneMsg: - if !msg.manual { - return m, fetchProjectsCmd(m.backend, m.projectStatusFilter()) - } - return m, tea.Batch(fetchProjectsCmd(m.backend, m.projectStatusFilter()), m.flash("Synced with server")) - - case syncPullErrMsg: - // An automatic pull failing is not an error the user did anything to - // cause. The status bar already reads SYNC ERR off the engine status; - // hijacking the bar every interval would bury real messages. - if msg.manual { - m.errorMsg = fmt.Sprintf("Sync: %v", msg.error) } - return m, nil - - case syncStatusMsg: - m.syncStatus = sync.SyncStatus(msg) - if m.syncEngine != nil { - return m, syncStatusTickCmd(m.syncEngine) - } - return m, nil + return m, tea.Batch(fetchProjectsCmd(m.backend, m.projectStatusFilter()), next) } return m, nil @@ -1626,14 +1555,11 @@ func (m *App) handleNormalKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { // --- Phase 6: Advanced --- case "R": - switch { - case m.syncEngine != nil: - return m, syncPullCmd(m.syncEngine, true) - case m.where.APIHost != "": - return m, tea.Batch(fetchProjectsCmd(m.backend, m.projectStatusFilter()), m.flash("Read again from "+m.where.APIHost)) + source := m.where.APIHost + if source == "" { + source = m.where.DatabasePath } - disabled := m.flash("Sync is disabled — running local-only") - return m, disabled + return m, tea.Batch(fetchProjectsCmd(m.backend, m.projectStatusFilter()), m.flash("Read again from "+source)) case "?": m.appMode = modeHelp @@ -2581,7 +2507,7 @@ func (m *App) renderHelpOverlay() string { 1 Filter: blocked only (toggle) 2 Filter: all + archived (toggle) 0 Filter: reset - R Sync now (runs automatically too) + R Read the data again now q Ctrl+C Quit ? Toggle this help` @@ -3324,19 +3250,6 @@ func clipHints(hints string, maxWidth int) string { func (m *App) renderStatusBar() string { var modeStr string switch { - case m.syncEngine != nil: - target := dimStyle.Render(" " + m.syncEngine.APIURL()) - s := m.syncStatus - switch { - case s.Syncing: - modeStr = syncingStyle.Render("SYNCING...") + target - case s.LastError != "": - modeStr = syncErrorStyle.Render("SYNC ERR") + target - case s.PendingCount > 0: - modeStr = syncPendingStyle.Render(fmt.Sprintf("PENDING (%d)", s.PendingCount)) + target - default: - modeStr = syncOKStyle.Render("SYNCED") + target - } case m.where.APIHost != "": modeStr = modeAPIStyle.Render("API") + dimStyle.Render(" "+m.where.APIHost) default: @@ -3365,7 +3278,7 @@ func (m *App) renderStatusBar() string { // rather than wrapped, and truncated before styling so no escape // sequence is cut in half. The 5 is the two spaces this function adds, // the style's own left and right padding, and the one column of gap - // that always separates the hints from the sync status. + // that always separates the hints from the mode on the right. left = clipHints(m.statusBarHints(), m.width-modeWidth-5) } diff --git a/tui/app_test.go b/tui/app_test.go index c7ac2dc..a525b26 100644 --- a/tui/app_test.go +++ b/tui/app_test.go @@ -1,14 +1,9 @@ package tui import ( - "encoding/json" - "errors" - "net/http" - "net/http/httptest" "slices" "strconv" "strings" - goSync "sync" "testing" "time" @@ -18,7 +13,6 @@ import ( "github.com/datapointchris/projects/model" "github.com/datapointchris/projects/service" "github.com/datapointchris/projects/store/sqlite" - "github.com/datapointchris/projects/sync" ) // newTestApp returns an app already showing one project's items, sized to @@ -44,9 +38,7 @@ func newTestAppWithItems(t *testing.T, width, height int, inputs []model.CreateP } t.Cleanup(func() { _ = database.Close() }) - // AssigningItemNumbers because the harness passes a nil sync engine, which - // is the sync-off case: nothing upstream will number these items. - b := service.New(sqlite.New(database), service.AssigningItemNumbers()) + b := service.New(sqlite.New(database)) project, err := b.CreateProject(model.CreateProject{Name: "work"}) if err != nil { t.Fatalf("creating project: %v", err) @@ -58,7 +50,7 @@ func newTestAppWithItems(t *testing.T, width, height int, inputs []model.CreateP } } - app := NewApp(b, nil, Where{DatabasePath: ":memory:"}, 2*time.Minute) + app := NewApp(b, Where{DatabasePath: ":memory:"}) send(t, app, tea.WindowSizeMsg{Width: width, Height: height}) send(t, app, app.Init()()) send(t, app, app.fetchItems()()) @@ -98,7 +90,7 @@ func drain(t *testing.T, app *App, cmd tea.Cmd, depth int) { } // runCmd abandons a command that does not produce a message promptly — -// flash and sync-status commands are tea.Tick timers that would otherwise +// flash and reread commands are tea.Tick timers that would otherwise // block the test for their full duration. func runCmd(cmd tea.Cmd) tea.Msg { result := make(chan tea.Msg, 1) @@ -289,53 +281,37 @@ func TestCreatedTaskAppearsAsRow(t *testing.T) { } } -// The auto-pull timer re-arms itself from its own message. A tick that returns -// no command ends background sync for the rest of the session, so the skipped -// ticks have to re-arm too — otherwise opening one modal at the wrong moment -// silently reverts the tool to manual syncing. -func TestSyncTickRearmsEvenWhenItSkipsThePull(t *testing.T) { - app := newTestApp(t, 80, 24, "Alpha") - - for _, mode := range []appMode{modeNormal, modeAddItem, modeMove, modeHelp} { - app.appMode = mode - _, cmd := app.Update(syncPullTickMsg{}) - if cmd == nil { - t.Errorf("tick in mode %v returned no command: the sync timer is now dead", mode) - } +// In API mode a tick reads the API again. One arriving mid-move waits for the +// next tick, because a read replaces the rows being moved. It still re-arms, or +// the board stops reading the API for the rest of the session. +func TestAnAPIRereadWaitsOutAMoveAndLandsOnTheNextTick(t *testing.T) { + app := newTestApp(t, 80, 24, "here at launch") + app.where = Where{APIHost: "projects.example.com"} + projects, err := app.backend.ListProjectsByStatus(model.StatusActive) + if err != nil || len(projects) == 0 { + t.Fatalf("listing projects: %v", err) } -} - -// safeToAutoPull gates the reconcile, not the timer. A pull rewrites items and -// ordering wholesale, which would move the ground under a grab or a text entry. -func TestAutoPullOnlyRunsInNormalMode(t *testing.T) { - app := newTestApp(t, 80, 24, "Alpha") - - app.appMode = modeNormal - if !app.safeToAutoPull() { - t.Error("normal mode must allow the background reconcile") + if _, err := app.backend.CreateItem(model.CreateProjectItem{Title: "written elsewhere", ProjectIDs: []string{projects[0].ID}}); err != nil { + t.Fatal(err) } - for _, mode := range []appMode{modeAddItem, modeExternalEdit, modeMove, modeMoveProject, modeReader} { - app.appMode = mode - if app.safeToAutoPull() { - t.Errorf("mode %v must defer the reconcile to the next tick", mode) - } + shown := func() bool { + return slices.ContainsFunc(app.items, func(item model.ProjectItemInProject) bool { return item.Title == "written elsewhere" }) } -} - -// An automatic pull lands every syncInterval. Announcing it would keep the -// status bar permanently flashing and bury the messages the user acted to see. -func TestAutomaticPullIsSilentAndManualPullIsNot(t *testing.T) { - app := newTestApp(t, 80, 24, "Alpha") - app.statusMsg = "" - send(t, app, syncPullDoneMsg{manual: false}) - if app.statusMsg != "" { - t.Errorf("automatic pull announced itself: %q", app.statusMsg) + app.appMode = modeMove + _, cmd := app.Update(rereadTickMsg{}) + if cmd == nil { + t.Fatal("a tick during a move returned no command, so the rereads stopped") + } + drain(t, app, cmd, 16) + if shown() { + t.Fatal("a tick during a move read the API under the rows being moved") } - send(t, app, syncPullDoneMsg{manual: true}) - if app.statusMsg == "" { - t.Error("a pull the user asked for should confirm it happened") + app.appMode = modeNormal + send(t, app, rereadTickMsg{}) + if !shown() { + t.Error("the tick after the move did not read the API") } } @@ -359,101 +335,6 @@ func TestOlderFlashTimerLeavesNewerFlashShowing(t *testing.T) { } } -// A failing automatic pull is not something the user did. The status bar already -// reads SYNC ERR off the engine; pinning an error banner every interval would -// make a briefly unreachable API look like a broken app. -func TestAutomaticPullFailureDoesNotHijackTheStatusBar(t *testing.T) { - app := newTestApp(t, 80, 24, "Alpha") - - send(t, app, syncPullErrMsg{error: errors.New("connection refused"), manual: false}) - if app.errorMsg != "" { - t.Errorf("automatic pull failure took over the status bar: %q", app.errorMsg) - } - - send(t, app, syncPullErrMsg{error: errors.New("connection refused"), manual: true}) - if app.errorMsg == "" { - t.Error("a pull the user asked for must report why it failed") - } -} - -// End-to-end for the background timer: a tick in normal mode must actually -// reconcile with the server and re-arm, and a tick in a modal must do neither -// the pull nor lose the timer. This is the behavior that removes the need to -// drop out of the TUI and run `projects sync` by hand. -func TestSyncTickReconcilesWithTheServer(t *testing.T) { - var mu goSync.Mutex - var hits int - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - mu.Lock() - hits++ - mu.Unlock() - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/api/v1/projects": - _ = json.NewEncoder(w).Encode([]model.Project{ - {ID: "proj-1", Name: "Remote", Position: 0, CreatedAt: time.Now()}, - }) - case "/api/v1/project-items": - _ = json.NewEncoder(w).Encode([]model.ProjectItem{ - {ID: "01900000-0000-7000-8000-00000000a001", Title: "Added Elsewhere", CreatedAt: time.Now(), UpdatedAt: time.Now()}, - }) - case "/api/v1/project-items/01900000-0000-7000-8000-00000000a001": - _ = json.NewEncoder(w).Encode(model.ProjectItemDetail{ - ProjectItem: model.ProjectItem{ID: "01900000-0000-7000-8000-00000000a001", Title: "Added Elsewhere", CreatedAt: time.Now(), UpdatedAt: time.Now()}, - Projects: []model.Project{{ID: "proj-1", Name: "Remote"}}, - }) - default: - _ = json.NewEncoder(w).Encode([]model.ProjectItemTask{}) - } - }) - ts := httptest.NewServer(handler) - t.Cleanup(ts.Close) - - database, err := sqlite.Open(":memory:") - if err != nil { - t.Fatalf("opening test db: %v", err) - } - t.Cleanup(func() { _ = database.Close() }) - - engine := sync.New(database, ts.URL, "") - t.Cleanup(engine.Stop) - app := NewApp(service.New(sqlite.New(database)), engine, Where{DatabasePath: ":memory:"}, 20*time.Millisecond) - send(t, app, tea.WindowSizeMsg{Width: 80, Height: 24}) - - countHits := func() int { - mu.Lock() - defer mu.Unlock() - return hits - } - - app.appMode = modeAddItem - send(t, app, syncPullTickMsg{}) - if countHits() != 0 { - t.Fatalf("a tick during text entry reconciled anyway: %d requests", countHits()) - } - - app.appMode = modeNormal - send(t, app, syncPullTickMsg{}) - if countHits() == 0 { - t.Fatal("a tick in normal mode never reached the server") - } - - // An item that only ever existed on the server has to land in the TUI - // without the user asking for it. - if _, err := app.backend.GetItem("01900000-0000-7000-8000-00000000a001"); err != nil { - t.Fatalf("background pull did not bring the remote item local: %v", err) - } - found := false - for _, p := range app.projects { - if p.Name == "Remote" { - found = true - } - } - if !found { - t.Errorf("pulled project never reached the rendered model: %v", app.projects) - } -} - // The row is where the handle has to be legible: the id column was 8 characters // of hex tail, which resolves but which nobody can read back off the screen. func TestItemRowShowsTheNumber(t *testing.T) { @@ -467,7 +348,7 @@ func TestItemRowShowsTheNumber(t *testing.T) { } for _, item := range items { if item.Number == nil { - t.Fatalf("item %q has no number in a sync-off database", item.Title) + t.Fatalf("item %q has no number in a database", item.Title) } if !strings.Contains(view, strconv.Itoa(*item.Number)) { t.Errorf("view is missing the handle %d for %q", *item.Number, item.Title) diff --git a/tui/editor.go b/tui/editor.go index b1a2864..3700146 100644 --- a/tui/editor.go +++ b/tui/editor.go @@ -80,7 +80,7 @@ func projectDescriptionFileName(p model.Project) string { // openInEditor hands the terminal to the editor on a temporary file holding // text. The app sits in modeExternalEdit meanwhile, which keeps the background -// pull from rewriting the row the save is about to update. +// read from replacing the row the save is about to update. func (m *App) openInEditor(target editTarget, fileName, text string) tea.Cmd { dir, err := os.MkdirTemp("", "projects-") if err != nil { diff --git a/tui/realdata_test.go b/tui/realdata_test.go index 26c2e2f..6d36a0d 100644 --- a/tui/realdata_test.go +++ b/tui/realdata_test.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "testing" - "time" tea "charm.land/bubbletea/v2" @@ -41,7 +40,7 @@ func TestRealDatabaseSurvivesEverySizeAndProject(t *testing.T) { for _, size := range sizes { width, height := size[0], size[1] - app := NewApp(service.New(sqlite.New(database)), nil, Where{DatabasePath: path}, 2*time.Minute) + app := NewApp(service.New(sqlite.New(database)), Where{DatabasePath: path}) send(t, app, tea.WindowSizeMsg{Width: width, Height: height}) send(t, app, app.Init()()) app.activePane = itemPane diff --git a/tui/styles.go b/tui/styles.go index 911e327..6ba07fe 100644 --- a/tui/styles.go +++ b/tui/styles.go @@ -122,18 +122,6 @@ var ( modeAPIStyle = lipgloss.NewStyle(). Foreground(lipgloss.Color("4")). Bold(true) - syncOKStyle = lipgloss.NewStyle(). - Foreground(lipgloss.Color("2")). - Bold(true) - syncingStyle = lipgloss.NewStyle(). - Foreground(lipgloss.Color("3")). - Bold(true) - syncPendingStyle = lipgloss.NewStyle(). - Foreground(lipgloss.Color("3")). - Bold(true) - syncErrorStyle = lipgloss.NewStyle(). - Foreground(lipgloss.Color("1")). - Bold(true) // Titles paneTitleStyle = lipgloss.NewStyle().