diff --git a/src/storage/version/put.js b/src/storage/version/put.js index 2b432844..a4716a3a 100644 --- a/src/storage/version/put.js +++ b/src/storage/version/put.js @@ -78,6 +78,16 @@ function shouldCreateVersion(contentType) { return type.startsWith('text/html') || type.startsWith('application/json'); } +// Infer a versionable contentType from the file extension when the stored +// ContentType is missing or octet-stream (legacy imports). Returns the original +// contentType when it is already versionable, or when we cannot map the extension. +function inferVersionableType(contentType, ext) { + if (shouldCreateVersion(contentType)) return contentType; + if (ext === 'html') return 'text/html'; + if (ext === 'json') return 'application/json'; + return contentType; +} + function buildInput({ bucket, org, key, body, type, contentLength, }) { @@ -225,7 +235,7 @@ export async function putObjectWithVersion( Repo: daCtx.site || undefined, Body: (body || storeBody ? current.body : ''), ContentLength: (body || storeBody ? current.contentLength : undefined), - ContentType: current.contentType, + ContentType: contentType, ID, Version, Ext: daCtx.ext, @@ -322,8 +332,13 @@ export async function postObjectVersionWithLabel(label, env, daCtx) { const bodyBuffer = body instanceof ReadableStream ? await new Response(body).arrayBuffer() : body; const { bucket, org, key } = daCtx; + // Legacy imports may have lost their ContentType metadata (stored as + // application/octet-stream). Recover the correct mime from the file + // extension so the version write + main-object PUT both heal. + const inferredType = inferVersionableType(contentType, daCtx.ext); + const resp = await putObjectWithVersion(env, daCtx, { - bucket, org, key, body: bodyBuffer, contentLength, type: contentType, label, + bucket, org, key, body: bodyBuffer, contentLength, type: inferredType, label, }, true); if (resp.status !== 200) return { status: resp.status }; @@ -334,6 +349,8 @@ export async function postObjectVersionWithLabel(label, env, daCtx) { // eslint-disable-next-line no-console console.error('Failed to version (no version created)', { contentType, + inferredType, + ext: daCtx.ext, hadLabel: label != null, currentStatus, }); diff --git a/test/storage/version/put.test.js b/test/storage/version/put.test.js index df42459b..0e675333 100644 --- a/test/storage/version/put.test.js +++ b/test/storage/version/put.test.js @@ -3162,28 +3162,37 @@ describe('Version Put', () => { assert.strictEqual(resp.status, 201, 'must return 201 when version already exists (concurrent 412)'); }); - it('logs diagnostics when versionCreated is false (silent 500 path)', async () => { - // Regression: legacy HTML imports can land in storage with no S3 - // ContentType metadata. shouldCreateVersion(undefined) returns false -> - // shouldCreateVersionObject is false -> putObjectWithVersion returns - // { status: 200, versionCreated: false } -> postObjectVersionWithLabel - // returns a silent 500 with empty Cloudflare Logs (no console.error). - // The fix adds a single diagnostic log capturing contentType / hadLabel / - // currentStatus so the next daily review can confirm root cause. + it('heals legacy octet-stream HTML on labelled version: snapshot + main object both repaired', async () => { + // Regression: legacy imports with missing or octet-stream ContentType could not be + // labelled-versioned because shouldCreateVersion gated out non-html/json. Recover the + // correct mime from the file extension so the version snapshot AND the main-object PUT + // both heal to text/html. + const versionWrites = []; + const mainWrites = []; + const auditCalls = []; + const mockGetObject = async () => ({ - body: 'doc content', - contentType: undefined, - contentLength: 200, + body: 'legacy', + contentType: 'application/octet-stream', + contentLength: 18, status: 200, - metadata: { id: 'doc-id', version: 'v1' }, + metadata: { + id: 'legacy-id', version: 'v1', timestamp: '123', users: '[]', path: '/mysite/legacy.html', + }, etag: '"etag1"', }); - const mainClient = { - async send() { return { $metadata: { httpStatusCode: 200 } }; }, - }; const versionClient = { - async send() { return { $metadata: { httpStatusCode: 200 } }; }, + async send(cmd) { + versionWrites.push(cmd.input); + return { $metadata: { httpStatusCode: 200 } }; + }, + }; + const mainClient = { + async send(cmd) { + mainWrites.push(cmd.input); + return { $metadata: { httpStatusCode: 200 } }; + }, }; const { postObjectVersionWithLabel } = await esmock('../../../src/storage/version/put.js', { @@ -3193,13 +3202,59 @@ describe('Version Put', () => { ifMatch: () => mainClient, }, '../../../src/storage/version/audit.js': { - writeAuditEntry: async () => ({ status: 200 }), + writeAuditEntry: async (env, ctx, repo, fileId, entry) => { + auditCalls.push({ repo, fileId, entry }); + }, }, '../../../src/storage/utils/config.js': { default: () => ({}) }, }); const daCtx = { - bucket: 'b', org: 'o', key: 'doc.html', ext: 'html', users: [], + bucket: 'b', org: 'o', site: 'mysite', key: 'mysite/legacy.html', ext: 'html', users: [], + }; + const resp = await postObjectVersionWithLabel('Pre-import snapshot', {}, daCtx); + + assert.strictEqual(resp.status, 201); + assert.strictEqual(versionWrites.length, 1, 'version snapshot must be written'); + assert.strictEqual(versionWrites[0].ContentType, 'text/html', 'snapshot ContentType must heal to text/html'); + assert.strictEqual(versionWrites[0].Metadata.Label, 'Pre-import snapshot'); + assert.strictEqual(mainWrites.length, 1, 'main object PUT must run'); + assert.strictEqual(mainWrites[0].ContentType, 'text/html', 'main object ContentType must heal in storage'); + assert.strictEqual(auditCalls.length, 1, 'audit entry must be written'); + assert.strictEqual(auditCalls[0].entry.versionLabel, 'Pre-import snapshot'); + assert.ok(auditCalls[0].entry.versionId); + }); + + it('logs diagnostics and returns 500 when labelled version requested on non-versionable ext', async () => { + // Preserves the binary-never-version semantics: when the file extension does not map + // to a versionable mime (and the stored contentType is also not versionable), the + // labelled-version request still 500s and the diagnostic log fires with inferredType + ext + // captured so we can spot future legacy patterns in Cloudflare Logs. + const mockGetObject = async () => ({ + body: 'binary content', + contentType: 'application/octet-stream', + contentLength: 14, + status: 200, + metadata: { id: 'bin-id', version: 'v1' }, + etag: '"etag1"', + }); + + const s3Client = { + async send() { return { $metadata: { httpStatusCode: 200 } }; }, + }; + + const { postObjectVersionWithLabel } = await esmock('../../../src/storage/version/put.js', { + '../../../src/storage/object/get.js': { default: mockGetObject }, + '../../../src/storage/utils/version.js': { + ifNoneMatch: () => s3Client, + ifMatch: () => s3Client, + }, + '../../../src/storage/version/audit.js': { writeAuditEntry: async () => ({ status: 200 }) }, + '../../../src/storage/utils/config.js': { default: () => ({}) }, + }); + + const daCtx = { + bucket: 'b', org: 'o', site: 'mysite', key: 'mysite/data.bin', ext: 'bin', users: [], }; const errors = []; @@ -3216,12 +3271,64 @@ describe('Version Put', () => { assert.strictEqual(resp.status, 500); assert.strictEqual(resp.error, 'Version was not created'); - assert(errors.length > 0, 'silent-500 path must emit a console.error so Cloudflare Logs is non-empty'); + assert(errors.length > 0, 'diagnostic log must fire for the unhealed octet-stream path'); const payload = errors[0].find((a) => a && typeof a === 'object'); assert(payload, 'log must include a structured diagnostic payload'); - assert.strictEqual(payload.contentType, undefined, 'payload must record contentType (undefined when source metadata is missing)'); - assert.strictEqual(payload.hadLabel, true, 'payload must record whether a label was supplied'); - assert.strictEqual(payload.currentStatus, 200, 'payload must record the source-object status'); + assert.strictEqual(payload.contentType, 'application/octet-stream'); + assert.strictEqual(payload.inferredType, 'application/octet-stream', 'inference must fall through for unknown ext'); + assert.strictEqual(payload.ext, 'bin'); + assert.strictEqual(payload.hadLabel, true); + assert.strictEqual(payload.currentStatus, 200); + }); + it('plain PUT (no label) still skips auto-version for non-html/json contentType', async () => { + // Companion regression: the labelled-path mime inference must NOT bleed into plain + // PUTs. Auto-versioning on plain PUT must still gate to html/json (no extra storage cost). + const versionWrites = []; + const mainWrites = []; + + const mockGetObject = async () => ({ + body: 'binary content', + contentType: 'application/octet-stream', + contentLength: 14, + status: 200, + metadata: { + id: 'octet-id', version: 'v1', timestamp: '123', users: '[]', path: '/legacy/file', + }, + etag: '"etag1"', + }); + + const versionClient = { + async send(cmd) { + versionWrites.push(cmd.input); + return { $metadata: { httpStatusCode: 200 } }; + }, + }; + const mainClient = { + async send(cmd) { + mainWrites.push(cmd.input); + return { $metadata: { httpStatusCode: 200 } }; + }, + }; + + const { putObjectWithVersion } = await esmock('../../../src/storage/version/put.js', { + '../../../src/storage/object/get.js': { default: mockGetObject }, + '../../../src/storage/utils/version.js': { + ifNoneMatch: () => versionClient, + ifMatch: () => mainClient, + }, + }); + + const daCtx = { + org: 'o', ext: 'bin', site: 'mysite', users: [{ email: 'u@x.com' }], + }; + const update = { + org: 'o', key: 'mysite/legacy/file', body: 'new', type: 'application/octet-stream', + }; + const resp = await putObjectWithVersion({}, daCtx, update, true); + + assert.strictEqual(resp.status, 200); + assert.strictEqual(versionWrites.length, 0, 'no version snapshot for plain octet-stream PUT without label'); + assert.strictEqual(mainWrites.length, 1, 'main object updated once'); }); it('returns 201 when version client body is a ReadableStream that would be disturbed by SDK retry', async () => {