Skip to content

feat(references): add support for remote datasets - #253

Open
nico-mcalley wants to merge 2 commits into
masterfrom
issue/CAAS-2663-support-remote-datasets
Open

nico-mcalley wants to merge 2 commits into
masterfrom
issue/CAAS-2663-support-remote-datasets

Conversation

@nico-mcalley

Copy link
Copy Markdown
Collaborator

No description provided.

@spirit-wiegmann

Copy link
Copy Markdown
Collaborator

Code review

Reviewed the full diff (+1796/−383). Unit tests on the branch pass (326 passed); tsc could not be run locally.

High

src/modules/CaaSMapper.ts:307 — registerReferencedItem no longer resolves the remote locale, breaking custom mappers

The old body did getRemoteConfigForProject(remoteProjectId) and passed the configured locale into unifyId. The new body takes the locale only from the new 5th parameter. registerReferencedItem is still handed to user code via the CustomMapper utils (line 368), where its public signature is (identifier, path, remoteProjectId?).

So a custom mapper calling registerReferencedItem(id, path, 'media-project') registers under media-project#<id>.<sourceLocale> and creates a group with locale: undefined, while FSXARemoteApi.fetchByFilter (line 561) still overrides to remoteConfig.locale and stores the fetched item as media-project#<id>.de_DE. The ids never match, and the reference is left in the payload as a raw [REFERENCED-REMOTE-ITEM-…] string.

Concrete case: app locale en_GB, remotes: { media: { id: 'media-project', locale: 'de_DE' } }, a customMapper registering a remote medium — previously resolved, now silently unresolved.

Medium

src/modules/CaaSMapper.ts:206 — url-derived project unconditionally overrides the inherited project

parsed?.projectId ?? referencedProject ?? inheritedProjectId means the url always wins. If a document's reference urls carry a project id that is neither api.projectID nor in remotes — e.g. content imported/copied from another project, or an app pointed at a CaaS whose documents were generated under a different project id — every such media/dataset reference becomes kind: 'unresolvable' and is left as a raw string, where before it resolved against the local project. Consider falling back to the inherited/local project when the parsed project is unknown, instead of giving up.

src/modules/FSXARemoteApi.ts:1021 — new DUPLICATE_REMOTE_ID throw is an unannounced breaking change

The remotes setter now throws when two entries share an id. Configurations like { mediaEn: { id: 'P', locale: 'en_GB' }, mediaDe: { id: 'P', locale: 'de_DE' } } were previously accepted (and usable via the remoteProject key of fetchElement); after upgrade they throw at construction with no migration path, since the new model permits exactly one locale per project id. At minimum this needs a breaking-change note.

Low

src/modules/FSXARemoteApi.ts:207 — removed typeof locale !== 'string' guard in buildNavigationServiceUrl

The condition is now just !locale.includes('_'). A JS consumer passing a non-string (which the retained locale.toString() in the error message shows was anticipated) now gets TypeError: locale.includes is not a function instead of the logged error plus HttpError(INVALID_LOCALE, 400).

src/modules/CaaSMapper.ts:551 — the PageRef/GCAPage branch of FS_REFERENCE was not migrated to resolveReferenceTarget

It still does entry.value.remoteProject || remoteProjectId and ignores entry.value.url. A PageRef inside a remote document that points at a third project therefore gets referenceRemoteProject set to the containing project, so consumers build the link against the wrong project. This contradicts the rule the README now states ("the project is read from the url") for the rest of the reference types.

src/modules/CaaSMapper.ts:594 — FS_INDEX registers paths with the pre-filter index, then .filter(Boolean)

Pre-existing, but this block is touched. Each record is registered at [...path, index] using the original array index, and nulls are removed afterwards, shifting the array. With a broken record at index 0 and a valid one at index 1, the output array has length 1 (the placeholder at index 0), while denormalizeResolvedReferences' set(resolvedReferences, path, …) writes the resolved dataset to index 1 — leaving an unresolved placeholder at index 0 and an extra element at index 1. Registering after filtering (or not filtering) would fix it.

src/types.ts:762 and src/types.ts:1000 — silent breaking type changes

CustomMapper.registerReferencedItem return type was widened to string | null although no implementation path returns null, and RemoteProjectConfigurationEntry.locale became optional. Both break compilation for consumers that assign the result to string or read remotes[x].locale as string.

README.md, section "Projects that are not configured" — doc contradicts the code

The README states "A reference whose URL points at a different CaaS instance or tenant takes the same path: its project is not configured, so it is not fetched." resolveReferenceTarget only reads parsed.projectId and ignores baseUrl, tenantId and contentMode (all parsed by ReferenceUrlParser but unused), so a url pointing at another host/tenant whose project id happens to match your own or a configured remote is fetched — from your own caasURL/tenant. The no-SSRF claim in the same paragraph holds; the "not fetched" claim does not.

src/modules/FSXARemoteApi.spec.ts (~line 880) — misleading test comment

"the locale configured in 'remotes' is no longer consulted" contradicts FSXARemoteApi.ts:561 and the sibling test "should fetch a configured remote project in its configured locale", which asserts the configured de_DE overrides the requested en_GB.

Non-blocking

  • src/testutils/createDataEntry.ts dropped the remoteProject field from the media fixture entirely, so the referencedProject fallback (media payloads that carry remoteProject but no parsable CaaS url — i.e. older CaaS content) is no longer covered by any test.
  • A large amount of unrelated reformatting (import reordering, JSDoc @param stubs, const x = …; return x collapsing, indentation changes in mapDataEntry) is mixed into the functional diff, which makes the real changes hard to isolate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants