[FEATURE] KeyValueStorage: Introduce Unified API for Application State#11650
Open
mjansenDatabay wants to merge 3 commits into
Open
[FEATURE] KeyValueStorage: Introduce Unified API for Application State#11650mjansenDatabay wants to merge 3 commits into
mjansenDatabay wants to merge 3 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
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
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.
Provenance and AI Usage
This approach builds on a former cache implementation by @mjansenDatabay, which
had been developed in a plugin, and on discussions around several UI / Removing of
Legacy-UIComponents-Service and Table (LUI) topics between @thibsy, @oliversamoila,
@fhelfer and @mjansenDatabay. @thibsy provided important feedback and impulses throughout.
The implementation in this PR was produced with AI assistance; the tool
operated solely on the architectural direction and design decisions of the contributors mentioned
above — not as an independent author of the architecture.
(Important) decisions are documented based on ADRs.
Summary
This PR introduces a dedicated component for namespace-scoped key-value storage of
application state — together with two backends contributed by
their natural owners. Each backend defines its own lifetime: the transient backend
binds state to the current session, the persistent backend retains it across sessions
until explicitly changed or cleared.
It deliberately carves out a niche distinct from both
ilSettingandILIAS\Cache.Rather than restating the reasoning here, please read
components/ILIAS/KeyValueStorage/README.md,which covers the API surface, the contract for consumers and backend
providers, and — most relevantly for review — the sections Choosing the Right
Storage (delimitation against
ilSettingandILIAS\Cache) and the ADR on thelifetime taxonomy.
The 3 Base Changes
1) The
KeyValueStorageComponentDefines the consumer-facing
Factory/Storages/Storageinterfaces, thecontributor-facing
StoragePort/StorageProvidercontracts, value objects(
StorageNamespace, key validation, JSON-only value codec), and the wiring. Thecomponent does not persist anything itself — it defines ports and seeks
StorageProvidercontributions. Lifetime is selected throughFactory::transient()/Factory::persistent(). Request-scoped (first-level) caching is an explicit, opt-in decoratorvia
Storages::requestCached(), not implicit behaviour. Full rationale and diagramsare in the README.
2) Transient backend —
AuthenticationThe
Authenticationcomponent contributes the transient backend, backed by the ILIASsession. Session storage is the natural fit for state that must not survive logout.
The storage layout (flat per-key session entries rather than a single nested bucket) and
its concurrency rationale are documented as an ADR in
components/ILIAS/Authentication/README.md.3) Persistent backend —
DatabaseThe
Databasecomponent contributes the persistent backend plus the schema for the greenfieldil_kv_storagetable. The database connection is resolved lazily so the port can be constructedduring build/bootstrap (
composer du,composer install, orphp cli/setup.php ...) where$DICis not yet available. Schema and connection decisions are recorded as ADRs incomponents/ILIAS/Database/README.md.UI Storage: No Behavioural Change
UI\Storagecontinues to be served by the transient backend, exactly as beforethis PR. The existing adapter in
Authenticationnow obtains its storage throughFactory::transient()->requestCached(...), which preserves the prior session-backedsemantics. This is intentional: the question of whether UI view-control
state (sorting, filters, current page) should become persistent across login
sessions is a separate product/architecture discussion and is explicitly out of scope
here. Moving the UI adapter into the UI component is likewise deferred to a follow-up
PR.
Important: The UI component and its users must ensure valid keys are passed to
KeyValueStorage. This is currently not the case: some UI > Table > Dataimplementations already pass PHP namespaces (with
\characters) as storage keys, whichviolates the key rules of the
KeyValueStoragecomponent.The "subject" Problem (for Discussion)
A point worth flagging for reviewers:
KeyValueStoragedoes not attach a subject(user, role, object) to stored values. The transient backend is implicitly per-user
because the session already is; the persistent backend is global —
namespace + keyword → value, no actor dimension. Consequently, any consumer thatwants to persist per-user data (the obvious candidate being UI preferences) must
encode the subject into the namespace or key itself today. This is documented in the
README.
Options we see for handling subject scoping properly, roughly in order of
architectural cleanliness:
CurrentUserabstraction resolvable incomponent wiring). This is the proper long-term fix and is useful well beyond this
component, but there is currently no
$use-able current-user service in the moderncontainer — the user still comes from
$DIC->user()at the composition root.with the current subject, fed by such an identity service. Keeps KeyValueStorage
subject-agnostic and concentrates the policy in one testable place.
authenticated ones) selected per request — this is what actually satisfies a
"survives logout" goal while avoiding junk rows for anonymous sessions.
scatters the policy and is easy to get wrong.
We looked at how other applications and frameworks expose the current actor for this
kind of scoping. The recurring pattern is an injectable accessor resolved per
request — not the user model itself — for example Symfony's
Security/TokenStorageInterface(getUser()), Spring Security'sSecurityContextHolder,ASP.NET Core's
IHttpContextAccessor(HttpContext.User), and Laravel'sAuthguard(
auth()->user()).The current user is request-scoped and may be absent (anonymous session, CLI, setup),
so it must be fetched on demand rather than bound at construction. ILIAS has no such
$use-able accessor in component wiring today (the user is taken from$DIC->user()at the composition root); introducing one would be the natural way to scope
KeyValueStorage per user without hand-encoding ids.
Tests
Unit tests cover the component, both backends, and the value objects; all green. See
the Tests section of the component README for the breakdown.
Two cross-cutting concerns come with any persistent per-user approach and should not be
overlooked: lifecycle/GC of per-user rows (e.g. clearing a user's namespace on account
deletion), and the GDPR implication that persistent UI preferences become user-linked
personal data.
We'd appreciate input on the preferred directions for the following next steps:
this foundation.
KeyValueStoragecomponent be useful forEnvironment?Environmentin Proposal/12/component revision some components #11430: be a candidate for a further backend of
KeyValueStorage, and if so,how should it be exposed? One idea is selectors such as
app(...)andclient(...)on a similar level totransient()andpersistent(). Of courseKeyValueStorageitself must not know about specific application settings. Also do discuss: Which component will own/contribute the "INI" backend?table state: sort direction and sort column, pagination, filters, row settings,
selection of visible columns, and related view-control persistence. Important: The UI component and it's users has to ensure valid keys are passed to the
KeyValueStoragecomponent. This is currently not the case. There are already UI > Table > Data implementations which result in PHP namespaces (with\characters) are passed to the storage, which violates the rules of theKeyValueStoragecomponent.