CEXT-6160: store commerce instance on association and expose retrieval helpers#511
CEXT-6160: store commerce instance on association and expose retrieval helpers#511vinayrao2000 wants to merge 30 commits into
Conversation
Drop the Associated prefix from the public helpers per Daniel's review feedback - cleaner, more discoverable API.
… CEXT-6160-commerce-system-config-on-association
Per Ivan's review feedback - 'not associated' is exceptional state, throwing keeps happy-path code clean without forcing null checks at every call site.
Per Daniel's review feedback - puts SDK-managed config under a system.* namespace, cleanly separated from user-defined configuration.* keys.
lib-config now exposes generic system config primitives (setSystemConfigByKey/getSystemConfigByKey), and lib-app has the typed association wrappers on top. Drops the delete operation - clearing is done by setting null.
Typed association helpers (setAssociationData, getAssociationData, clearAssociationData) live in lib-app per the layered design, not lib-config. lib-config only exposes the generic system config primitives.
lib-config provides persistence via lib-files with lib-state as a performance cache. Cache expiry triggers automatic re-fetch from files, so no TTL refresh logic is needed in our code.
Adds setSystemConfigByKey / getSystemConfigByKey primitives under modules/configuration/system/. Uses lib-files for persistent storage with lib-state as a performance cache. Passing null to set clears the entry. Exposed via the new @adobe/aio-commerce-lib-config/system subpath export. Domain-agnostic - operates purely on opaque keys and values. Will be used by lib-app's association module via the public subpath export.
- AppNotAssociatedError class (extends CommerceSdkErrorBase) - Internal association module: setAssociationData / getAssociationData / clearAssociationData (calls into lib-config system submodule) - Public root entrypoint: getCommerceInstance and getCommerceClient helpers, both throw AppNotAssociatedError when no data is stored - Add root export to package.json and tsdown.config.ts
- New association runtime action with POST / and DELETE / handlers,
protected with require-adobe-auth: true
- POST / accepts { commerceBaseUrl, commerceEnv } and stores it via
the internal association module
- DELETE / clears the stored data
- Add association.js.template for app scaffolding
- Update buildAppManagementExtConfig to deploy the association action
alongside app-config (always deployed, no feature gating)
- Update OpenAPI spec with the new POST /association and DELETE /association
routes, bump info.version to 1.1.0
- Update test fixtures and config test expectations
- Unit tests for AppNotAssociatedError, association repository, public helpers (getCommerceInstance / getCommerceClient), and the association runtime action handlers (POST / and DELETE /) - changesets for minor bumps in lib-app and lib-config - Document the new helpers in lib-app usage.md
🦋 Changeset detectedLatest commit: 0a4de5c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| const logger = getLogger("@adobe/aio-commerce-lib-config:system-repository"); | ||
| try { | ||
| const state = await getSharedState(); | ||
| await state.put(key, payload); |
There was a problem hiding this comment.
Shouldn't we add a TTL here? As I know a default value is 24 hours
There was a problem hiding this comment.
Good question. This is intentional, state.put without a ttl applies the service default of 24h, which is the behavior we want for the cache layer. Per the storage design in the spec, lib-state is a TTL-backed performance cache sitting on top of lib-files as the no-TTL source of truth: when a cached entry expires, the read path falls back to files and re-caches transparently, so no data is ever lost on expiry. Since the system config changes only on associate/unassociate, the occasional post-expiry re-read from files is negligible.
Happy to make the 24h explicit ({ ttl: 86400 }) if you'd prefer it documented in code rather than relying on the service default, it's a no-op behaviorally, but the current implementation already matches the intended caching design.
| "enum": ["saas", "paas"] | ||
| } | ||
| }, | ||
| "required": ["baseUrl", "env"] |
There was a problem hiding this comment.
Is there a reason why the response body is different from the request body? I expected to see commerceBaseUrl or baseUrl in both.
There was a problem hiding this comment.
There's no real reason for this. It wasn't intentional, the response was echoing the internal stored shape (baseUrl/env) instead of mirroring the request. Do you want me to change them so the endpoint uses commerceBaseUrl/commerceEnv on both the request and the response.
| "title": "App Management API", | ||
| "description": "REST API for managing Adobe Commerce App Builder apps: resolving app configuration, managing configuration values, running the installation and uninstallation lifecycle, exposing the Admin UI SDK registration, and managing the scope tree.", | ||
| "version": "1.1.0", | ||
| "version": "1.1.1", |
There was a problem hiding this comment.
should be minor instead of patch
| "The 'commerceBaseUrl' field must be a valid absolute URL (e.g., 'https://my-store.example.com')", | ||
| ), | ||
| ), | ||
| commerceEnv: v.picklist(["saas", "paas"]), |
There was a problem hiding this comment.
didn't we used flavour in other parts of the sdk? we should align this
There was a problem hiding this comment.
system-repository.ts reimplements the two-layer cache pattern from configuration-repository.ts: read from state, fall back to files, re-cache, swallow cache errors. ~150 lines plus ~164 lines of tests for logic that's already there.
The only real difference between system config and regular config is that system config doesn't participate in scope-tree inheritance. But that inheritance logic lives above the repository layer, not within it, so there's nothing in configuration-repository.ts that would bleed into it. A deleteConfig (needed here regardless) plus calling loadConfig/persistConfig/deleteConfig with a fixed key would've been enough. getSystemConfigByKey/setSystemConfigByKey could then just live on the root entrypoint, which also eliminates the need for the new ./system subpath, the barrel file, and the extra tsdown entry.
There was a problem hiding this comment.
Agreed the two-layer mechanics are duplicated, and the scope-inheritance that differs lives above the repo layer so it wouldn't bleed in. But reusing configuration-repository's loadConfig/persistConfig drags in its quirks, the stringify({ data }) double-wrap, the files.list pre-check I removed from system-repository earlier in this review, and the configuration.* namespace the spec deliberately keeps separate from system.*. So I don't think coupling to it is the right de-dupe. Same root concern @obarcelonap raised about createCombinedStore; the real fix that satisfies both is moving both repos onto one shared store bigger change touching shared infra + an existing consumer,
There was a problem hiding this comment.
If the logic is the same, then whatever can be fixed in one can be fixed in the other. If your code optimizes/fixes the pre-check of files.list we can also apply it in the configuration repository and still re-use the logic, while also optimizing other consumers of the config repository at the same time (double win). Not sure what "double-wrap of stringify" means, but pretty sure it can be fixed instead of just duplicated.
and the configuration.* namespace the spec deliberately keeps separate from system.*. So I don't think coupling to it is the right de-dupe.
I am not talking about re-using the same namespace; you can still just use different namespaces (just duplicate the constant namespace from the repository). It's just about reusing logic that is already implemented. Adding/duplicating all that code for me doesn't make much sense and adds a lot of maintenance overhead that can be avoided.
The concern raised by @obarcelonap about createCombinedStore is correct, but I don't think this belongs in this PR. It's different because you're adding unnecessary logic, while his concern would require modifying working code, which is better done separately from this task.
If your concern is about semantics (using the configuration repository for "system" config), just rename things, but duplicating for the sake of duplicating, I don't think it's the solution.
There was a problem hiding this comment.
Makes sense, happy to go this route. Just flagging it deviates from the spec (which describes a separate system/ submodule + ./system subpath), so I'd update the spec to match.
Plan: drop the system/ submodule, ./system subpath, barrel, and tsdown entry. getSystemConfigByKey/setSystemConfigByKey would build on loadConfig/persistConfig + a new deleteConfig (parameterized by a system.* namespace) and export from the package root. Behavior stays the same.
| const DEFAULT_MESSAGE = | ||
| "App is not associated with a Commerce instance. Re-associate the app to resolve this error."; | ||
|
|
||
| type AppNotAssociatedErrorOptions = ErrorOptions & { |
There was a problem hiding this comment.
AppNotAssociatedErrorOptions is ErrorOptions & { traceId?: string } — identical to CommerceSdkErrorBaseOptions from lib-core. Just use that directly instead of re-declaring it.
There was a problem hiding this comment.
Done, removed the local AppNotAssociatedErrorOptions and use CommerceSdkErrorBaseOptions directly.
Fixed in 8b61c1e
There was a problem hiding this comment.
I would still keep the custom error options for better future extensibility and avoiding type contract changes. But you only need to keep it empty, not redeclare traceId.
| */ | ||
|
|
||
| // biome-ignore lint/performance/noBarrelFile: export as part of the Public API | ||
| export { |
There was a problem hiding this comment.
This lives under source/modules/association/ but there's already source/management/ with installation/ inside it. Association is a management lifecycle concern (triggered by App Management on associate/unassociate), so management/association/ would be consistent.
There was a problem hiding this comment.
Good point on the single-occupant modules/ folder. My only hesitation: management/ backs the ./management subpath of install-time tooling, while association's public API (getCommerceInstance/getCommerceClient) is general-purpose runtime access exported from root, so I grouped it as a "module" rather than management tooling. The store/clear lifecycle part already lives in actions/association/.
Please suggest, happy to move it to management/association/ if you think it fits better there.
There was a problem hiding this comment.
Valid concern, what I would argue then is that maybe then association is not a correct name at all, because association is indeed part of the management lifecycle.
| * ``` | ||
| */ | ||
| export async function getCommerceInstance( | ||
| _params: RuntimeActionParams, |
There was a problem hiding this comment.
_params is accepted but never used — getAssociationData() reads from storage without needing credentials from params. If the intent is API consistency with getCommerceClient, the signature makes sense. If not, the parameter shouldn't be there.
There was a problem hiding this comment.
It's unused today, agreed. The intent is API symmetry: getCommerceInstance and getCommerceClient share the same (params) shape so callers can swap between "give me the instance" and "give me a ready client" without changing the call. getCommerceClient needs params for auth; getCommerceInstance takes it only for that symmetry, hence the _ prefix.
Please suggest you still want to remove _param
There was a problem hiding this comment.
It should be removed; we don't do this kind of symmetry anywhere in the libraries. If a param is not used, it doesn't make sense to me to ask for it. What I would say is that maybe getCommerceClient should not take params at all. Because then it's doing multiple things, it's resolving params into the client options and also creating the client.
Helpers like this should be composable. Maybe resolving should happen outside, and the resolved params should be used as input. Or if the getCommerceClient only needs the auth part to be resolved because the url and the flavor are given by getCommerceInstance then that should be what's resolved outside (and properly documented)
Description
Adds a new feature that stores the Commerce instance details (Base URL + deployment type) when an app is associated with a Commerce instance, and exposes typed async helpers so any runtime action can call Commerce APIs without custom storage setup.
Two new helpers in
@adobe/aio-commerce-lib-app:getCommerceInstance(params){ baseUrl, env }getCommerceClient(params)AdobeCommerceHttpClient(ready to call APIs)Both throw
AppNotAssociatedErrorif the app isn't currently associated.Storage (via new
./systemsubmodule in@adobe/aio-commerce-lib-config):getSystemConfigByKey/setSystemConfigByKeyAPI — reusable for other "system-level" config beyond associationsystem.associationfor this featureNew runtime action auto-scaffolded into apps:
POST /association— store{ commerceBaseUrl, commerceEnv }(called by App Management UI on associate)DELETE /association— clear stored data (called on unassociate)Related Issue
CEXT-6160
Motivation and Context
Before this change, every action that wanted to call Commerce APIs had to:
paramsAdobeCommerceHttpClientThis pushed boilerplate and inconsistent storage patterns into every app. Customers had no standard way to know which Commerce instance their app was associated with at runtime.
After this change, one line in any action gets a configured client:
The SDK owns where association data is stored (state + files), how it's read, and how it propagates from the association event to runtime actions.
How Has This Been Tested?
Unit tests (~1,865 tests passing across the monorepo):
lib-config:system-repository.test.tscovers set/get/clear, state-as-cache + files-as-source-of-truth, null clears, type safetylib-app:app-not-associated-error.test.ts— class, message, optionsassociation-repository.test.ts— round-trip via system configactions/association.test.ts— router (POST/DELETE), validation, logger middlewareindex.test.ts—getCommerceInstanceandgetCommerceClienthappy + error pathsTypecheck: clean across all 13 packages (
pnpm typecheck)Lint: clean (
pnpm check:ci)E2E (in
commerce-app-purchase-approvaldemo app):POST .../association→ 200 OKcommerce-config-demoaction →{ associated: true, instance: { baseUrl, env }, clientType: "AdobeCommerceHttpClient" }DELETE .../association→ 204 No Content{ associated: false, ... }Both helpers verified working end-to-end through deployed actions.
Types of changes
Checklist:
Changesets
aio-commerce-lib-app— minor (new helpers + new action; additive only)aio-commerce-lib-config— minor (new./systemsubpath export; additive only)Affected packages
@adobe/aio-commerce-lib-appgetCommerceInstance,getCommerceClient,AppNotAssociatedError, association runtime action + scaffolding template@adobe/aio-commerce-lib-config./systemsubpath export withgetSystemConfigByKey/setSystemConfigByKey