Skip to content

Commit cdeadf1

Browse files
committed
refactor(keystone): DOMA-13081 modularize ExternalContent field utilities and centralize adapter definitions
- Split ExternalContent utilities into separate files in /packages/keystone/fieldsUtils/ * constants.js, isFileMeta.js, createExternalDataField.js, validateFilePath.js * readFromAdapter.js, getOrCreateLoader.js, resolveExternalContentValue.js, FileContentLoader.js - Split test suites into individual files for each utility - Update all imports to use new modular structure - Export fieldsUtils from package.json for proper module access - Centralize BillingReceiptRawFieldFileAdapter in common.js - Extract adapter from BILLING_RECEIPT_RAW_FIELD field config in findOrganizationsByAddress.js - Update README.md with correct import paths - Add error context wrapping in readFromAdapter for better debugging
1 parent 9f40750 commit cdeadf1

23 files changed

Lines changed: 628 additions & 338 deletions

apps/condo/bin/backfillExternalContentField.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,9 @@ const path = require('path')
2323

2424
const commander = require('commander')
2525

26+
const { ExternalContent: { isFileMeta } } = require('@open-condo/keystone/fieldsUtils')
2627
const { prepareKeystoneExpressApp } = require('@open-condo/keystone/prepareKeystoneApp')
2728
const { itemsQuery } = require('@open-condo/keystone/schema')
28-
const { isFileMeta } = require('@open-condo/keystone/utils/externalContentFieldType')
2929

3030
const { prompt } = require('./lib/prompt')
3131

apps/condo/domains/billing/schema/fields/common.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
1+
const { ExternalContent: { createExternalDataField } } = require('@open-condo/keystone/fieldsUtils')
12
const FileAdapter = require('@open-condo/keystone/fileAdapter/fileAdapter')
2-
const { createExternalDataField } = require('@open-condo/keystone/utils/externalContentFieldType')
33

44
const { validatePeriod } = require('@condo/domains/billing/utils/validation.utils')
55

6+
67
const BILLING_FILE_ADAPTER = new FileAdapter('BillingIntegrations')
78
const BillingReceiptRawFieldFileAdapter = new FileAdapter('BillingReceiptRawField')
89

apps/condo/domains/resident/utils/serverSchema/findOrganizationsByAddress.js

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
const { get } = require('lodash')
1+
const get = require('lodash/get')
22

33
const { featureToggleManager } = require('@open-condo/featureflags/featureToggleManager')
4+
const { ExternalContent: { resolveExternalContentValue } } = require('@open-condo/keystone/fieldsUtils')
45
const { find } = require('@open-condo/keystone/schema')
56

67
const {
@@ -9,10 +10,12 @@ const {
910
const {
1011
ONLINE_INTERACTION_CHECK_ACCOUNT_SUCCESS_STATUS,
1112
} = require('@condo/domains/billing/constants/onlineInteraction')
13+
const { BILLING_RECEIPT_RAW_FIELD: { adapter: BillingReceiptRawFieldFileAdapter } } = require('@condo/domains/billing/schema/fields/common')
1214
const { getAccountsWithOnlineInteractionUrl } = require('@condo/domains/billing/utils/serverSchema/checkAccountNumberWithOnlineInteractionUrl')
1315
const { DISABLE_DISCOVER_SERVICE_CONSUMERS } = require('@condo/domains/common/constants/featureflags')
1416
const { CONTEXT_FINISHED_STATUS: BILLING_CONTEXT_FINISHED_STATUS } = require('@condo/domains/miniapp/constants')
1517

18+
1619
/*
1720
TODO: (DOMA-11059) Eliminate unnecessary subqueries like MeterResourceOwner and BillingContext.
1821
Currently, each organization is processed separately, leading to excessive subqueries.
@@ -168,15 +171,28 @@ async function getOrganizationReceipts (billingContexts, addressKey, query = {})
168171
const billingAccountsNumbersIndex = Object.fromEntries(
169172
billingAccounts.map(account => [account.id, account.number])
170173
)
171-
return receipts.map(receipt => ({
172-
category: receipt.category,
173-
balance: receipt.toPay,
174-
accountNumber: billingAccountsNumbersIndex[receipt.account],
175-
routingNumber: receipt.recipient.bic,
176-
bankAccount: receipt.recipient.bankAccount,
177-
address: get(receipt, 'raw.address', null),
178-
contextId: receipt.context,
174+
175+
// Resolve ExternalContent field values for receipts
176+
const resolvedReceipts = await Promise.all(receipts.map(async (receipt) => {
177+
const rawValue = await resolveExternalContentValue(receipt.raw, {
178+
adapter: BillingReceiptRawFieldFileAdapter,
179+
deserialize: (raw) => (raw.length === 0 ? null : JSON.parse(raw)),
180+
fieldPath: 'raw',
181+
item: receipt,
182+
})
183+
184+
return {
185+
category: receipt.category,
186+
balance: receipt.toPay,
187+
accountNumber: billingAccountsNumbersIndex[receipt.account],
188+
routingNumber: receipt.recipient.bic,
189+
bankAccount: receipt.recipient.bankAccount,
190+
address: get(rawValue, 'address', null),
191+
contextId: receipt.context,
192+
}
179193
}))
194+
195+
return resolvedReceipts
180196
}
181197

182198
async function getOrganizationMeters (organization, addressKey, properties, query = {}) {

packages/keystone/fields/ExternalContent/Implementation.js

Lines changed: 21 additions & 153 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,17 @@
1-
const { readFile } = require('fs/promises')
21
const { Readable } = require('stream')
32

43
const { Implementation } = require('@open-keystone/fields')
54
const cuid = require('cuid')
65

6+
const { ExternalContent } = require('@open-condo/keystone/fieldsUtils')
77
const { getLogger } = require('@open-condo/keystone/logging')
8-
const { FILE_META_TYPE, isFileMeta } = require('@open-condo/keystone/utils/externalContentFieldType')
98

10-
const { FileContentLoader } = require('./FileContentLoader')
11-
const { validateFilePath } = require('./utils')
129

10+
const { FILE_META_TYPE, isFileMeta, resolveExternalContentValue } = ExternalContent
1311
const logger = getLogger('ExternalContent')
1412

15-
// WeakMap to store unique loader keys per adapter instance
16-
// This prevents loader key collisions when multiple adapters have the same folder name
17-
const adapterLoaderKeys = new WeakMap()
18-
1913
const DEFAULT_FORMAT = 'json'
20-
// Default max size: 10MB
21-
const DEFAULT_MAX_SIZE_BYTES = 10 * 1024 * 1024
22-
14+
const DEFAULT_MAX_SIZE_BYTES = 10 * 1024 * 1024 // Default max size: 10MB
2315
const DEFAULT_PROCESSORS = {
2416
json: {
2517
graphQLInputType: 'JSON',
@@ -53,89 +45,6 @@ const DEFAULT_PROCESSORS = {
5345
},
5446
}
5547

56-
/**
57-
* Reads file contents for ExternalContent field from the provided adapter.
58-
*
59-
* Why not just `fetch(file.publicUrl)`?
60-
* - `publicUrl()` often returns an indirect URL like `/api/files/...` served by our app.
61-
* - Those endpoints frequently require authentication (cookie / Authorization / signature).
62-
* - Field resolvers do not have a browser session and should not depend on request-specific auth.
63-
*
64-
* Therefore:
65-
* - For cloud adapters we use `adapter.acl.generateUrl(...)` to get a short-lived signed *direct* URL
66-
* (S3/OBS), and fetch bytes from it without cookies.
67-
* - For local adapter we read from filesystem directly via `adapter.src`.
68-
*
69-
* @param {*} adapter File adapter instance passed into field config
70-
* @param {{ filename: string, mimetype?: string, originalFilename?: string }} fileMeta Stored file-meta object
71-
* @returns {Promise<Buffer>}
72-
*/
73-
async function readFromAdapter (adapter, fileMeta) {
74-
const filename = fileMeta.filename
75-
76-
// Local adapter (from `packages/keystone/fileAdapter/fileAdapter.js`)
77-
if (typeof adapter?.src === 'string') {
78-
const fullPath = validateFilePath(adapter.src, filename)
79-
return Buffer.from(await readFile(fullPath))
80-
}
81-
82-
// Cloud adapters provide acl.generateUrl which returns a signed, time-limited direct URL.
83-
// It does not require auth cookies (unlike indirect /api/files/... urls from publicUrl()).
84-
if (adapter?.acl && typeof adapter.acl.generateUrl === 'function' && adapter.folder) {
85-
const directUrl = adapter.acl.generateUrl({
86-
filename: `${adapter.folder}/${filename}`,
87-
mimetype: fileMeta.mimetype,
88-
originalFilename: fileMeta.originalFilename,
89-
})
90-
91-
if (!directUrl || typeof directUrl !== 'string') {
92-
throw new Error(`Invalid URL generated for file: ${filename}`)
93-
}
94-
95-
const res = await fetch(directUrl)
96-
if (!res.ok) {
97-
throw new Error(`Fetch failed with status ${res.status} for file: ${filename}`)
98-
}
99-
const buf = Buffer.from(await res.arrayBuffer())
100-
return buf
101-
}
102-
103-
throw new Error('ExternalContent: unsupported file adapter for read')
104-
}
105-
106-
/**
107-
* Get or create a FileContentLoader for the given adapter.
108-
*
109-
* Implements lazy initialization: creates loader on first access and reuses it for subsequent calls.
110-
* Each adapter instance gets its own unique loader (keyed by adapter instance via WeakMap).
111-
*
112-
* @param {Object} context - GraphQL context object
113-
* @param {Object} adapter - File adapter instance
114-
* @param {number} [batchDelayMs] - Time window in milliseconds to collect requests before executing batch
115-
* @returns {FileContentLoader} Loader instance for this adapter
116-
*/
117-
function getOrCreateLoader (context, adapter, batchDelayMs) {
118-
// Initialize loaders map if not exists
119-
if (!context._externalContentLoaders) {
120-
context._externalContentLoaders = new Map()
121-
}
122-
123-
// Use adapter instance as key via WeakMap to prevent collisions
124-
// This ensures different adapter instances get different loaders even if they have the same folder
125-
if (!adapterLoaderKeys.has(adapter)) {
126-
adapterLoaderKeys.set(adapter, Symbol('loader'))
127-
}
128-
const loaderKey = adapterLoaderKeys.get(adapter)
129-
130-
// Return existing loader or create new one
131-
if (!context._externalContentLoaders.has(loaderKey)) {
132-
const options = batchDelayMs !== undefined ? { batchDelayMs } : undefined
133-
context._externalContentLoaders.set(loaderKey, new FileContentLoader(adapter, options))
134-
}
135-
136-
return context._externalContentLoaders.get(loaderKey)
137-
}
138-
13948
class ExternalContentImplementation extends Implementation {
14049
constructor (path, {
14150
adapter,
@@ -222,71 +131,30 @@ class ExternalContentImplementation extends Implementation {
222131
/**
223132
* Resolves the field value for GraphQL output by reading the file content from storage.
224133
*
225-
* For backward compatibility, returns inline JSON objects directly if they don't have file-meta structure.
226-
* For file-meta objects, fetches the file content and deserializes it.
227-
*
228-
* Performance optimization:
229-
* - Uses DataLoader for batching and caching when context is available (GraphQL queries)
230-
* - Batches multiple file reads into single operation (10ms window)
231-
* - Caches results within request to prevent duplicate reads
232-
* - Falls back to direct readFromAdapter for non-GraphQL usage
233-
*
234-
* Note: This reads the entire file into memory. For very large files (10MB+), this could cause
235-
* memory pressure under load. Current use case (BillingReceipt.raw) typically has files <1MB.
134+
* Delegates to resolveExternalContentValue utility which handles:
135+
* - Backward compatibility with inline JSON objects
136+
* - File-meta object resolution with DataLoader batching
137+
* - Graceful handling of missing files
236138
*
237139
* @returns {Object} Field resolver mapping
238140
*/
239141
gqlOutputFieldResolvers () {
240142
return {
241143
[this.path]: async (item, args, context) => {
242-
let value = item?.[this.path]
243-
if (value === null || typeof value === 'undefined') return value
244-
245-
// Parse JSON string if needed (database stores serialized JSON)
246-
if (typeof value === 'string') {
247-
try {
248-
value = JSON.parse(value)
249-
} catch (err) {
250-
// If parsing fails, return the string as-is (backward compatibility)
251-
return value
252-
}
253-
}
254-
255-
// Backward compatibility: old `Json` field stored raw object directly in DB
256-
if (!isFileMeta(value)) return value
257-
258-
// Use DataLoader for batching and caching when context is available
259-
let buf
260-
try {
261-
if (context) {
262-
const loader = getOrCreateLoader(context, this.adapter, this.batchDelayMs)
263-
buf = await loader.load(value)
264-
} else {
265-
// Fallback to direct read for non-GraphQL usage (tests, scripts, etc.)
266-
buf = await readFromAdapter(this.adapter, value)
267-
}
268-
} catch (err) {
269-
// Handle missing files gracefully - return null instead of crashing
270-
if (err.code === 'ENOENT') {
271-
const itemId = item?.id || 'unknown'
272-
logger.warn({ msg: 'File not found for ExternalContent field', field: this.path, itemId, filename: value?.filename || 'unknown' })
273-
return null
274-
}
275-
throw err
276-
}
277-
278-
// Handle null buffer from FileContentLoader (missing file)
279-
if (buf === null) {
280-
return null
281-
}
282-
144+
const value = item?.[this.path]
283145
try {
284-
const raw = buf.toString('utf-8')
285-
return this.deserialize(raw)
146+
return await resolveExternalContentValue(value, {
147+
adapter: this.adapter,
148+
deserialize: this.deserialize,
149+
context,
150+
batchDelayMs: this.batchDelayMs,
151+
fieldPath: this.path,
152+
item,
153+
})
286154
} catch (err) {
287155
const itemId = item?.id || 'unknown'
288-
const errMsg = err?.message || String(err)
289-
throw new Error(`Failed to deserialize ${this.path} for item ${itemId}: ${errMsg}`)
156+
logger.warn({ msg: 'Error resolving ExternalContent field', field: this.path, itemId, err })
157+
throw err
290158
}
291159
},
292160
}
@@ -388,13 +256,13 @@ class ExternalContentImplementation extends Implementation {
388256

389257
// Save first, then delete old file.
390258
// This prevents losing the previous file if save() fails.
391-
259+
392260
const payload = this.serialize(nextValue)
393261
const payloadSizeBytes = Buffer.byteLength(String(payload), 'utf-8')
394-
262+
395263
// Validate size limit after serialization
396264
this._validatePayloadSize(payloadSizeBytes)
397-
265+
398266
const stream = Readable.from([Buffer.from(String(payload), 'utf-8')])
399267

400268
const prefix = listKey || 'item'

packages/keystone/fields/ExternalContent/ExternalContentFieldType.spec.js renamed to packages/keystone/fields/ExternalContent/Implementation.spec.js

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,12 @@ jest.mock('fs/promises', () => ({
44
readFile: mockReadFile,
55
}))
66

7+
// Mock fetch before importing Implementation
8+
const mockFetch = jest.fn()
9+
jest.mock('@open-condo/keystone/fetch', () => ({
10+
fetch: mockFetch,
11+
}))
12+
713
// Mock logger to avoid console output in tests
814
jest.mock('@open-condo/keystone/logging', () => ({
915
getLogger: () => ({
@@ -262,8 +268,12 @@ describe('ExternalContent field type', () => {
262268
describe('readFromAdapter error handling', () => {
263269
// Note: readFromAdapter is not exported, so we test it indirectly through gqlOutputFieldResolvers
264270

271+
beforeEach(() => {
272+
mockFetch.mockClear()
273+
})
274+
265275
afterEach(() => {
266-
delete global.fetch
276+
mockFetch.mockClear()
267277
})
268278

269279
test('handles cloud adapter with mimetype parameter', async () => {
@@ -275,11 +285,14 @@ describe('ExternalContent field type', () => {
275285
folder: 'test-folder',
276286
}
277287

278-
// Mock global fetch
279-
global.fetch = jest.fn(async () => ({
288+
// Setup mock fetch to return successful response
289+
mockFetch.mockResolvedValueOnce({
280290
ok: true,
281-
arrayBuffer: async () => Buffer.from(JSON.stringify({ test: 'data' })),
282-
}))
291+
arrayBuffer: async () => {
292+
const data = JSON.stringify({ test: 'data' })
293+
return new TextEncoder().encode(data).buffer
294+
},
295+
})
283296

284297
const impl = new ExternalContentImplementation('raw', { adapter, format: 'json' }, createMeta())
285298
const resolver = impl.gqlOutputFieldResolvers().raw
@@ -325,10 +338,10 @@ describe('ExternalContent field type', () => {
325338
folder: 'test-folder',
326339
}
327340

328-
global.fetch = jest.fn(async () => ({
341+
mockFetch.mockResolvedValueOnce({
329342
ok: false,
330343
status: 404,
331-
}))
344+
})
332345

333346
const impl = new ExternalContentImplementation('raw', { adapter, format: 'json' }, createMeta())
334347
const resolver = impl.gqlOutputFieldResolvers().raw
@@ -346,16 +359,15 @@ describe('ExternalContent field type', () => {
346359
folder: 'test-folder',
347360
}
348361

349-
global.fetch = jest.fn(async () => {
350-
throw new Error('Network error')
351-
})
362+
// Setup mock fetch to throw network error
363+
mockFetch.mockRejectedValueOnce(new Error('Network error'))
352364

353365
const impl = new ExternalContentImplementation('raw', { adapter, format: 'json' }, createMeta())
354366
const resolver = impl.gqlOutputFieldResolvers().raw
355367

356368
await expect(resolver({
357369
raw: { id: 'test-id', filename: 'test.json' },
358-
})).rejects.toThrow('Network error')
370+
})).rejects.toThrow('ExternalContent: failed to read file test.json: Network error')
359371
})
360372
})
361373

packages/keystone/fields/ExternalContent/README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ const MySchema = {
3636
### Using the Helper Function
3737

3838
```javascript
39-
const { createExternalDataField } = require('@open-condo/keystone/utils/externalContentFieldType')
39+
const { ExternalContent } = require('@open-condo/keystone/fieldsUtils')
40+
const { createExternalDataField } = ExternalContent
4041
const FileAdapter = require('@open-condo/keystone/fileAdapter/fileAdapter')
4142

4243
const MyFieldFileAdapter = new FileAdapter('MyFieldFolder')

0 commit comments

Comments
 (0)