Skip to content

fix(version): heal ContentType for legacy octet-stream files on labelled version #288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kptdobe merged 3 commits into from
May 29, 2026
Merged

fix(version): heal ContentType for legacy octet-stream files on labelled version #288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
76401da
fix(version): create labelled versions for non-html/json content types
kptdobe May 29, 2026
fc74b28
test(version): drop ticket-id references from test names and comments
kptdobe May 29, 2026
bbded36
fix(version): infer ContentType from extension instead of widening th…
kptdobe May 29, 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
21 changes: 19 additions & 2 deletions src/storage/version/put.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}) {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 };
Expand All @@ -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,
});
Expand Down
151 changes: 129 additions & 22 deletions test/storage/version/put.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: '<html>legacy</html>',
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', {
Expand All @@ -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 = [];
Expand All @@ -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 () => {
Expand Down
Loading