feat(condo): DOMA-13081 field type for store data externally#7412
Merged
AleX83Xpert merged 85 commits intomainfrom Apr 24, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new ExternalContent Keystone field type with adapter-backed external storage, processors (json/xml/text), request-scoped batched loader, secure path validation, admin UI views/controllers, GraphQL raw + resolved outputs, utilities/tests/README, and a submodule pointer bump. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant AdminUI as Admin UI
participant Keystone as Keystone Server
participant Loader as FileContentLoader
participant Adapter as File Adapter
participant Storage as Storage/HTTP
AdminUI->>Keystone: Query <field> and <field>Resolved
Keystone->>Loader: load(fileMeta) [context-scoped]
Loader->>Adapter: read / generateUrl (filename, metadata)
Adapter->>Storage: fetch/read file
Storage-->>Adapter: bytes / 404 / error
Adapter-->>Loader: Buffer | null | error
Loader-->>Keystone: Buffer | null
Keystone->>Keystone: processor.deserialize(Buffer -> value)
Keystone-->>AdminUI: resolved content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AleX83Xpert
added
the
🔬 WIP
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
598b875 to
aca2921
Compare
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
f1a0bf9 to
5b4a31b
Compare
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
5b4a31b to
cdeadf1
Compare
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
cdeadf1 to
d7bbce1
Compare
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
506c858 to
c82ddcf
Compare
AleX83Xpert
force-pushed
the
feat/condo/doma-13081/field-type-for-store-data-externally-for-billingreceipt-raw
branch
from
5894686 to
fc54960
Compare
… variable in ExternalContentField component
…typeof for undefined comparisons in ExternalContent
…t to top-level in ExternalContent Implementation
… FileContentLoader.spec.js
… Implementation.spec.js
…iguration into this.config
…k in ExternalContent field
…nConfig in ExternalContentController getQueryFragment
…utomatic file cleanup
… before processing ExternalContent values
…dation error message
…e adminConfig instead of isAdminUIReadOnly
…entLoader by wrapping filename in data object
…er import in ExternalContent Implementation
…processing in FileContentLoader
…king for cloud adapter capabilities instead of local adapter properties
…t null checks in ExternalContentController serialize and deserialize methods
…ENT_FIELD_TYPE_META and change file-meta detection logic
… URL generation and update file metadata with item id in afterChange
…om ExternalContentImplementation
…o clarify cloud storage URL handling and access control, replace <oops>Cloud reference with generic cloud storage term
|
YEgorLu
reviewed
Apr 24, 2026
| // Update file metadata with actual item id for new files | ||
| // This allows OBSFilesMiddleware to verify access control | ||
| const prevFilename = isFileMeta(prevValue) ? prevValue.filename : null | ||
| const nextFilename = isFileMeta(nextValue) ? nextValue.filename : null |
Contributor
There was a problem hiding this comment.
1) Above filename is being made with random ID every time. Does that mean that nextFilename === prefFilename situation impossible?
2) If filename changes, we do not delete old file. But if filename did not change, we delete old file.... I don't really get it. We delete same file after it's update or something?
It seems I was mistaken. For some reason I read all "if"s backwards
YEgorLu
approved these changes
Apr 24, 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
5 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.
Code Review Guide - ExternalContent Field
🎯 Start Here
This PR adds a new
ExternalContentfield type that stores large data in external files instead of the database. It also includes a generic backfill script to migrate any existing inline JSON field to file storage.📍 Review Priority Order
1. Security - Path Traversal Protection⚠️ CRITICAL
File:
packages/keystone/fieldsUtils/ExternalContent/validateFilePath.jsWhat to check:
validateFilePath()(for bothbasePathandfilename)path.isAbsolute(filename)before any joinpath.join()+path.normalize()on both the full path and the base pathnormalized === normalizedBase(resolving to base dir itself)normalized.startsWith(normalizedBase + path.sep)- returnsnormalized, notfullPathWhy it matters: Prevents attackers from reading arbitrary files from the server.
2. Race Condition Fix - DataLoader⚠️ CRITICAL
File:
packages/keystone/fieldsUtils/ExternalContent/FileContentLoader.jsWhat to check:
cache(Map),pending(Map), andqueue(Array)load()checkscachefirst, thenpendingmap before creating a new promisependingimmediately after creation, keyed bycacheKeypromise.finally()moves result frompendingtocacheand cleans uppending_schedulingBatchflag to prevent a second timer from being set between the!this.batchTimercheck and the timer assignmentbatchDelayMs: 0usessetImmediateinstead ofsetTimeoutfor immediate executionWhy it matters: Without
pendingmap, concurrent GraphQL resolver calls for the same file create duplicate queue entries and duplicate reads.3. Backfill Script - Migration Safety⚠️ CRITICAL
File:
apps/rb/bin/backfillExternalContentField.jsWhat to check:
isFileMeta(fieldValue)check skips records that are already migrated (idempotent)null/undefinedraw values are skipped (falsy check is intentional -false/0/''are valid JSON payloads)serverUtils[model].update()which triggersresolveInput— the field's own save logic handles file creation and DB update atomically within Keystone's mutation pipeline--dry-runflag logs what would happen without writing files or updating DB--continue-on-errorflag allows partial runs; failed record IDs are collected and printed at the endskip) — safe because processed records change their shape (become file-meta), so they won't appear in a re-runWhy it matters: Prevents re-migrating already-migrated records and provides a safe recovery path on partial failures.
4. Dual GraphQL Fields -
fieldvsfieldResolvedFile:
packages/keystone/fields/ExternalContent/Implementation.jsWhat to check:
gqlOutputFields()returns two fields:${path}: String(raw file-meta JSON for Admin UI) and${path}Resolved: <format type>(deserialized content for API clients)gqlOutputFieldResolvers()only registers a resolver for${path}Resolved— the raw field is served directly from the DB columnresolveInput()receivescontextand passescontext?.authedItemasusertoadapter.publicUrl()— needed for adapters that sign URLs per-user_type: FILE_META_TYPEandmeta: { format }soresolveExternalContentValuecan deserialize without guessing the format5. Admin UI - Field Component
File:
packages/keystone/fields/ExternalContent/views/Field.jsWhat to check:
isReadOnly = field.isReadOnly !== false— reads directly from the controller instance, not fromfield.controllerinitialEditValuestringify logic anduseEffectboth usetypeof resolvedContent === 'string' ? resolvedContent : JSON.stringify(resolvedContent, null, 2)— consistent handling prevents[object Object]in textareauseEffectdependency is[resolvedContent]only — triggers when async resolved content arrives after initial render6. Input Validation
File:
packages/keystone/fieldsUtils/ExternalContent/createExternalDataField.jsWhat to check:
adapteris required — throws immediately with clear message if missingmaxSizeBytesvalidated as a positive finite numberbatchDelayMsvalidated as a non-negative finite numberisAdminUIReadOnlymapped toadminConfig.isReadOnlyand not leaked into the returned field objecttype: 'ExternalContent'is placed after...otherPropsto prevent the caller from overriding itWhy it matters: Fails fast with clear errors if misconfigured.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores