feat: per-user JSON UI settings store#151
Merged
Merged
Conversation
Add a simple, extensible per-user settings store for the upcoming web UI (theme/dark mode, default tenant, active FHIR version, recent queries, …). The value is an opaque JSON object, so new settings keys need no schema or code changes — the frontend owns the document shape. Design: - Dedicated `user_settings` table (one opaque JSON document per user), kept separate from the FHIR `resources` table so UI preferences never leak into CapabilityStatement, _history, _search, or $export. Schema migration v10→v11 for both SQLite and PostgreSQL backends. - New `SettingsStore` trait (get/put/patch) in helios-persistence core, plus an RFC 7386 JSON merge-patch helper. Implemented for SQLite and PostgreSQL with transactional read-modify-write and a monotonic `version` for optimistic locking (surfaced as a weak ETag). - Keyed by user only (issuer|subject), tenant-independent; falls back to a fixed `local|default` key when auth is disabled. - REST endpoints GET/PUT/PATCH /_user/settings. The leading-underscore path keeps them authenticated yet exempt from FHIR scope checks and invisible to FHIR machinery. PUT replaces the document; PATCH merge-patches; both honor If-Match, GET honors If-None-Match (304). - Wired into the SQLite and PostgreSQL standalone backends in the hfs binary; other backends report the feature as unavailable (501). Tests: persistence unit tests (merge-patch + SQLite store), PostgreSQL integration tests, REST extractor tests, and an end-to-end HTTP suite covering defaults, round-trip, merge/delete, optimistic locking, validation, and 304. Also refresh the HTS README to reflect completed PostgreSQL backend parity.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Resolve conflicts by taking main as the base and re-applying the per-user UI settings feature on top: - rest/state.rs: keep both AppState fields (user_settings + bulk_submit) - rest/lib.rs: build_app now takes both new params (bulk_submit, settings_store); rename create_app_with_auth_bulk_export_and_settings -> create_app_with_auth_bulk_and_settings so settings-capable backends also wire bulk submit; fix all build_app call arities - hfs/main.rs: SQLite/PostgreSQL now wire bulk export + bulk submit + settings together via the combined builder; drop unused create_app_with_auth_and_bulk_export - persistence schema (sqlite+postgres): renumber user_settings migration to v12->v13 (main took v11->v12 for _contained/bulk-submit); SCHEMA_VERSION = 13 - persistence/postgres_tests.rs: keep both settings and _contained test suites
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…uilders Address Codecov patch-coverage gaps on PR #151: - handlers/user_settings.rs: unit-test parse_object_body (empty/invalid/ non-object), parse_if_match_version (wildcard/unparseable/absent), and the 501-when-no-store branch - sqlite/user_settings.rs: error paths for a corrupt JSON row (decode -> backend_err) and an unparseable stored timestamp (falls back to now) - rest/lib.rs: builder_tests exercising create_app_with_auth_and_bulk and create_app_with_auth_bulk_and_settings (SOF disabled for a side-effect-free router build)
smunini
requested changes
Jul 6, 2026
smunini
left a comment
smunini
left a comment
Contributor
There was a problem hiding this comment.
The design looks good, but this needs to support all of our primary (standalone) storage backends, not just SQLite and PostgreSQL. Right now MongoDB and S3 fall through to 501, and both are first-class standalone primary FHIR stores per HFS_STORAGE_BACKEND (sqlite, postgres, mongodb, s3 — see crates/hfs/src/main.rs).
Blocking:
- MongoDB — a full standalone primary backend (
ResourceStorage,VersionedStorage,SearchProvider,ConditionalStorage, history,BundleProvider, bulk export/submit). Amongodb-only deployment would run a working FHIR server whose UI settings silently 501. It has the transaction/optimistic-locking primitives to implement the same read-modify-write + monotonicversionused in the SQLite/Postgres stores. Please add aSettingsStoreimpl.
Please address or explicitly track:
- S3 — also offered standalone (
HFS_STORAGE_BACKEND=s3, CRUD/versioning/history). Weaker case since its recommended role is Archive and the object-store concurrency/versionstory differs, but amongodb/s3user shouldn't hit an unexplained 501. Either implement it or file a tracked follow-up rather than leaving an implicit 501.
Elasticsearch returning 501 is correct — it's search-only and never a standalone primary, so no action needed there.
Requesting changes for the MongoDB gap.
6 tasks
6 tasks
MongoDB is a full standalone primary FHIR backend, so a mongodb-only deployment must not have its /_user/settings endpoints silently 501. Implement SettingsStore for MongoBackend using a version-conditioned update: read the current document, compute the new one, then persist with a filter pinned to the read version. A concurrent writer that bumped the version makes the update match nothing, which is retried for unconditional writes and surfaced as an OptimisticLockFailure for If-Match writes. A Some(0) precondition maps to an insert whose duplicate-key error (from the new unique index on user_key) becomes the same lock failure. This yields the same read-modify-write + monotonic-version semantics as the SQLite/Postgres stores without requiring multi-document transactions. - schema: unique idx_user_settings_key index; bump SCHEMA_VERSION 4 -> 5, wired into both initialize and migrate paths. - hfs: wire the settings store into start_mongodb and start_mongodb_elasticsearch (the latter sources settings from the underlying Mongo primary behind the composite storage). - tests: MongoDB integration suite mirroring the SQLite/Postgres coverage plus a concurrent-patch test asserting no lost updates. S3 remains unimplemented (its recommended role is Archive and object-store optimistic locking needs conditional PutObject); tracked in #199. The /_user/settings 501 message now names the supported backends and explicitly excludes S3/Elasticsearch so operators get an explained response rather than a bare 501.
Contributor
Author
|
Thanks @smunini. Addressed in fe36ad6:
|
The MongoDB integration suite is gated on HFS_TEST_MONGODB_URL and skips in CI, so the new mongodb/user_settings.rs landed at 0% patch coverage and dragged the PR's Codecov patch report down to ~81%. Provision the settings-store tests from an ephemeral standalone Mongo testcontainer when no external URL is set (preferring the URL when it is), mirroring how the PostgreSQL suite is provisioned. Docker is available in the coverage/test-rust jobs, so these tests now actually run and exercise the SettingsStore impl. The store uses version-conditioned writes rather than multi-document transactions, so a standalone (non replica-set) Mongo is sufficient. If neither a URL nor Docker is available the tests skip, so no environment is hard-broken. - Cargo: enable the testcontainers-modules `mongo` feature (test-only).
The new settings_mongo helper module called MongoBackend::initialize(), which is provided by the core `Backend` trait. That trait is in scope at the top of mongodb_tests.rs but not inside the nested module, so clippy and Test Rust failed to compile the test binary (E0599). Import the trait in the module.
Lift mongodb/user_settings.rs coverage toward its SQLite/Postgres siblings by exercising two more real branches: - Drop the seed in the concurrent-patch test so the racers start from version 0: exactly one wins the initial insert and the rest hit the unique-index duplicate-key path and retry into the version-conditioned update, covering both races in one test. - Add a decode-error test that inserts a row whose `data` is not valid JSON and asserts get_settings surfaces a backend error, mirroring the existing SQLite/PostgreSQL corrupt-row coverage.
This was referenced Jul 6, 2026
This was referenced Jul 6, 2026
smunini
reviewed
Jul 7, 2026
smunini
reviewed
Jul 7, 2026
# Conflicts: # crates/hfs/src/main.rs
… composites The SQLite+Elasticsearch and PostgreSQL+Elasticsearch composite startup paths served their apps through the plain `create_app_with_auth` builder and never passed a settings store, so `/_user/settings` returned 501 even though the SQLite/Postgres primary fully implements `SettingsStore` — the same backend that is settings-capable in the standalone `start_sqlite`/ `start_postgres` paths. Wire the settings store from the underlying primary (mirroring `start_mongodb_elasticsearch`, which already did this from its `mongo` primary) and route both composites through `create_app_with_auth_bulk_and_settings`. Narrow the now-unused import gates: `create_app_with_auth` is only used by the S3 paths (`#[cfg(feature = "s3")]`), and `create_app_with_auth_and_bulk` only by the S3+ES sidecar path (`#[cfg(all(s3, elasticsearch, sqlite))]`). Verified compiling: sqlite; sqlite+es; postgres+es; s3+es+sqlite; s3.
smunini
approved these changes
Jul 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a simple, extensible per-user settings store for the upcoming web UI. Settings like dark mode are just the first of many — the stored value is an opaque JSON object, so new keys need zero schema or code changes (the frontend owns the document shape). Example settings: theme/dark mode, recent FHIR queries by resource type, default tenant, active FHIR version.
The design is hybrid: a server-side store is the source of truth (settings roam across devices), intended to be fronted by a client-side
localStoragecache for instant first paint.Design decisions
user_settingstable/collection, kept separate from the FHIRresourcesstore so private UI prefs never leak intoCapabilityStatement,_history,_search, or$export. Schema migration for SQLite (v12→v13), PostgreSQL (v12→v13), and MongoDB (v4→v5).SettingsStoretrait (get/put/patch) inhelios-persistencecore + an RFC 7386 JSON merge-patch helper. Implemented for SQLite, PostgreSQL, and MongoDB with read-modify-write and a monotonicversionfor optimistic locking (surfaced as a weakETag).issuer|subject), tenant-independent — a setting like "default tenant" is inherently cross-tenant. Falls back to a fixedlocal|defaultkey when auth is disabled.GET/PUT/PATCH /_user/settings. The leading-underscore path keeps them authenticated yet exempt from FHIR scope checks and invisible to FHIR machinery.PUTreplaces;PATCHmerge-patches (great for toggling one key); both honorIf-Match,GEThonorsIf-None-Match(304).GETreturns{}by default so the UI always gets a usable document.Backend coverage
The store is implemented on every standalone primary backend that offers the required read-modify-write + monotonic-version primitives:
SELECT … FOR UPDATEread-modify-writePutObject; recommended role is ArchiveOn unsupported backends the endpoints return
501 Not Implementedwith a message that names the supported backends and explicitly excludes S3/Elasticsearch, so operators get an explained response rather than a bare 501.MongoDB implementation notes
MongoDB standalone deployments don't support multi-document transactions, so rather than a
SELECT … FOR UPDATEthe write path reads the current document, computes the new one, then persists with a filter pinned to the read version. A concurrent writer that bumped the version makes the update match nothing — retried for unconditional writes, surfaced as anOptimisticLockFailureforIf-Matchwrites. ASome(0)"must-not-exist" precondition maps to an insert whose duplicate-key error (from a new uniqueuser_keyindex) becomes the same lock failure. This gives identical semantics to the relational stores without requiring a replica set.API
/_user/settings{}),ETag: W/"{version}"/_user/settings/_user/settingsTests
HFS_TEST_MONGODB_URL): missing→None, put/get/version, patch merge+delete, patch-creates, optimistic lock, and a concurrent-patch test asserting no lost updates.UserKeyextractor unit tests.{}, round-trip, single-key merge, null-delete, staleIf-Match→ 412, matchingIf-Match, non-object body → 400,If-None-Match→ 304.Notes for review
defaultTenantis keyed per-user globally (deliberate — it's cross-tenant). Future per-tenant settings can nest asperTenant: {...}without changing the store key.localStoragecache layer is out of scope here (UI team owns it).