Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
85 commits
Select commit Hold shift + click to select a range
399df29
feat(keystone): DOMA-13081 add ExternalContent field implementation a…
AleX83Xpert Mar 19, 2026
85765ab
feat(billing): DOMA-13081 replace RAW_DATA_FIELD with EXTERNAL_DATA_F…
AleX83Xpert Mar 19, 2026
1ea42ea
feat(billing): DOMA-13081 add backfill script for BillingReceipt.raw …
AleX83Xpert Mar 19, 2026
7b370c8
refactor(keystone): DOMA-13081 update ExternalContent field utilities…
AleX83Xpert Mar 19, 2026
dcc6f1a
refactor(billing): DOMA-13081 improve backfill script logic and updat…
AleX83Xpert Mar 19, 2026
455f0d6
refactor(keystone): DOMA-13081 improve ExternalContent file handling …
AleX83Xpert Mar 19, 2026
4de0116
refactor(keystone): DOMA-13081 update ExternalContent deserialization…
AleX83Xpert Mar 19, 2026
8d0ea8e
refactor(billing): DOMA-13081 enhance backfill script to track last p…
AleX83Xpert Mar 19, 2026
de5d904
refactor(keystone): DOMA-13081 enhance ExternalContentImplementation …
AleX83Xpert Mar 19, 2026
29953a6
refactor(billing): DOMA-13081 update FileAdapter instantiation in bac…
AleX83Xpert Mar 19, 2026
4ffc842
test(billing,keystone): DOMA-13081 add comprehensive tests for Extern…
AleX83Xpert Mar 19, 2026
6a74af7
refactor(billing): DOMA-13081 update FileAdapter folder name in backf…
AleX83Xpert Mar 30, 2026
81cdcaf
refactor(billing): DOMA-13081 rename EXTERNAL_DATA_FIELD to BILLING_R…
AleX83Xpert Mar 30, 2026
6a1e8ba
refactor(billing): DOMA-13081 improve backfill script UX with interac…
AleX83Xpert Mar 30, 2026
659587e
fix(condo): DOMA-13081 improve error handling, logging, and file nami…
AleX83Xpert Mar 30, 2026
5e0af20
test(keystone): DOMA-13081 add DataLoader implementation for External…
AleX83Xpert Mar 30, 2026
13a3ee3
fix(condo): DOMA-13081 security and reliability improvements
AleX83Xpert Mar 30, 2026
381ac2a
refactor(ExternalContent): DOMA-13081 add field-level size limits and…
AleX83Xpert Mar 30, 2026
0912814
fix(condo): DOMA-13081 improve security, race conditions, and validation
AleX83Xpert Mar 30, 2026
b813904
fix(condo): DOMA-13081 resolve critical security and performance issues
AleX83Xpert Mar 30, 2026
78b520d
fix(condo): DOMA-13081 improve error handling and validation in Exter…
AleX83Xpert Mar 30, 2026
693aa37
refactor(condo): DOMA-13081 remove raw field filtering from GraphQL s…
AleX83Xpert Mar 30, 2026
f669ceb
refactor(keystone): DOMA-13081 add configurable batch delay to Extern…
AleX83Xpert Mar 31, 2026
292e089
refactor(condo): DOMA-13081 remove BillingReceipt raw field backfill …
AleX83Xpert Mar 31, 2026
0e0978d
feat(condo): DOMA-13081 add backfill script for ExternalContent field…
AleX83Xpert Mar 31, 2026
5607d2e
fix(keystone): DOMA-13081 handle missing files gracefully in FileCont…
AleX83Xpert Apr 1, 2026
1c5a275
refactor(condo): DOMA-13081 remove --start-from-id option from backfi…
AleX83Xpert Apr 1, 2026
c1e8fd7
refactor(keystone): DOMA-13081 improve ExternalContent field reliabil…
AleX83Xpert Apr 1, 2026
060e230
test(keystone): DOMA-13081 add comprehensive tests for ExternalConten…
AleX83Xpert Apr 1, 2026
adf0315
refactor(condo): DOMA-13081 improve backfill script null handling and…
AleX83Xpert Apr 1, 2026
cd84cd4
chore(condo): DOMA-13081 exclude heavy keystone apps from backfill sc…
AleX83Xpert Apr 1, 2026
d198362
refactor(keystone): DOMA-13081 improve ExternalContent field initiali…
AleX83Xpert Apr 2, 2026
e67604e
refactor(keystone): DOMA-13081 modularize ExternalContent field utili…
AleX83Xpert Apr 2, 2026
b55fd6c
docs(keystone): DOMA-13081 update ExternalContent README with createE…
AleX83Xpert Apr 3, 2026
9a9873f
refactor(billing): DOMA-13081 revert BillingReceipt.raw from External…
AleX83Xpert Apr 3, 2026
92cd97d
refactor(keystone): DOMA-13081 add admin UI support for ExternalConte…
AleX83Xpert Apr 3, 2026
8a31cb4
refactor(keystone): DOMA-13081 simplify ExternalContent field by remo…
AleX83Xpert Apr 6, 2026
0215003
refactor(keystone): DOMA-13081 extract DEFAULT_PROCESSORS to separate…
AleX83Xpert Apr 6, 2026
1accb98
refactor(keystone): DOMA-13081 simplify createExternalDataField
AleX83Xpert Apr 6, 2026
7beef92
refactor(keystone): DOMA-13081 simplify ExternalContent field configu…
AleX83Xpert Apr 6, 2026
13a929e
refactor(keystone): DOMA-13081 add ExternalContent field support to h…
AleX83Xpert Apr 6, 2026
344b13f
refactor(keystone): DOMA-13081 improve createExternalDataField JSDoc …
AleX83Xpert Apr 6, 2026
22b1bb8
refactor(keystone): DOMA-13081 replace JSON adapters with Text adapte…
AleX83Xpert Apr 6, 2026
ccebead
refactor(keystone): DOMA-13081 simplify ExternalContentImplementation…
AleX83Xpert Apr 6, 2026
486ec5e
refactor(keystone): DOMA-13081 optimize lodash imports in historical …
AleX83Xpert Apr 6, 2026
2c6291c
refactor(keystone): DOMA-13081 update ExternalContent README with for…
AleX83Xpert Apr 6, 2026
cb72eaa
refactor(keystone): DOMA-13081 split ExternalContent field into raw m…
AleX83Xpert Apr 6, 2026
4f7f510
refactor(keystone): DOMA-13081 add file accumulation warning to Exter…
AleX83Xpert Apr 6, 2026
b2bfcf3
test(keystone): update ExternalContent tests for publicUrl, meta fiel…
AleX83Xpert Apr 7, 2026
d1bd49f
test(keystone): add tests for ExternalContent default processors (jso…
AleX83Xpert Apr 7, 2026
6c04530
chore(condo): update rb submodule
AleX83Xpert Apr 7, 2026
4605bf4
refactor(keystone): pass context to publicUrl adapter method and impr…
AleX83Xpert Apr 7, 2026
e5136bd
refactor(keystone): DOMA-13081 extract object classification logic in…
AleX83Xpert Apr 7, 2026
7164ba7
feat(condo): DOMA-13081 remove backfillExternalContentField.js from c…
AleX83Xpert Apr 7, 2026
19cab0a
feat(rb): DOMA-13081 add commander for backFill script
AleX83Xpert Apr 7, 2026
12ed029
refactor(keystone): DOMA-13081 use node: protocol for built-in module…
AleX83Xpert Apr 8, 2026
3171110
refactor(keystone): DOMA-13081 add default value for meta parameter a…
AleX83Xpert Apr 8, 2026
3ee0d53
refactor(keystone): DOMA-13081 use optional chaining in ExternalConte…
AleX83Xpert Apr 8, 2026
015b0c7
refactor(keystone): DOMA-13081 extract conditional rendering logic to…
AleX83Xpert Apr 8, 2026
b42534f
refactor(keystone): DOMA-13081 use strict equality checks instead of …
AleX83Xpert Apr 8, 2026
9c076b8
refactor(keystone): DOMA-13081 move resolveExternalContentValue impor…
AleX83Xpert Apr 8, 2026
a2c10b8
test(keystone): DOMA-13081 use node: protocol for fs/promises mock in…
AleX83Xpert Apr 8, 2026
4dee816
test(keystone): DOMA-13081 use node: protocol for fs/promises mock in…
AleX83Xpert Apr 13, 2026
371b672
refactor(keystone): DOMA-13081 improve logging structure
AleX83Xpert Apr 13, 2026
f59e73e
refactor(keystone): DOMA-13081 consolidate ExternalContent field conf…
AleX83Xpert Apr 13, 2026
8cf01a6
refactor(keystone): DOMA-13081 defer file deletion to afterChange hoo…
AleX83Xpert Apr 13, 2026
aa35249
refactor(keystone): DOMA-13081 use config.adminConfig instead of admi…
AleX83Xpert Apr 13, 2026
eca8225
docs(keystone): DOMA-13081 update README to reflect adminConfig and a…
AleX83Xpert Apr 13, 2026
8153067
refactor(keystone): DOMA-13081 parse JSON strings in afterChange hook…
AleX83Xpert Apr 13, 2026
49544e3
fix(keystone): DOMA-13081 correct parameter name in batchDelayMs vali…
AleX83Xpert Apr 13, 2026
02ea04a
test(keystone): DOMA-13081 update createExternalDataField tests to us…
AleX83Xpert Apr 13, 2026
c6f4b2e
refactor(keystone): DOMA-13081 improve logic and one log
AleX83Xpert Apr 15, 2026
08dd98a
docs(keystone): DOMA-13081 remove unused ExternalContent import from …
AleX83Xpert Apr 21, 2026
5a21ad9
refactor(keystone): DOMA-13081 standardize logging format in FileCont…
AleX83Xpert Apr 21, 2026
d554b89
refactor(keystone): DOMA-13081 use getLogger() instead of direct logg…
AleX83Xpert Apr 21, 2026
0df6c8c
refactor(keystone): DOMA-13081 remove empty finally block from batch …
AleX83Xpert Apr 21, 2026
966b6e3
refactor(keystone): DOMA-13081 improve adapter type detection by chec…
AleX83Xpert Apr 21, 2026
09fedc8
refactor(keystone): DOMA-13081 replace optional chaining with explici…
AleX83Xpert Apr 21, 2026
950a9c1
refactor(keystone): DOMA-13081 rb to main
AleX83Xpert Apr 22, 2026
bd6294c
refactor(keystone): DOMA-13081 update yarn.lock
AleX83Xpert Apr 22, 2026
bf1bbed
refactor(keystone): DOMA-13081 rename FILE_META_TYPE to EXTERNAL_CONT…
AleX83Xpert Apr 22, 2026
b2d3084
refactor(keystone): DOMA-13081 use adapter.publicUrl() for consistent…
AleX83Xpert Apr 23, 2026
15b37a0
refactor(keystone): DOMA-13081 remove unused getGqlAuxTypes method fr…
AleX83Xpert Apr 23, 2026
9708cbe
refactor(keystone): DOMA-13081 update ExternalContent documentation t…
AleX83Xpert Apr 23, 2026
93e0315
chore(condo): DOMA-13081 update rb submodule to main
AleX83Xpert Apr 24, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/rb
Submodule rb updated from bc2201 to ae05cc
2 changes: 2 additions & 0 deletions packages/keystone/KSv5v6/v5/registerSchema.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const {
AddressPartWithType,
AutoIncrementInteger,
DateInterval,
ExternalContent,
Json,
LocalizedText,
Select,
Expand Down Expand Up @@ -79,6 +80,7 @@ function convertStringToTypes (schema) {
DateInterval,
File: CustomFile,
Json,
ExternalContent,
HiddenRelationship,
LocalizedText,
Select,
Expand Down
299 changes: 299 additions & 0 deletions packages/keystone/fields/ExternalContent/Implementation.js
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be great to have a tool for migration from "old format" to new one

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it exists. See apps/rb/bin/backfillExternalContentField.js.

Original file line number Diff line number Diff line change
@@ -0,0 +1,299 @@
const { randomUUID } = require('node:crypto')
const { Readable } = require('node:stream')

const { Implementation } = require('@open-keystone/fields')

const { ExternalContent } = require('@open-condo/keystone/fieldsUtils')
const { getLogger } = require('@open-condo/keystone/logging')

const logger = getLogger()

const { EXTERNAL_CONTENT_FIELD_TYPE_META, DEFAULT_PROCESSORS, isFileMeta, resolveExternalContentValue } = ExternalContent

const DEFAULT_MAX_SIZE_BYTES = 10 * 1024 * 1024 // Default max size: 10MB
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not 100mb as "max size" that could be uploaded to platform?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content of the file is loaded into memory. So, I set the default maxSize less than on the platform.
Anyway, this is a tunable option.


/**
* ExternalContent field implementation for Keystone.
*
* Stores large data externally in files rather than directly in the database.
* Provides transparent access to file content through GraphQL with DataLoader batching.
*
* @typedef {import('@open-condo/keystone/fieldsUtils/ExternalContent/defaultProcessors').ExternalContentProcessor} ExternalContentProcessor
*
* @extends {import('@keystonejs/fields').Implementation}
*/
class ExternalContentImplementation extends Implementation {
/**
* Creates an ExternalContent field implementation.
*
* @param {string} path - Field path (e.g., 'raw', 'metadata')
* @param {Object} options - Configuration options
* @param {Object} options.adapter - File adapter instance (required)
* @param {string} [options.format='json'] - Data format ('json', 'xml', or 'text')
* @param {Object.<string, ExternalContentProcessor>} [options.processors={}] - Custom format processors
* @param {number} [options.maxSizeBytes=10485760] - Maximum payload size in bytes (default: 10MB)
* @param {number} [options.batchDelayMs] - DataLoader batch delay in milliseconds
* @param {string} [options.schemaDoc] - Field description for schema
* @param {string} [options.graphQLAdminFragment=''] - GraphQL fragment for admin UI
* @param {Object} [meta] - Keystone field metadata
*
* @throws {Error} If adapter is not provided
* @throws {Error} If format is unknown
* @throws {Error} If adapter is not properly configured
*/
constructor (path, options = {}, meta = {}) {
const {
adapter,
format = 'json',
processors = {},
} = options

// Compute processor config
const byFormat = { ...DEFAULT_PROCESSORS, ...processors }
const cfg = byFormat[format]
if (!cfg) {
throw new Error(`ExternalContent: unknown format "${format}" for ${path}`)
}

if (!adapter) {
throw new Error(`ExternalContent: "adapter" is required for ${path}`)
}

// Validate adapter is properly configured (not NoFileAdapter)
if (adapter.constructor.name === 'NoFileAdapter' || adapter.error?.message?.includes('NoFileAdapter')) {
throw new Error(`ExternalContent: adapter is not properly configured for ${path}. Check FILE_FIELD_ADAPTER and storage configuration.`)
}

super(path, options, meta)

this.config.format ??= 'json'
this.config.maxSizeBytes ??= DEFAULT_MAX_SIZE_BYTES

this.adapter = adapter
Comment thread
YEgorLu marked this conversation as resolved.
this.graphQLInputType = cfg.graphQLInputType
this.graphQLReturnType = cfg.graphQLReturnType
this.mimetype = cfg.mimetype
this.fileExt = cfg.fileExt
this.formatProcessors = byFormat
}

// GQL Output
gqlOutputFields () {
// Return both raw metadata field and virtual deserialized content field
return [
`${this.path}: String`, // Raw file metadata (JSON string) for admin UI
`${this.path}Resolved: ${this.graphQLReturnType}`, // Deserialized content for API clients
]
}

// Admin
extendAdminMeta (meta) {
return {
...meta,
graphQLAdminFragment: this.config.graphQLAdminFragment || '',
format: this.config.format,
processors: this.formatProcessors,
adminConfig: this.adminConfig || {},
}
Comment on lines +90 to +97
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Compare admin-meta builders and how they propagate adminConfig.
rg -n -C2 "extendAdminMeta|this\.config\.adminConfig|this\.adminConfig|adminConfig:" packages/keystone/fields

Repository: open-condo-software/condo

Length of output: 3285


🏁 Script executed:

cat -n packages/keystone/fields/ExternalContent/Implementation.js | head -150

Repository: open-condo-software/condo

Length of output: 6852


🏁 Script executed:

rg -n "constructor|this\.adminConfig\s*=" packages/keystone/fields/ExternalContent/Implementation.js

Repository: open-condo-software/condo

Length of output: 237


🏁 Script executed:

rg -n "adminConfig:" packages/keystone/fields -A1 -B1 | head -60

Repository: open-condo-software/condo

Length of output: 972


🏁 Script executed:

rg -n "adminConfig" packages/keystone/fields/ExternalContent/views -A2 -B2

Repository: open-condo-software/condo

Length of output: 1183


🏁 Script executed:

rg -n "field\.config\.adminConfig|adminConfig\.isReadOnly" packages/keystone/fields -l

Repository: open-condo-software/condo

Length of output: 51


🏁 Script executed:

rg -n "this\.adminConfig\s*=" packages/keystone/fields/ExternalContent

Repository: open-condo-software/condo

Length of output: 51


🏁 Script executed:

rg -n "Object\.defineProperty.*adminConfig" packages/keystone/fields/ExternalContent

Repository: open-condo-software/condo

Length of output: 51


Pass adminConfig from this.config into admin meta.

extendAdminMeta() references this.adminConfig, which is never set. The constructor passes options to the parent Implementation class, which stores them in this.config. Both views/Controller.js and views/Field.js expect adminConfig to be read from field.config?.adminConfig, but extendAdminMeta() returns an undefined value, causing the admin UI to default to read-only mode instead of respecting the configured isReadOnly setting.

Suggested change
         return {
             ...meta,
             graphQLAdminFragment: this.config.graphQLAdminFragment || '',
             format: this.config.format,
             processors: this.formatProcessors,
-            adminConfig: this.adminConfig || {},
+            adminConfig: this.config.adminConfig || {},
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
extendAdminMeta (meta) {
return {
...meta,
graphQLAdminFragment: this.config.graphQLAdminFragment || '',
format: this.config.format,
processors: this.formatProcessors,
adminConfig: this.adminConfig || {},
}
extendAdminMeta (meta) {
return {
...meta,
graphQLAdminFragment: this.config.graphQLAdminFragment || '',
format: this.config.format,
processors: this.formatProcessors,
adminConfig: this.config.adminConfig || {},
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/keystone/fields/ExternalContent/Implementation.js` around lines 106
- 113, extendAdminMeta currently returns adminConfig from this.adminConfig which
is never set; change it to read adminConfig from this.config (e.g., use
this.config.adminConfig || {}) so the admin meta includes the configured admin
options. Update the extendAdminMeta method (in Implementation.js) to return
adminConfig: this.config.adminConfig || {} so the UI components
(views/Controller.js and views/Field.js) receive the intended isReadOnly and
other admin settings.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! See node_modules/@open-keystone/fields/dist/fields.cjs.dev.js:64

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

Let me check the parent Implementation class to verify this.


🏁 Script executed:

#!/bin/bash
# Check lines around 64 in the open-keystone fields implementation
sed -n '50,100p' node_modules/@open-keystone/fields/dist/fields.cjs.dev.js

Length of output: 100


🏁 Script executed:

#!/bin/bash
# Find the open-keystone fields implementation
fd "fields.cjs.dev.js" --full-path 2>/dev/null | head -5

# Also try finding the Implementation class source
fd "fields.cjs*" --full-path 2>/dev/null | grep open-keystone | head -10

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Search for adminConfig in existing field implementations to see how it's used
rg -n "this\.adminConfig" packages/keystone/fields/ -A2 -B2

# Also search for adminConfig assignment in any cached/vendored keystone source
rg -rn "this\.adminConfig\s*=" --type js | grep -v spec | grep -v test | head -20

# Check if there's a local copy of the open-keystone source
fd "Implementation" packages/ --type f | head -10

Length of output: 1048


🏁 Script executed:

#!/bin/bash
# Search for open-keystone package source anywhere
fd "." --type d -g "*open-keystone*" 2>/dev/null | head -10

# Look in yarn berry cache or pnp
find . -name "fields.cjs*" -not -path "*/\.*" 2>/dev/null | head -10

# Also check the package.json for open-keystone to understand what version/location
rg -n "open-keystone" packages/keystone/package.json

# Look for .yarn or .pnp cache
ls -la .yarn/ 2>/dev/null | head -20

Length of output: 995


🏁 Script executed:

#!/bin/bash
# Check if there's a yarn cache in a different location
find /home -name "fields.cjs*" 2>/dev/null | head -5
find /tmp -name "fields.cjs*" 2>/dev/null | head -5
find /root -name "fields.cjs*" 2>/dev/null | head -5

# Check the .yarn/releases or plugins for hints
ls .yarn/releases/ 2>/dev/null
ls .yarn/plugins/ 2>/dev/null

# Look for a yarn.lock or package-lock to find the resolved package location
grep -A3 '"@open-keystone/fields"' yarn.lock 2>/dev/null | head -20

Length of output: 757


🏁 Script executed:

#!/bin/bash
# Look for open-keystone fields in yarn PnP cache
find . -path "*.yarn/cache*open-keystone*fields*" -name "*.zip" 2>/dev/null | head -5
find . -path "*yarn*cache*" -name "*open-keystone-fields*" 2>/dev/null | head -5

# Try to read it from the PnP virtual filesystem
python3 -c "
import zipfile, os, glob
# Search for any zip containing 'fields.cjs.dev.js'
for root, dirs, files in os.walk('.'):
    for f in files:
        if 'open-keystone' in f and 'field' in f and f.endswith('.zip'):
            path = os.path.join(root, f)
            print('Found:', path)
            try:
                with zipfile.ZipFile(path) as z:
                    names = [n for n in z.namelist() if 'fields.cjs' in n]
                    print('  Contents:', names[:5])
            except Exception as e:
                print('  Error:', e)
" 2>/dev/null | head -30

Length of output: 0


@AleX83Xpert, you're absolutely right — I apologize for the incorrect suggestion. The parent Implementation class in @open-keystone/fields assigns this.adminConfig directly from the field options in its constructor (at line 64 of fields.cjs.dev.js), so this.adminConfig is correctly populated and the existing code is valid. My suggestion to change it to this.config.adminConfig was wrong.


✏️ Learnings added
Learnt from: AleX83Xpert
Repo: open-condo-software/condo PR: 7412
File: packages/keystone/fields/ExternalContent/Implementation.js:106-113
Timestamp: 2026-04-21T05:30:48.218Z
Learning: In `packages/keystone/fields/ExternalContent/Implementation.js`, `extendAdminMeta()` uses `this.adminConfig` (not `this.config.adminConfig`). This is correct because the parent `Implementation` class from `open-keystone/fields` (at node_modules/open-keystone/fields/dist/fields.cjs.dev.js line 64) assigns `this.adminConfig` directly from the field options in its constructor. Do not flag `this.adminConfig` as undefined in ExternalContent or any other field implementation extending `open-keystone/fields`'s `Implementation` class.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: AleX83Xpert
Repo: open-condo-software/condo PR: 7412
File: packages/keystone/fields/ExternalContent/views/Controller.js:27-31
Timestamp: 2026-04-13T11:29:04.433Z
Learning: In `packages/keystone/fields/ExternalContent/views/Controller.js`, the `deserialize()` and `serialize()` methods use `!data?.[path]` to detect a missing/empty value. This is intentional and correct: the ExternalContent field stores only a JSON string of file metadata or a legacy content string — falsy primitives like `''`, `0`, or `false` are never valid stored values for this field, so the falsy guard does not lose any real data.

Learnt from: YEgorLu
Repo: open-condo-software/condo PR: 7412
File: packages/keystone/fields/ExternalContent/Implementation.js:156-158
Timestamp: 2026-04-08T06:31:31.532Z
Learning: When emitting logs from Keystone field resolvers (any file under packages/keystone/fields/*.js), use the standard log schema: set `entity` to the `listKey`, set `entityId` to the item’s id, and put field-specific context under `data` (e.g., `data: { field: this.path }`). Do not introduce custom root-level keys like `field` or `itemId`; keep those details inside `data` to ensure logs are consistent and queryable.

Learnt from: CR
Repo: open-condo-software/condo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-27T14:03:17.418Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Don't import from open-keystone/fields* or open-condo/keystone/fields

Learnt from: AleX83Xpert
Repo: open-condo-software/condo PR: 7412
File: packages/keystone/fields/ExternalContent/views/Controller.js:1-3
Timestamp: 2026-04-13T11:31:04.318Z
Learning: In `packages/keystone/fields`, field controller files (e.g. `views/Controller.js`) must import `FieldController` from `'open-keystone/fields/Controller'` — this is the established pattern used by all field controllers (Json, Options, ExternalContent, etc.) and is explicitly allowed within `packages/keystone/fields`. The guideline "Don't import from `open-keystone/fields`*" applies only to app-level and consumer code, not to field implementations inside `packages/keystone/fields` itself.

Learnt from: SavelevMatthew
Repo: open-condo-software/condo PR: 6945
File: apps/condo/domains/user/oidc/adapter/OidcModelClientAdapter.js:213-215
Timestamp: 2025-11-24T05:39:47.470Z
Learning: In the Condo codebase using Keystone, visual fields (like `domains` on B2BApp and B2CApp models) always resolve to the correct shape and are never null/undefined, so null safety checks are not needed when accessing properties on visual fields.

Learnt from: AleX83Xpert
Repo: open-condo-software/condo PR: 7139
File: packages/keystone/runtimeStats.js:169-188
Timestamp: 2026-01-28T10:49:33.372Z
Learning: In log serialization (e.g., in the Condo codebase's logging serializers), ensure that top-level fields such as reqId, url, method, path, and ip are kept at the root of the log object alongside existing fields like msg, data, entityId, entity, count, status, and err. Do not nest these fields inside the data object or move them into other nested structures.

}

/**
* Provides both raw file metadata and deserialized content via GraphQL.
*
* - `fieldName` - Returns raw file metadata (JSON string from DB) for admin UI
* - `fieldNameResolved` - Virtual field that returns deserialized content for API clients
*
* @returns {Object} Field resolver mapping
*/
gqlOutputFieldResolvers () {
return {
// Virtual field for deserialized content (for API clients)
[`${this.path}Resolved`]: async (item, args, context) => {
const value = item?.[this.path]

try {
return await resolveExternalContentValue(value, {
adapter: this.adapter,
formatProcessors: this.formatProcessors,
context,
batchDelayMs: this.config.batchDelayMs,
})
} catch (err) {
const itemId = item?.id || 'unknown'
logger.warn({
msg: 'Error resolving ExternalContent field',
entity: this.listKey,
entityId: itemId,
err,
data: { path: this.path },
})
throw err
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
},
}
}

// GQL Input
/**
* Returns query input fields for filtering.
* We store only file references in DB, so querying by content is not supported.
*
* @returns {Array} Empty array - no query filters available
*/
gqlQueryInputFields () {
return []
}

/**
* Returns GraphQL input fields for update mutations.
*
* @returns {Array<string>} Field definitions for GraphQL schema
*/
gqlUpdateInputFields () {
return [`${this.path}: ${this.graphQLInputType}`]
}

/**
* Returns GraphQL input fields for create mutations.
*
* @returns {Array<string>} Field definitions for GraphQL schema
*/
gqlCreateInputFields () {
return [`${this.path}: ${this.graphQLInputType}`]
}

// Hooks
/**
* Validates payload size against configured maximum.
*
* @private
* @param {number} sizeBytes - Size in bytes to validate
* @throws {Error} If size exceeds maxSizeBytes
*/
_validatePayloadSize (sizeBytes) {
if (sizeBytes > this.config.maxSizeBytes) {
const sizeMB = (sizeBytes / 1024 / 1024).toFixed(2)
const maxMB = (this.config.maxSizeBytes / 1024 / 1024).toFixed(2)
throw new Error(`ExternalContent: payload size (${sizeMB}MB) exceeds maximum allowed size (${maxMB}MB) for field ${this.path}`)
}
}

/**
* Resolves input data before saving to database.
*
* Handles file lifecycle:
* 1. If setting to null: returns null (old file deleted in afterChange)
* 2. If setting new value: saves new file to storage, returns file-meta (old file deleted in afterChange)
*
* Known limitation: If adapter.save() succeeds but the DB write fails, the new file will be
* orphaned in storage. This is acceptable — old file is never deleted until afterChange confirms
* the DB write succeeded.
*
* @param {Object} params
* @param {Object} params.resolvedData - New field values
* @param {Object} params.existingItem - Current item from database
* @param {string} params.listKey - Name of the list/model
* @param {Object} [params.context] - Keystone context (used for authed user when generating public URLs)
* @returns {Promise<Object|null|undefined>} File-meta object to store in DB
*/
async resolveInput ({ resolvedData, existingItem, listKey, context }) {
const nextValue = resolvedData[this.path]

if (nextValue === undefined || nextValue === null) return nextValue

const processor = this.formatProcessors[this.config.format]
const payload = processor.serialize(nextValue)
const payloadSizeBytes = Buffer.byteLength(String(payload), 'utf-8')

this._validatePayloadSize(payloadSizeBytes)

const stream = Readable.from([Buffer.from(String(payload), 'utf-8')])

const prefix = listKey || 'item'
const id = randomUUID()
const originalFilename = `${prefix}_${this.path}_${id}.${this.fileExt}`

// Save minimal metadata for OBSFilesMiddleware to identify and authorize access
// id is not available yet (generated by DB), will be set in afterChange
const fileMeta = {
listkey: listKey,
mimetype: this.mimetype,
}

const saved = await this.adapter.save({
stream,
filename: originalFilename,
mimetype: this.mimetype,
encoding: 'utf-8',
id,
meta: fileMeta,
})

// Generate publicUrl using adapter's method
// This ensures consistent URL generation with other file fields
// For cloud adapters, this returns middleware URL (not direct signed URL) so links don't expire
const publicUrl = this.adapter.publicUrl({
filename: originalFilename,
originalFilename,
id,
meta: fileMeta,
})

return { ...saved, publicUrl, [EXTERNAL_CONTENT_FIELD_TYPE_META]: { format: this.config.format } }
}

/**
* Deletes the old file after DB write succeeds.
*
* Called by Keystone after the item is saved to the database.
* Deleting here ensures the old file is only removed
* once the new value is durably committed — preventing data loss on DB failure.
*
* @param {Object} params
* @param {Object} params.existingItem - Item state before the update
* @param {Object} params.updatedItem - Item state after the update
*/
async afterChange ({ existingItem, updatedItem, listKey }) {
const parseDbValue = (v) => {
if (typeof v !== 'string') return v
try { return JSON.parse(v) } catch { return null }
}

const prevValue = parseDbValue(existingItem?.[this.path])
const nextValue = parseDbValue(updatedItem?.[this.path])

// 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
Copy link
Copy Markdown
Contributor

@YEgorLu YEgorLu Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (nextFilename && prevFilename !== nextFilename && this.adapter.acl?.setMeta) {
try {
const folder = this.adapter.folder || ''
await this.adapter.acl.setMeta(`${folder}/${nextFilename}`, {
listkey: listKey,
id: updatedItem.id,
mimetype: this.mimetype,
})
} catch (err) {
logger.warn({ msg: 'Failed to update file metadata with item id', err, data: { nextValue, itemId: updatedItem.id } })
}
}

if (!isFileMeta(prevValue)) return

// Old file should be deleted if field was cleared or replaced with a new file
if (prevFilename === nextFilename) return

try {
await this.adapter.delete(prevValue)
} catch (err) {
logger.warn({ msg: 'Failed to delete old file after change', err, data: { prevValue } })
}
Comment on lines +287 to +291
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don’t log the full file metadata on delete failures.

prevValue can contain publicUrl or adapter-specific storage details. Log stable identifiers instead, and keep field/item context in standard structured fields.

Suggested change
         try {
             await this.adapter.delete(prevValue)
         } catch (err) {
-            logger.warn({ msg: 'Failed to delete old file after change', err, data: { prevValue } })
+            logger.warn({
+                msg: 'Failed to delete old file after change',
+                entity: this.listKey,
+                entityId: updatedItem?.id || existingItem?.id || 'unknown',
+                err,
+                data: {
+                    field: this.path,
+                    filename: prevValue.filename,
+                },
+            })
         }

As per coding guidelines, "Use standard logging fields: msg, data, entityId, entity, count, status, and err instead of custom field names".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/keystone/fields/ExternalContent/Implementation.js` around lines 274
- 278, The logger.warn call in the delete catch logs the entire prevValue (which
may include publicUrl or adapter secrets); change it to log only stable
identifiers and standard fields: call logger.warn({ msg: 'Failed to delete old
file after change', entity: 'ExternalContent', entityId: prevValue?.id ||
prevValue?.filename || null, data: { fieldKey: this.path }, err }) and remove
prevValue from the log payload; keep the this.adapter.delete(prevValue) call and
ensure err is passed as the err field for structured logging.

}
}

module.exports = {
ExternalContentImplementation,
DEFAULT_PROCESSORS,
}

Loading
Loading