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

Code before:
		this.cache = cache;
		this.systemCache = systemCache;
	}

	private get hasReadAccess() {
		return !!this.accountability?.permissions?.find((permission) => {
			return permission.collection === 'directus_fields' && permission.action === 'read';
		});
	}

	async columnInfo(collection?: string): Promise<Column[]>;
	async columnInfo(collection: string, field: string): Promise<Column>;
	async columnInfo(collection?: string, field?: string): Promise<Column | Column[]> {
		const schemaCacheIsEnabled = Boolean(env['CACHE_SCHEMA']);

		let columnInfo: Column[] | null = null;

		if (schemaCacheIsEnabled) {
			columnInfo = await getCacheValue(this.systemCache, 'columnInfo');
		}

		if (!columnInfo) {
			columnInfo = await this.schemaInspector.columnInfo();

			if (schemaCacheIsEnabled) {
				setCacheValue(this.systemCache, 'columnInfo', columnInfo);
			}
		}

		if (collection) {
			columnInfo = columnInfo.filter((column) => column.table === collection);
		}

		if (field) {
			return columnInfo.find((column) => column.name === field) as Column;
		}

		return columnInfo;
	}

	async readAll(collection?: string): Promise<Field[]> {
		let fields: FieldMeta[];

		if (this.accountability && this.accountability.admin !== true && this.hasReadAccess === false) {
			throw new ForbiddenError();
		}

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

Review comment (performance):
Could we cache in the `this.localSchemaCache` instead of `systemCache`? The schema cache was updated to be local as the data could be big. 🤔 

Code after:

		this.cache = cache;
		this.systemCache = systemCache;
		this.schemaCache = localSchemaCache;
	}

	private get hasReadAccess() {
		return !!this.accountability?.permissions?.find((permission) => {
			return permission.collection === 'directus_fields' && permission.action === 'read';
		});
	}

	async columnInfo(collection?: string): Promise<Column[]>;
	async columnInfo(collection: string, field: string): Promise<Column>;
	async columnInfo(collection?: string, field?: string): Promise<Column | Column[]> {
		const schemaCacheIsEnabled = Boolean(env['CACHE_SCHEMA']);

		let columnInfo: Column[] | null = null;

		if (schemaCacheIsEnabled) {
			columnInfo = await getCacheValue(this.schemaCache, 'columnInfo');
		}

		if (!columnInfo) {
			columnInfo = await this.schemaInspector.columnInfo();

			if (schemaCacheIsEnabled) {
				setCacheValue(this.schemaCache, 'columnInfo', columnInfo);
			}
		}

		if (collection) {
			columnInfo = columnInfo.filter((column) => column.table === collection);
		}

		if (field) {
			return columnInfo.find((column) => column.name === field) as Column;
		}

		return columnInfo;
	}

	async readAll(collection?: string): Promise<Field[]> {
		let fields: FieldMeta[];

		if (this.accountability && this.accountability.admin !== true && this.hasReadAccess === false) {
			throw new ForbiddenError();
		}

		const nonAuthorizedItemsService = new ItemsService('directus_fields', {
			knex: this.knex,
Repository: directus/directus
File: app/src/displays/related-values/related-values.vue (Vue)

Code before:
					{{ unit }}
				</span>
			</span>
		</template>

		<v-list class="links">
			<v-list-item v-for="item in value" :key="item[primaryKeyFieldPath!]">
				<v-list-item-content>
					<render-template
						:template="internalTemplate"
						:item="item"
						:collection="junctionCollection ?? relatedCollection"
					/>
				</v-list-item-content>
				<v-list-item-icon>
					<router-link :to="getLinkForItem(item)!"><v-icon name="launch" small /></router-link>
				</v-list-item-icon>
			</v-list-item>
		</v-list>
	</v-menu>
	<render-template v-else :template="internalTemplate" :item="value" :collection="relatedCollection" />
</template>

<style lang="scss" scoped>
$toggle-px: 6px;
$toggle-py: 4px;

.toggle {
	position: relative;

	&::before {
		position: absolute;
		top: #{$toggle-py * -1};
		left: #{$toggle-px * -1};
		z-index: 1;
		width: calc(100% + #{$toggle-px * 2});
		height: calc(100% + #{$toggle-py * 2});
		background-color: var(--theme--background-normal);
		border-radius: var(--theme--border-radius);
		opacity: 0;
		transition: opacity var(--fast) var(--transition);
		content: '';
	}

	.label {
		position: relative;
		z-index: 2;
	}

	&:not(.disabled):hover::before {
		opacity: 1;

Review comment (suggestion):
I don't think we're using SCSS variables in other components.
For the sake of consistency, let's use native CSS `var`.

Code after:
					{{ unit }}
				</span>
			</span>
		</template>

		<v-list class="links">
			<v-list-item v-for="item in value" :key="item[primaryKeyFieldPath!]">
				<v-list-item-content>
					<render-template
						:template="internalTemplate"
						:item="item"
						:collection="junctionCollection ?? relatedCollection"
					/>
				</v-list-item-content>
				<v-list-item-icon>
					<router-link :to="getLinkForItem(item)!"><v-icon name="launch" small /></router-link>
				</v-list-item-icon>
			</v-list-item>
		</v-list>
	</v-menu>
	<render-template v-else :template="internalTemplate" :item="value" :collection="relatedCollection" />
</template>

<style lang="scss" scoped>
.toggle {
	position: relative;
	--toggle-px: 6px;
	--toggle-py: 4px;

	&::before {
		position: absolute;
		top: calc(-1 * var(--toggle-py));
		left: calc(-1 * var(--toggle-px));
		z-index: 1;
		width: calc(100% + var(--toggle-px) * 2);
		height: calc(100% + var(--toggle-py) * 2);
		background-color: var(--theme--background-normal);
		border-radius: var(--theme--border-radius);
		opacity: 0;
		transition: opacity var(--fast) var(--transition);
		content: '';
	}

	.label {
		position: relative;
		z-index: 2;
	}

	&:not(.disabled):hover::before {
		opacity: 1;
	}
Repository: directus/directus
File: api/src/telemetry/types/report.ts (TypeScript)

Code before:
	items: number;

	/**
	 * Number of files in the system
	 */
	files: number;

	/**
	 * Number of shares in the system
	 */
	shares: number;

	/**
	 * Maximum number of fields in a collection
	 */
	fields_max: number;

	/**
	 * Number of fields in the system
	 */
	fields_total: number;

	/**
	 * Size of the database
	 */
	database_size: number;
}


Review comment (suggestion):
```suggestion
	/**
	 * Size of the database in bytes
	 */
	database_size: number;
```

Code after:
	items: number;

	/**
	 * Number of files in the system
	 */
	files: number;

	/**
	 * Number of shares in the system
	 */
	shares: number;

	/**
	 * Maximum number of fields in a collection
	 */
	fields_max: number;

	/**
	 * Number of fields in the system
	 */
	fields_total: number;

	/**
	 * Size of the database in bytes
	 */
	database_size: number;
}
Repository: directus/directus
File: api/src/database/helpers/schema/types.ts (TypeScript)

Code before:
		return 'CAST(?? AS CHAR(255))';
	}

	applyMultiRelationalSort(
		knex: Knex,
		dbQuery: Knex.QueryBuilder,
		table: string,
		primaryKey: string,
		orderByString: string,
		orderByFields: Knex.Raw[],
	): Knex.QueryBuilder {
		dbQuery.rowNumber(
			knex.ref('directus_row_number').toQuery(),
			knex.raw(`partition by ?? order by ${orderByString}`, [`${table}.${primaryKey}`, ...orderByFields]),
		);

		return dbQuery;
	}

	formatUUID(uuid: string): string {
		return uuid; // no-op by default
	}

	async getDatabaseSize(): Promise<number> {
		return 0;
	}
}


Review comment (refactor):
```suggestion

	/**
	 * @returns Size of the database in bytes
	 */
	async getDatabaseSize(): Promise<number> {
		return 0;
	}
```

Adding the jsdoc in the abstract class shows it then in every place (dialects included) which is helpful imo :) 

Example:

![image](https://github.com/directus/directus/assets/14810858/0b847ae6-7a99-4e58-9d99-bd6bda93f1e0)


Code after:
		return 'CAST(?? AS CHAR(255))';
	}

	applyMultiRelationalSort(
		knex: Knex,
		dbQuery: Knex.QueryBuilder,
		table: string,
		primaryKey: string,
		orderByString: string,
		orderByFields: Knex.Raw[],
	): Knex.QueryBuilder {
		dbQuery.rowNumber(
			knex.ref('directus_row_number').toQuery(),
			knex.raw(`partition by ?? order by ${orderByString}`, [`${table}.${primaryKey}`, ...orderByFields]),
		);

		return dbQuery;
	}

	formatUUID(uuid: string): string {
		return uuid; // no-op by default
	}

	/**
	 * @returns Size of the database in bytes
	 */
	async getDatabaseSize(): Promise<number | null> {
		return null;
	}
}
Repository: directus/directus
File: api/src/database/helpers/schema/types.ts (TypeScript)

Code before:

	applyMultiRelationalSort(
		knex: Knex,
		dbQuery: Knex.QueryBuilder,
		table: string,
		primaryKey: string,
		orderByString: string,
		orderByFields: Knex.Raw[],
	): Knex.QueryBuilder {
		dbQuery.rowNumber(
			knex.ref('directus_row_number').toQuery(),
			knex.raw(`partition by ?? order by ${orderByString}`, [`${table}.${primaryKey}`, ...orderByFields]),
		);

		return dbQuery;
	}

	formatUUID(uuid: string): string {
		return uuid; // no-op by default
	}

	/**
	 * @returns Size of the database in bytes
	 */
	async getDatabaseSize(): Promise<number> {
		return 0;
	}
}


Review comment (bug):
Should we return `null` here as well as for failing `getDatabaseSize` calls in the dialects implementation so we can identify / distinguish these cases?

Code after:

	applyMultiRelationalSort(
		knex: Knex,
		dbQuery: Knex.QueryBuilder,
		table: string,
		primaryKey: string,
		orderByString: string,
		orderByFields: Knex.Raw[],
	): Knex.QueryBuilder {
		dbQuery.rowNumber(
			knex.ref('directus_row_number').toQuery(),
			knex.raw(`partition by ?? order by ${orderByString}`, [`${table}.${primaryKey}`, ...orderByFields]),
		);

		return dbQuery;
	}

	formatUUID(uuid: string): string {
		return uuid; // no-op by default
	}

	/**
	 * @returns Size of the database in bytes
	 */
	async getDatabaseSize(): Promise<number | null> {
		return null;
	}
}
Repository: directus/directus
File: api/src/telemetry/lib/get-report.ts (TypeScript)

Code before:
		getExtensionCount(db),
		helpers.schema.getDatabaseSize(),
	]);

	return {
		url: env['PUBLIC_URL'] as string,
		version: version,
		database: getDatabaseClient(),

		dashboards: basicCounts.directus_dashboards,
		files: basicCounts.directus_files,
		flows: basicCounts.directus_flows,
		roles: basicCounts.directus_roles,
		shares: basicCounts.directus_shares,

		users_admin: userCounts.admin,
		users_app: userCounts.app,
		users_api: userCounts.api,

		collections: userItemCount.collections,
		items: userItemCount.items,

		fields_max: fieldsCounts.max,
		fields_total: fieldsCounts.total,

		extensions: extensionsCounts.activeTotal - extensionsCounts.activeBundles,

		database_size: databaseSize ?? 0,
	};
};


Review comment (suggestion):
Like mentioned in the comment for `get-extension-count.ts` we should do all the work inside of the function because `activeTotal` currently has an ambiguous meaning.

Code after:
		getExtensionCount(db),
		helpers.schema.getDatabaseSize(),
	]);

	return {
		url: env['PUBLIC_URL'] as string,
		version: version,
		database: getDatabaseClient(),

		dashboards: basicCounts.directus_dashboards,
		files: basicCounts.directus_files,
		flows: basicCounts.directus_flows,
		roles: basicCounts.directus_roles,
		shares: basicCounts.directus_shares,

		admin_users: userCounts.admin,
		app_users: userCounts.app,
		api_users: userCounts.api,

		collections: userItemCount.collections,
		items: userItemCount.items,

		fields_max: fieldsCounts.max,
		fields_total: fieldsCounts.total,

		extensions: extensionsCounts.totalEnabled,

		database_size: databaseSize ?? 0,
	};
};
Repository: directus/directus
File: api/src/telemetry/utils/get-extension-count.test.ts (TypeScript)

Code before:
	} as unknown as Knex;
});

afterEach(() => {
	vi.clearAllMocks();
});

test('Fetches extension count from the database', async () => {
	const result = await getExtensionCount(mockDb);

	expect(mockDb.count).toHaveBeenCalledWith('*', { as: 'count' });
	expect(mockDb.from).toHaveBeenCalledWith('directus_extensions');
	expect(mockDb.where).toHaveBeenCalledWith('enabled', '=', true);
	expect(mockDb.distinct).toHaveBeenCalledWith('bundle');
	expect(mockDb.whereIn).toHaveBeenCalledWith('id', expect.anything());
	expect(mockDb.andWhere).toHaveBeenCalledWith('enabled', '=', true);

	expect(mockDb.count).toBeCalledTimes(2);
	expect(mockDb.from).toBeCalledTimes(3);
	expect(mockDb.where).toBeCalledTimes(1);
	expect(mockDb.distinct).toBeCalledTimes(1);
	expect(mockDb.whereIn).toBeCalledTimes(1);
	expect(mockDb.andWhere).toBeCalledTimes(1);

	expect(result).toEqual({ activeTotal: 15, activeBundles: 2 });
});


Review comment (suggestion):
This test is testing the hard coded implementation instead of the result, which means that changing any query logic forces you to update the test, even when the result stays the same. 

Code after:
		bundle,
		meta: {
			enabled,
			bundle,
		},
		schema: {
			type: 'endpoint',
		},
	};
}

function generateExtension(enabled: boolean) {
	return {
		id: randomUUID(),
		meta: {
			enabled,
		},
		schema: {
			type: 'endpoint',
		},
	};
}

function generateMissingExtension(enabled: boolean) {
	return {
		id: randomUUID(),
		meta: {
			enabled,
		},
		schema: null,
	};
}

beforeEach(() => {
	vi.mocked(ExtensionsService.prototype.readAll).mockResolvedValue([
		// Normal extensions
		generateExtension(true),
		generateExtension(true),
		generateExtension(false),
		// Parent bundle extension
		generateParentBundleExtension(true),
		generateParentBundleExtension(true),
		generateParentBundleExtension(false),
		// Extensions within a bundle
		generateNestedBundleExtension(true),
		generateNestedBundleExtension(true),
		generateNestedBundleExtension(false),
		// Missing extensions
		generateMissingExtension(true),
		generateMissingExtension(true),
		generateMissingExtension(false),
Repository: directus/directus
File: api/src/middleware/error-handler.ts (TypeScript)

Code before:
			} else {
				// Don't expose unknown errors to non-admin users
				errors = [{ message: FALLBACK_ERROR.message, extensions: { code: FALLBACK_ERROR.code } }];
			}
		}
	}

	res.status(status ?? FALLBACK_ERROR.status);

	const updatedErrors = await emitter.emitFilter(
		'request.error',
		errors,
		{},
		{
			database: getDatabase(),
			schema: req.schema,
			accountability: req.accountability ?? null,
		},
	);

	return res.json({ errors: updatedErrors });
});

function asyncErrorHandler(fn: ErrorRequestHandler) {
	return (err: any, req: Request, res: Response, next: NextFunction) =>
		Promise.resolve(fn(err, req, res, next)).catch((error) => {
			// To be on the safe side and ensure the response call is reached in any case
			try {
				const logger = useLogger();
				logger.error(error, 'Unexpected error in error handler');
			} catch {
				// Ignore
			}

			res.status(FALLBACK_ERROR.status);
			return res.json({ errors: [{ message: FALLBACK_ERROR.message, extensions: { code: FALLBACK_ERROR.code } }] });
		});
}


Review comment (refactor):
Lets add a comment here to make it very clear that this `Promise.resolve` is here to support both regular and async functions as `fn()`. I personally think thats not directly clear from just the code/types and may be a footgun for future refactoring.

Code after:
			} else {
				// Don't expose unknown errors to non-admin users
				errors = [{ message: FALLBACK_ERROR.message, extensions: { code: FALLBACK_ERROR.code } }];
			}
		}
	}

	res.status(status ?? FALLBACK_ERROR.status);

	const updatedErrors = await emitter.emitFilter(
		'request.error',
		errors,
		{},
		{
			database: getDatabase(),
			schema: req.schema,
			accountability: req.accountability ?? null,
		},
	);

	return res.json({ errors: updatedErrors });
});

function asyncErrorHandler(
	fn: (...args: Parameters<ErrorRequestHandler>) => Promise<any>,
): (...args: Parameters<ErrorRequestHandler>) => Promise<ReturnType<ErrorRequestHandler>> {
	return (err, req, res, next) =>
		fn(err, req, res, next).catch((error) => {
			// To be on the safe side and ensure this doesn't lead to an unhandled (potentially crashing) error
			try {
				const logger = useLogger();
				logger.error(error, 'Unexpected error in error handler');
			} catch {
				// Ignore
			}

			// Delegate to default error handler to close the connection
			if (res.headersSent) return next(err);

			res.status(FALLBACK_ERROR.status);
			return res.json({ errors: [{ message: FALLBACK_ERROR.message, extensions: { code: FALLBACK_ERROR.code } }] });
		});
}
Repository: directus/directus
File: api/src/middleware/error-handler.ts (TypeScript)

Code before:
				];
			} else {
				// Don't expose unknown errors to non-admin users
				errors = [{ message: FALLBACK_ERROR.message, extensions: { code: FALLBACK_ERROR.code } }];
			}
		}
	}

	res.status(status ?? FALLBACK_ERROR.status);

	const updatedErrors = await emitter.emitFilter(
		'request.error',
		errors,
		{},
		{
			database: getDatabase(),
			schema: req.schema,
			accountability: req.accountability ?? null,
		},
	);

	return res.json({ errors: updatedErrors });
});

function asyncErrorHandler(fn: ErrorRequestHandler) {
	return (err: any, req: Request, res: Response, next: NextFunction) =>
		Promise.resolve(fn(err, req, res, next)).catch((error) => {
			// To be on the safe side and ensure the response call is reached in any case
			try {
				const logger = useLogger();
				logger.error(error, 'Unexpected error in error handler');
			} catch {
				// Ignore
			}

			res.status(FALLBACK_ERROR.status);
			return res.json({ errors: [{ message: FALLBACK_ERROR.message, extensions: { code: FALLBACK_ERROR.code } }] });
		});
}


Review comment (suggestion):
```suggestion
    // Note: keep all 4 parameters here. That's how Express recognizes it's the error handler, even if
    // we don't use next
	return (err: any, req: Request, res: Response, next: NextFunction) =>
```

Lets not forget adding back the original comment here

Code after:
				];
			} else {
				// Don't expose unknown errors to non-admin users
				errors = [{ message: FALLBACK_ERROR.message, extensions: { code: FALLBACK_ERROR.code } }];
			}
		}
	}

	res.status(status ?? FALLBACK_ERROR.status);

	const updatedErrors = await emitter.emitFilter(
		'request.error',
		errors,
		{},
		{
			database: getDatabase(),
			schema: req.schema,
			accountability: req.accountability ?? null,
		},
	);

	return res.json({ errors: updatedErrors });
});

function asyncErrorHandler(
	fn: (...args: Parameters<ErrorRequestHandler>) => Promise<any>,
): (...args: Parameters<ErrorRequestHandler>) => Promise<ReturnType<ErrorRequestHandler>> {
	return (err, req, res, next) =>
		fn(err, req, res, next).catch((error) => {
			// To be on the safe side and ensure this doesn't lead to an unhandled (potentially crashing) error
			try {
				const logger = useLogger();
				logger.error(error, 'Unexpected error in error handler');
			} catch {
				// Ignore
			}

			// Delegate to default error handler to close the connection
			if (res.headersSent) return next(err);

			res.status(FALLBACK_ERROR.status);
			return res.json({ errors: [{ message: FALLBACK_ERROR.message, extensions: { code: FALLBACK_ERROR.code } }] });
		});
}
Repository: directus/directus
File: api/src/services/versions.ts (TypeScript)

Code before:
			origin: this.accountability?.origin ?? null,
			item,
		});

		const revisionDelta = await payloadService.prepareDelta(data);

		await revisionsService.createOne({
			activity,
			version: key,
			collection,
			item,
			data: revisionDelta,
			delta: revisionDelta,
		});

		let existingDelta = version['delta'];

		if (!existingDelta) {
			const saves = await this.getVersionSavesById(key);

			existingDelta = assign({}, ...saves);
		}

		const finalVersionDelta = assign({}, existingDelta, revisionDelta ? JSON.parse(revisionDelta) : null);

		await this.updateOne(key, { delta: finalVersionDelta });

		const { cache } = getCache();

		if (shouldClearCache(cache, undefined, collection)) {
			cache.clear();
		}

		return data;
	}

	async promote(version: PrimaryKey, mainHash: string, fields?: string[]) {
		const { id, collection, item, delta } = (await this.readOne(version)) as ContentVersion;

		// will throw an error if the accountability does not have permission to update the item
		await this.authorizationService.checkAccess('update', collection, item);

		const { outdated } = await this.verifyHash(collection, item, mainHash);

		if (outdated) {
			throw new UnprocessableContentError({
				reason: `Main item has changed since this version was last updated`,
			});
		}

		let versionResult;

Review comment (security):
Saving a new version now requires update permission on `directus_versions` due to this call - we may want to disregard permissions here (need to check whether that is safe), make it a breaking change requiring update permissions, or find another solution

Code after:
			user: this.accountability?.user ?? null,
			collection,
			ip: this.accountability?.ip ?? null,
			user_agent: this.accountability?.userAgent ?? null,
			origin: this.accountability?.origin ?? null,
			item,
		});

		const revisionDelta = await payloadService.prepareDelta(data);

		await revisionsService.createOne({
			activity,
			version: key,
			collection,
			item,
			data: revisionDelta,
			delta: revisionDelta,
		});

		let existingDelta = version['delta'];

		if (!existingDelta) {
			const saves = await this.getLegacyVersionSaves(key);

			existingDelta = assign({}, ...saves);
		}

		const finalVersionDelta = assign({}, existingDelta, revisionDelta ? JSON.parse(revisionDelta) : null);

		const sudoService = new ItemsService(this.collection, {
			knex: this.knex,
			schema: this.schema,
		});

		await sudoService.updateOne(key, { delta: finalVersionDelta });

		const { cache } = getCache();

		if (shouldClearCache(cache, undefined, collection)) {
			cache.clear();
		}

		return finalVersionDelta;
	}

	async promote(version: PrimaryKey, mainHash: string, fields?: string[]) {
		const { id, collection, item, delta } = (await this.readOne(version)) as ContentVersion;

		// will throw an error if the accountability does not have permission to update the item
		if (this.accountability) {
			await validateAccess(