ronantakizawa/codereview-bench huggingface dataset:
Repository: directus/directus
File: api/src/schedules/retention.ts (TypeScript)

Code before:
				.limit(batch);

			if (task.where) {
				subquery.where(...task.where);
			}

			if (task.join) {
				subquery.join(...task.join);
			}

			try {
				// reset count
				count = 0;

				// subquery executed here as mysql/maria does not allow limit within an IN statement
				const records = await subquery;

				if (records.length !== 0) {
					count = await database(task.collection)
						.where(
							'id',
							'in',
							records.map((r) => r.id),
						)
						.delete();
				}
			} catch (error) {
				logger.error(error, `Retention failed for Collection ${task.collection}`);

				break;
			}

			// Update lock time to prevent concurrent runs
			await lock.set(retentionLockKey, Date.now());
		} while (count >= batch);
	}

	await lock.delete(retentionLockKey);
}

/**
 * Schedule the retention tracking
 *
 * @returns Whether or not retention has been initialized
 */
export default async function schedule(): Promise<boolean> {
	const env = useEnv();

	if (!toBoolean(env['RETENTION_ENABLED'])) {
		return false;
	}

Review comment (performance):
Could we extract this as a generic helper (if possible) so that most vendors run the efficient single query delete?

There's also the `whereIn` function that we could use.

Code after:
				.limit(batch);

			if (task.where) {
				subquery.where(...task.where);
			}

			if (task.join) {
				subquery.join(...task.join);
			}

			try {
				let records = [];
				const isMySQL = helpers.schema.isOneOfClients(['mysql']);

				// mysql/maria does not allow limit within a subquery
				// https://dev.mysql.com/doc/refman/8.4/en/subquery-restrictions.html
				if (isMySQL) {
					records = await subquery.then((r) => r.map((r) => r.id));

					if (records.length === 0) {
						break;
					}
				}

				count = await database(task.collection)
					.whereIn('id', isMySQL ? records : subquery)
					.delete();
			} catch (error) {
				logger.error(error, `Retention failed for Collection ${task.collection}`);

				break;
			}

			// Update lock time to prevent concurrent runs
			await lock.set(retentionLockKey, Date.now());
		} while (count >= batch);
	}

	await lock.delete(retentionLockKey);
}

/**
 * Schedule the retention tracking
 *
 * @returns Whether or not retention has been initialized
 */
export default async function schedule(): Promise<boolean> {
	const env = useEnv();

	if (!toBoolean(env['RETENTION_ENABLED'])) {
		return false;
Repository: directus/directus
File: api/src/services/fields.ts (TypeScript)

Code before:

				// Cleanup directus_fields
				const metaRow = await trx
					.select('collection', 'field')
					.from('directus_fields')
					.where({ collection, field })
					.first();

				if (metaRow) {
					// Handle recursive FK constraints
					await trx('directus_fields')
						.update({ group: null })
						.where({ group: metaRow.field, collection: metaRow.collection });
				}

				const fieldIds = (await trx('directus_fields').select('id').where({ collection, field })).map(
					(field) => field.id,
				);

				const itemsService = new ItemsService('directus_fields', {
					knex: trx,
					accountability: this.accountability,
					schema: this.schema,
				});

				await itemsService.deleteMany(fieldIds, { emitEvents: false });
			});

			const actionEvent = {
				event: 'fields.delete',
				meta: {
					payload: [field],
					collection: collection,
				},
				context: {
					database: this.knex,
					schema: this.schema,
					accountability: this.accountability,
				},
			};

			if (opts?.bypassEmitAction) {
				opts.bypassEmitAction(actionEvent);
			} else {
				nestedActionEvents.push(actionEvent);
			}
		} finally {
			if (runPostColumnChange) {
				await this.helpers.schema.postColumnChange();
			}


Review comment (refactor):
Could we simplify this with a 

```js
itemsService.deleteByQuery({
  filter: {
    collection: { _eq: collection },
    field: { _eq: field }
  }
})
```

?

Code after:

				// Cleanup directus_fields
				const metaRow = await trx
					.select('collection', 'field')
					.from('directus_fields')
					.where({ collection, field })
					.first();

				if (metaRow) {
					// Handle recursive FK constraints
					await trx('directus_fields')
						.update({ group: null })
						.where({ group: metaRow.field, collection: metaRow.collection });
				}

				const itemsService = new ItemsService('directus_fields', {
					knex: trx,
					accountability: this.accountability,
					schema: this.schema,
				});

				await itemsService.deleteByQuery(
					{
						filter: {
							collection: { _eq: collection },
							field: { _eq: field },
						},
					},
					{ emitEvents: false },
				);
			});

			const actionEvent = {
				event: 'fields.delete',
				meta: {
					payload: [field],
					collection: collection,
				},
				context: {
					database: this.knex,
					schema: this.schema,
					accountability: this.accountability,
				},
			};

			if (opts?.bypassEmitAction) {
				opts.bypassEmitAction(actionEvent);
			} else {
				nestedActionEvents.push(actionEvent);
			}
		} finally {
Repository: directus/directus
File: app/src/views/private/components/render-template.vue (Vue)

Code before:
				];
			} else if (field?.meta?.display) {
				return value.map((v) => {
					return {
						component: field.meta?.display,
						options: field.meta?.display_options,
						value: v,
						interface: field.meta?.interface,
						interfaceOptions: field.meta?.options,
						type: field.type,
						collection: field.collection,
						field: field.field,
					};
				});
			} else {
				return value;
			}
		})
		.map((p) => p ?? null),
);
</script>

<template>
	<div ref="templateEl" class="render-template">
		<span class="vertical-aligner" />
		<template v-for="(part, index) in parts" :key="index">
			<template v-for="(subPart, subIndex) in part" :key="subIndex">
				<v-error-boundary>
					<value-null
						v-if="
							subPart === null ||
							(typeof subPart === 'object' && (subPart.value === null || subPart.value === undefined))
						"
					/>
					<template v-else-if="subPart?.component">
						<component
							:is="`display-${subPart.component}`"
							v-bind="subPart.options"
							:value="subPart.value"
							:interface="subPart.interface"
							:interface-options="subPart.interfaceOptions"
							:type="subPart.type"
							:collection="subPart.collection"
							:field="subPart.field"
						/>
						<span>&nbsp;</span>
					</template>
					<span v-else-if="typeof subPart === 'string'" :dir="direction">{{ translate(subPart) }}</span>
					<span v-else>{{ subPart }}</span>
					<template #fallback>
						<span>{{ subPart?.value || subPart }}</span>

Review comment (suggestion):
We should avoid using array indices for the `:key` attribute.

Code after:
				];
			} else if (component) {
				return value.map((v) => {
					return {
						component: component,
						options: field.meta?.display_options,
						value: v,
						interface: field.meta?.interface,
						interfaceOptions: field.meta?.options,
						type: field.type,
						collection: field.collection,
						field: field.field,
					};
				});
			} else {
				return value;
			}
		})
		.map((p) => p ?? null),
);
</script>

<template>
	<div ref="templateEl" class="render-template">
		<span class="vertical-aligner" />
		<template v-for="(part, index) in parts" :key="index">
			<template v-for="(subPart, subIndex) in part" :key="subIndex">
				<v-error-boundary>
					<value-null
						v-if="
							subPart === null ||
							(typeof subPart === 'object' && (subPart.value === null || subPart.value === undefined))
						"
					/>
					<template v-else-if="subPart?.component">
						<component
							:is="`display-${subPart.component}`"
							v-bind="subPart.options"
							:value="subPart.value"
							:interface="subPart.interface"
							:interface-options="subPart.interfaceOptions"
							:type="subPart.type"
							:collection="subPart.collection"
							:field="subPart.field"
						/>
						<span>&nbsp;</span>
					</template>
					<span v-else-if="typeof subPart === 'string'" :dir="direction">{{ translate(subPart) }}</span>
					<span v-else>{{ subPart }}</span>
					<template #fallback>
						<span>{{ subPart?.value || subPart }}</span>
Repository: directus/directus
File: app/src/displays/formatted-value/formatted-value.vue (Vue)

Code before:
			text: '',
		},
	);
});

const computedStyle = computed(() => {
	return {
		color: computedFormat.value.color,
		borderStyle: 'solid',
		borderWidth: props.border ? '2px' : 0,
		borderColor: computedFormat.value.color,
		backgroundColor: computedFormat.value.background ?? 'transparent',
	};
});

const displayValue = computed(() => {
	if (props.masked) return '**********';

	if (computedFormat.value.text) {
		const { text } = computedFormat.value;
		return text.startsWith('$t:') ? t(text.slice(3)) : text;
	}

	if (isNil(props.value) || props.value === '') return null;

	let value = Array.isArray(props.value) ? props.value.join(', ') : String(props.value);

	if (props.translate && value.startsWith('$t:')) {
		value = t(value.slice(3));
	}

	// Strip out all HTML tags
	value = dompurify.sanitize(value, { ALLOWED_TAGS: [] });

	// Decode any HTML encoded characters (like &copy;)
	value = decode(value);

	if (props.format) {
		if (['string', 'text'].includes(props.type)) {
			value = formatTitle(value);
		} else if (['float', 'decimal'].includes(props.type)) {
			value = n(parseFloat(value));
		} else {
			value = n(parseInt(value));
		}
	}

	const prefix = props.prefix ?? '';
	const suffix = props.suffix ?? '';

	return `${prefix}${value}${suffix}`;

Review comment (suggestion):
To prevent strings like `, Item 2, , `, we need to ensure that each array element exists, like `props.value.map((p) => p || p === 0 ? p : '--').join(', ')`. Ideally we should use the `<value-null>` component instead of `'--'`, but I’ll leave that up to you.

Code after:
			text: '',
		},
	);
});

const computedStyle = computed(() => {
	return {
		color: computedFormat.value.color,
		borderStyle: 'solid',
		borderWidth: props.border ? '2px' : 0,
		borderColor: computedFormat.value.color,
		backgroundColor: computedFormat.value.background ?? 'transparent',
	};
});

const displayValue = computed(() => {
	if (props.masked) return '**********';

	if (computedFormat.value.text) {
		const { text } = computedFormat.value;
		return text.startsWith('$t:') ? t(text.slice(3)) : text;
	}

	if (isNil(props.value) || props.value === '') return null;

	let value = Array.isArray(props.value)
		? props.value.map((v) => (isNil(v) || v === '' ? '--' : v)).join(', ')
		: String(props.value);

	if (props.translate && value.startsWith('$t:')) {
		value = t(value.slice(3));
	}

	// Strip out all HTML tags
	value = dompurify.sanitize(value, { ALLOWED_TAGS: [] });

	// Decode any HTML encoded characters (like &copy;)
	value = decode(value);

	if (props.format) {
		if (['string', 'text'].includes(props.type)) {
			value = formatTitle(value);
		} else if (['float', 'decimal'].includes(props.type)) {
			value = n(parseFloat(value));
		} else {
			value = n(parseInt(value));
		}
	}

	const prefix = props.prefix ?? '';
	const suffix = props.suffix ?? '';
Repository: directus/directus
File: tests/blackbox/tests/db/routes/permissions/cache-purge.test.ts (TypeScript)

Code before:
import config, { getUrl, paths, type Env } from '@common/config';
import vendors, { type Vendor } from '@common/get-dbs-to-test';
import { ROLE, USER } from '@common/variables';
import { awaitDirectusConnection } from '@utils/await-connection';
import { ChildProcess, spawn } from 'child_process';
import getPort from 'get-port';
import { cloneDeep } from 'lodash-es';
import request from 'supertest';
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
import { collection, type Collection } from './cache-purge.seed';
import { CreatePolicy } from '@common/functions';

describe('Permissions Cache Purging Tests', () => {
	const cacheStatusHeader = 'x-cache-status';

	const action = 'read';
	const updatedAction = 'update';
	let policyId: string;

	const mutations = {
		createOne: async (vendor: Vendor, env: Env) => {
			const item = { collection, action, policy: policyId };

			await clearCacheAndFetchOnce(vendor, env);

			await request(getUrl(vendor, env))
				.post('/permissions')
				.send(item)

Review comment (suggestion):
```suggestion
import { USER } from '@common/variables';
```

Code after:
import config, { getUrl, paths, type Env } from '@common/config';
import vendors, { type Vendor } from '@common/get-dbs-to-test';
import { USER } from '@common/variables';
import { awaitDirectusConnection } from '@utils/await-connection';
import { ChildProcess, spawn } from 'child_process';
import getPort from 'get-port';
import { cloneDeep } from 'lodash-es';
import request from 'supertest';
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
import { collection, type Collection } from './cache-purge.seed';
import { CreatePolicy } from '@common/functions';

describe('Permissions Cache Purging Tests', () => {
	const cacheStatusHeader = 'x-cache-status';

	const action = 'read';
	const updatedAction = 'update';
	let policyId: string;

	const mutations = {
		createOne: async (vendor: Vendor, env: Env) => {
			const item = { collection, action, policy: policyId };

			await clearCacheAndFetchOnce(vendor, env);

			await request(getUrl(vendor, env))
				.post('/permissions')
				.send(item)
Repository: directus/directus
File: api/src/utils/permissions-cachable.ts (TypeScript)

Code before:
import type { Accountability, Filter } from '@directus/types';
import { fetchPermissions } from '../permissions/lib/fetch-permissions.js';
import { fetchPolicies } from '../permissions/lib/fetch-policies.js';
import type { Context } from '../permissions/types.js';
import { createDefaultAccountability } from '../permissions/utils/create-default-accountability.js';

/**
 * Check if the read permissions for a collection contain the dynamic variable $NOW.
 * If they do, the permissions are not cachable.
 */
export async function permissionsCachable(collection: string, context: Context, accountability?: Accountability) {
	if (!accountability) {
		accountability = createDefaultAccountability();
	}

	const policies = await fetchPolicies(accountability, context);

	const permissions = await fetchPermissions(
		{ action: 'read', policies, collections: [collection], accountability, bypassDynamicVariableProcessing: true },
		context,
	);

	return permissions.some((permission) => {
		if (!permission.permissions) {
			return true;
		}

		return !filter_has_now(permission.permissions);
	});
}

export function filter_has_now(filter: Filter): boolean {
	return Object.entries(filter).some(([key, value]) => {
		if (key === '_and' || key === '_or') {
			return (value as Filter[]).some((sub_filter) => filter_has_now(sub_filter));
		} else if (typeof value === 'object') {
			return filter_has_now(value);
		} else if (typeof value === 'string') {
			return value.startsWith('$NOW');
		}

		return false;
	});
}


Review comment (security):
Due to `req.collection` not being defined for all endpoints should we early return for those case (e.g. `POST /auth/login`)?

Code after:
import type { Accountability, Filter } from '@directus/types';
import { fetchPermissions } from '../permissions/lib/fetch-permissions.js';
import { fetchPolicies } from '../permissions/lib/fetch-policies.js';
import type { Context } from '../permissions/types.js';
import { createDefaultAccountability } from '../permissions/utils/create-default-accountability.js';

/**
 * Check if the read permissions for a collection contain the dynamic variable $NOW.
 * If they do, the permissions are not cachable.
 */
export async function permissionsCachable(
	collection: string | undefined,
	context: Context,
	accountability?: Accountability,
) {
	if (!collection) {
		return true;
	}

	if (!accountability) {
		accountability = createDefaultAccountability();
	}

	const policies = await fetchPolicies(accountability, context);

	const permissions = await fetchPermissions(
		{ action: 'read', policies, collections: [collection], accountability, bypassDynamicVariableProcessing: true },
		context,
	);

	const has_now = permissions.some((permission) => {
		if (!permission.permissions) {
			return false;
		}

		return filter_has_now(permission.permissions);
	});

	return !has_now;
}

export function filter_has_now(filter: Filter): boolean {
	return Object.entries(filter).some(([key, value]) => {
		if (key === '_and' || key === '_or') {
			return (value as Filter[]).some((sub_filter) => filter_has_now(sub_filter));
Repository: directus/directus
File: api/src/utils/permissions-cachable.test.ts (TypeScript)

Code before:
			{
				created_on: {
					_gt: '2021-01-01',
				},
			},
		],
	};

	expect(filter_has_now(filter)).toBe(false);

	filter = {
		_or: [
			{
				created_on: {
					some: {
						_gt: '2021-01-01',
					},
				},
			},
		],
	};

	expect(filter_has_now(filter)).toBe(false);
});

test('permissions cachable on many policies', async () => {
	vi.mocked(fetchPolicies).mockResolvedValue(['policy1', 'policy2', 'policy3']);

	const permissions: Permission[] = [
		{
			action: 'read',
			collection: 'items',
			fields: ['*'],
			permissions: {
				created_on: {
					_gt: '2021-01-01',
				},
			},
			policy: 'policy1',
			presets: [],
			validation: null,
		},
		{
			action: 'read',
			collection: 'items',
			fields: ['*'],
			permissions: {
				created_on: {
					_gt: '$NOW',
				},
			},

Review comment (security):
```suggestion
test('permissions are not cached on many policies with $NOW', async () => {
```

Code after:
			{
				created_on: {
					_gt: '2021-01-01',
				},
			},
		],
	};

	expect(filter_has_now(filter)).toBe(false);

	filter = {
		_or: [
			{
				created_on: {
					some: {
						_gt: '2021-01-01',
					},
				},
			},
		],
	};

	expect(filter_has_now(filter)).toBe(false);
});

test('permissions are not cacheable on many policies with $NOW', async () => {
	vi.mocked(fetchPolicies).mockResolvedValue(['policy1', 'policy2', 'policy3']);

	const permissions: Permission[] = [
		{
			action: 'read',
			collection: 'items',
			fields: ['*'],
			permissions: {
				created_on: {
					_gt: '2021-01-01',
				},
			},
			policy: 'policy1',
			presets: [],
			validation: null,
		},
		{
			action: 'read',
			collection: 'items',
			fields: ['*'],
			permissions: {
				created_on: {
					_gt: '$NOW',
				},
			},
Repository: directus/directus
File: api/src/utils/permissions-cachable.test.ts (TypeScript)

Code before:
			presets: [],
			validation: null,
		},
		{
			action: 'read',
			collection: 'items',
			fields: ['*'],
			permissions: {
				created_on: {
					_gt: '$NOW',
				},
			},
			policy: 'policy1',
			presets: [],
			validation: null,
		},
	];

	vi.mocked(fetchPermissions).mockResolvedValue(permissions);

	const result = await permissionsCachable('items', {} as any, {} as any);

	expect(result).toBe(false);
});

test('permissions cachable on many policies', async () => {
	vi.mocked(fetchPolicies).mockResolvedValue(['policy1', 'policy2', 'policy3']);

	const permissions: Permission[] = [
		{
			action: 'read',
			collection: 'items',
			fields: ['*'],
			permissions: {
				created_on: {
					_gt: '2021-01-01',
				},
			},
			policy: 'policy1',
			presets: [],
			validation: null,
		},
		{
			action: 'read',
			collection: 'items',
			fields: ['*'],
			permissions: {
				created_on: {
					_gt: '2021-01-01',
				},
			},

Review comment (security):
```suggestion
test('permissions are cacheable on many policies without $NOW', async () => {
```

Code after:
			presets: [],
			validation: null,
		},
		{
			action: 'read',
			collection: 'items',
			fields: ['*'],
			permissions: {
				created_on: {
					_gt: '$NOW',
				},
			},
			policy: 'policy1',
			presets: [],
			validation: null,
		},
	];

	vi.mocked(fetchPermissions).mockResolvedValue(permissions);

	const result = await permissionsCachable('items', {} as any, {} as any);

	expect(result).toBe(false);
});

test('permissions are cacheable on many policies without $NOW', async () => {
	vi.mocked(fetchPolicies).mockResolvedValue(['policy1', 'policy2', 'policy3']);

	const permissions: Permission[] = [
		{
			action: 'read',
			collection: 'items',
			fields: ['*'],
			permissions: {
				created_on: {
					_gt: '2021-01-01',
				},
			},
			policy: 'policy1',
			presets: [],
			validation: null,
		},
		{
			action: 'read',
			collection: 'items',
			fields: ['*'],
			permissions: {
				created_on: {
					_gt: '2021-01-01',
				},
			},
Repository: directus/directus
File: app/src/modules/content/routes/item.vue (Vue)

Code before:
				currentVersion.value = null;
				break;
			case 'stay':
				revisionsDrawerDetailRef.value?.refresh?.();
				refresh();
				break;
			case 'quit':
				if (!props.singleton) router.push(`/content/${props.collection}`);
				break;
		}
	} catch {
		// Save version shows unexpected error dialog
	}
}

async function saveAndStay() {
	if (isSavable.value === false) return;

	try {
		const savedItem: Record<string, any> = await save();

		if (props.primaryKey === '+') {
			const newPrimaryKey = savedItem[primaryKeyField.value!.field];

			router.replace(getItemRoute(props.collection, newPrimaryKey));
		} else {
			revisionsDrawerDetailRef.value?.refresh?.();
			refresh();
		}
	} catch {
		// Save shows unexpected error dialog
	}
}

async function saveAndAddNew() {
	if (isSavable.value === false) return;
	if (unref(currentVersion) !== null) return;

	try {
		await save();

		if (isNew.value === true) {
			refresh();
		} else {
			router.push(getItemRoute(props.collection, '+'));
		}
	} catch {
		// Save shows unexpected error dialog
	}
}


Review comment (suggestion):
![Screenshot 2025-02-27 at 11 35 17 AM](https://github.com/user-attachments/assets/f2464cbd-2162-41b8-b893-42cd51c49e5f)

I think because "Save and stay" is now the default, we no longer need it under the options menu.

Furthermore, I think we should maybe add "Save and return to main" as an option to replace "Save and stay"


Code after:
	return { breadcrumb };
}

async function saveVersionAction(action: 'main' | 'stay' | 'quit') {
	if (isSavable.value === false) return;

	try {
		await saveVersion(edits);
		edits.value = {};

		switch (action) {
			case 'main':
				currentVersion.value = null;
				break;
			case 'stay':
				revisionsDrawerDetailRef.value?.refresh?.();
				refresh();
				break;
			case 'quit':
				if (!props.singleton) router.push(`/content/${props.collection}`);
				break;
		}
	} catch {
		// Save version shows unexpected error dialog
	}
}

async function saveAndStay() {
	if (isSavable.value === false) return;

	try {
		const savedItem: Record<string, any> = await save();

		if (props.primaryKey === '+') {
			const newPrimaryKey = savedItem[primaryKeyField.value!.field];

			router.replace(getItemRoute(props.collection, newPrimaryKey));
		} else {
			revisionsDrawerDetailRef.value?.refresh?.();
			refresh();
		}
	} catch {
		// Save shows unexpected error dialog
	}
}

async function saveAndAddNew() {
	if (isSavable.value === false) return;
	if (unref(currentVersion) !== null) return;

	try {