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

Code before:
		const result = await revisionsService.readByQuery({
			filter: { version: { _eq: id } },
		});

		return result.map((revision) => revision['delta']);
	}

	async getVersionSaves(key: string, collection: string, item: string | undefined): Promise<Partial<Item>[] | null> {
		const filter: Filter = {
			key: { _eq: key },
			collection: { _eq: collection },
		};

		if (item) {
			filter['item'] = { _eq: item };
		}

		const versions = await this.readByQuery({ filter });

		if (!versions?.[0]) return null;

		if (versions[0]['delta']) {
			return [versions[0]['delta']];
		}

		const saves = await this.getVersionSavesById(versions[0]['id']);

		return saves;
	}

	override async createOne(data: Partial<Item>, opts?: MutationOptions): Promise<PrimaryKey> {
		await this.validateCreateData(data);

		const mainItem = await this.getMainItem(data['collection'], data['item']);

		data['hash'] = objectHash(mainItem);

		return super.createOne(data, opts);
	}

	override async createMany(data: Partial<Item>[], opts?: MutationOptions): Promise<PrimaryKey[]> {
		if (!Array.isArray(data)) {
			throw new InvalidPayloadError({ reason: 'Input should be an array of items' });
		}

		const keyCombos = new Set();

		for (const item of data) {
			await this.validateCreateData(item);

			const keyCombo = `${item['key']}-${item['collection']}-${item['item']}`;

Review comment (suggestion):
It feels strange, that this method now (in most/all cases?) returns only one item (the `delta`), especially while it is called `getVersionSaves`.
Do we even need that method and `getVersionSavesById` anymore? Can't we just always use the `delta`? I think the associated revision items now only serve the purpose of history, but not as source of truth anymore.

Code after:

		return { outdated: hash !== mainHash, mainHash };
	}

	async getLegacyVersionSaves(id: PrimaryKey): Promise<Partial<Item>[]> {
		const revisionsService = new RevisionsService({
			knex: this.knex,
			schema: this.schema,
		});

		const result = await revisionsService.readByQuery({
			filter: { version: { _eq: id } },
		});

		return result.map((revision) => revision['delta']);
	}

	// TODO: Remove legacy need to return a version array in subsequent release
	async getVersionSaves(key: string, collection: string, item: string | undefined): Promise<Partial<Item>[] | null> {
		const filter: Filter = {
			key: { _eq: key },
			collection: { _eq: collection },
		};

		if (item) {
			filter['item'] = { _eq: item };
		}

		const versions = await this.readByQuery({ filter });

		if (!versions?.[0]) return null;

		if (versions[0]['delta']) {
			return [versions[0]['delta']];
		}

		const saves = await this.getLegacyVersionSaves(versions[0]['id']);

		return saves;
	}

	override async createOne(data: Partial<Item>, opts?: MutationOptions): Promise<PrimaryKey> {
		await this.validateCreateData(data);

		const mainItem = await this.getMainItem(data['collection'], data['item']);

		data['hash'] = objectHash(mainItem);

		return super.createOne(data, opts);
	}
Repository: directus/directus
File: api/src/controllers/versions.ts (TypeScript)

Code before:
		}

		const main = await service.getMainItem(version['collection'], version['item']);

		res.locals['payload'] = { data: { outdated, mainHash, current, main } };

		return next();
	}),
	respond,
);

router.post(
	'/:pk/save',
	asyncHandler(async (req, res, next) => {
		const service = new VersionsService({
			accountability: req.accountability,
			schema: req.schema,
		});

		const version = await service.readOne(req.params['pk']!);

		const mainItem = await service.getMainItem(version['collection'], version['item']);

		await service.save(req.params['pk']!, req.body);

		const updatedVersion = await service.readOne(req.params['pk']!);

		const result = assign(mainItem, updatedVersion['delta']);

		res.locals['payload'] = { data: result || null };

		return next();
	}),
	respond,
);

router.post(
	'/:pk/promote',
	asyncHandler(async (req, res, next) => {
		if (typeof req.body.mainHash !== 'string') {
			throw new InvalidPayloadError({ reason: `"mainHash" field is required` });
		}

		const service = new VersionsService({
			accountability: req.accountability,
			schema: req.schema,
		});

		const updatedItemKey = await service.promote(req.params['pk']!, req.body.mainHash, req.body?.['fields']);

		res.locals['payload'] = { data: updatedItemKey || null };

Review comment (suggestion):
I think we could update `save` to return the `delta` and use that instead of an additional `readOne` call.

Code after:
		}

		const main = await service.getMainItem(version['collection'], version['item']);

		res.locals['payload'] = { data: { outdated, mainHash, current, main } };

		return next();
	}),
	respond,
);

router.post(
	'/:pk/save',
	asyncHandler(async (req, res, next) => {
		const service = new VersionsService({
			accountability: req.accountability,
			schema: req.schema,
		});

		const version = await service.readOne(req.params['pk']!);

		const mainItem = await service.getMainItem(version['collection'], version['item']);

		const updatedVersion = await service.save(req.params['pk']!, req.body);

		const result = assign(mainItem, updatedVersion);

		res.locals['payload'] = { data: result || null };

		return next();
	}),
	respond,
);

router.post(
	'/:pk/promote',
	asyncHandler(async (req, res, next) => {
		if (typeof req.body.mainHash !== 'string') {
			throw new InvalidPayloadError({ reason: `"mainHash" field is required` });
		}

		const service = new VersionsService({
			accountability: req.accountability,
			schema: req.schema,
		});

		const updatedItemKey = await service.promote(req.params['pk']!, req.body.mainHash, req.body?.['fields']);

		res.locals['payload'] = { data: updatedItemKey || null };

		return next();
Repository: directus/directus
File: api/src/services/roles.ts (TypeScript)

Code before:
		for (const ip of value) {
			if (typeof ip !== 'string' || ip.includes('*')) return false;

			try {
				const match = getMatch(ip);
				if (match.type == 'IPMask') return false;
			} catch {
				return false;
			}
		}

		return true;
	}

	private assertValidIpAccess(partialItem: Partial<Item>): void {
		if ('ip_access' in partialItem && !this.isIpAccessValid(partialItem['ip_access'])) {
			throw new InvalidPayloadError({
				reason: 'IP Access contains an incorrect value. Valid values are: IP addresses, IP ranges and CIDR blocks',
			});
		}
	}

	private getRoleAccessType(data: Partial<Item>) {
		if ('admin_access' in data && data['admin_access'] === true) {
			return 'admin';
		} else if ('app_access' in data && (data['app_access'] === true || 'app_access' in data === false)) {
			return 'app';
		} else {
			return 'api';
		}
	}

	override async createOne(data: Partial<Item>, opts?: MutationOptions): Promise<PrimaryKey> {
		this.assertValidIpAccess(data);

		const increasedCounts: AccessTypeCount = {
			admin: 0,
			app: 0,
			api: 0,
		};

		const existingIds: PrimaryKey[] = [];

		if ('users' in data) {
			const type = this.getRoleAccessType(data);
			increasedCounts[type] += data['users'].length;

			for (const user of data['users']) {
				if (typeof user === 'string') {
					existingIds.push(user);
				} else if (typeof user === 'object' && 'id' in user) {

Review comment (suggestion):
Seems to me if the first condition `'app_access' in data` is `true` then the `'app_access' in data === false` inside the second condition will always be `false` hence never influencing the `||` and making it redundant

Code after:

	private isIpAccessValid(value?: any[] | null): boolean {
		if (value === undefined) return false;
		if (value === null) return true;
		if (Array.isArray(value) && value.length === 0) return true;

		for (const ip of value) {
			if (typeof ip !== 'string' || ip.includes('*')) return false;

			try {
				const match = getMatch(ip);
				if (match.type == 'IPMask') return false;
			} catch {
				return false;
			}
		}

		return true;
	}

	private assertValidIpAccess(partialItem: Partial<Item>): void {
		if ('ip_access' in partialItem && !this.isIpAccessValid(partialItem['ip_access'])) {
			throw new InvalidPayloadError({
				reason: 'IP Access contains an incorrect value. Valid values are: IP addresses, IP ranges and CIDR blocks',
			});
		}
	}

	private getRoleAccessType(data: Partial<Item>) {
		if ('admin_access' in data && data['admin_access'] === true) {
			return 'admin';
		} else if (('app_access' in data && data['app_access'] === true) || 'app_access' in data === false) {
			return 'app';
		} else {
			return 'api';
		}
	}

	override async createOne(data: Partial<Item>, opts?: MutationOptions): Promise<PrimaryKey> {
		this.assertValidIpAccess(data);

		const increasedCounts: AccessTypeCount = {
			admin: 0,
			app: 0,
			api: 0,
		};

		const existingIds: PrimaryKey[] = [];

		if ('users' in data) {
			const type = this.getRoleAccessType(data);
Repository: directus/directus
File: sdk/src/utils/extract-data.ts (TypeScript)

Code before:
import { isFetchResponse } from './is-response.js';

/**
 * Tries to extract a fetch response
 */
export async function extractData(response: unknown) {
	if (typeof response !== 'object' || !response) return;

	if (isFetchResponse(response)) {
		const type = response.headers.get('Content-Type')?.toLowerCase();

		if (type?.startsWith('application/json') || type?.startsWith('application/health+json')) {
			const result = await response.json();
			if (!response.ok || 'errors' in result) throw result;
			if ('data' in result) return result.data;
			return result;
		}

		if (type?.startsWith('text/html') || type?.startsWith('text/plain')) {
			const result = await response.text();
			if (!response.ok) throw result;
			return result;
		}

		// fallback for anything else
		return response;
	}

	// exception for alternatives like ofetch that don't return the Response object
	return 'data' in response ? response.data : response;
}


Review comment (suggestion):
The tricky bit here is that a GraphQL response can have both `errors` and `data` at the same time. This solution would mean we would throw a partial-successful response as well.

Code after:
import { isFetchResponse } from './is-response.js';

/**
 * Tries to extract a fetch response
 */
export async function extractData(response: unknown) {
	if (typeof response !== 'object' || !response) return;

	if (isFetchResponse(response)) {
		const type = response.headers.get('Content-Type')?.toLowerCase();

		if (type?.startsWith('application/json') || type?.startsWith('application/health+json')) {
			const result = await response.json();

			if (!response.ok || 'errors' in result) throw result;

			if ('data' in result) return result.data;

			return result;
		}

		if (type?.startsWith('text/html') || type?.startsWith('text/plain')) {
			const result = await response.text();
			if (!response.ok) throw result;
			return result;
		}

		// fallback for anything else
		return response;
	}

	// exception for alternatives like ofetch that don't return the Response object

	if ('errors' in response) throw response;

	if ('data' in response) return response.data;

	return response;
}
Repository: directus/directus
File: sdk/src/utils/request.ts (TypeScript)

Code before:
import type { FetchInterface } from '../index.js';
import { extractData } from './extract-data.js';

/**
 * Request helper providing default settings
 *
 * @param url The request URL
 * @param options The request options
 *
 * @returns The API result if successful
 */
export const request = async <Output = any>(
	url: string,
	options: RequestInit,
	fetcher: FetchInterface = globalThis.fetch,
): Promise<Output> => {
	options.headers =
		typeof options.headers === 'object' && !Array.isArray(options.headers)
			? (options.headers as Record<string, string>)
			: {};

	return fetcher(url, options).then((response) => {
		return extractData(response).catch((reason) => {
			const errors = typeof reason === 'object' && 'errors' in reason ? reason.errors : reason;
			const data = typeof reason === 'object' && 'data' in reason ? reason.data : null;
			return Promise.reject({ data, errors, response });
		});
	});
};


Review comment (bug):
The above has already been there, but just noticed that we should adjust these two lines to at least ensure the value is not `null` since the `in` operator would fail in that case, so e.g.
```js
			const errors = reason && typeof reason === 'object' && 'errors' in reason ? reason.errors : reason;
			const data = reason && typeof reason === 'object' && 'data' in reason ? reason.data : null;
```

Code after:
import type { FetchInterface } from '../index.js';
import { extractData } from './extract-data.js';

/**
 * Request helper providing default settings
 *
 * @param url The request URL
 * @param options The request options
 *
 * @returns The API result if successful
 */
export const request = async <Output = any>(
	url: string,
	options: RequestInit,
	fetcher: FetchInterface = globalThis.fetch,
): Promise<Output> => {
	options.headers =
		typeof options.headers === 'object' && !Array.isArray(options.headers)
			? (options.headers as Record<string, string>)
			: {};

	return fetcher(url, options).then((response) => {
		return extractData(response).catch((reason) => {
			const result: { response: unknown; errors: any; data?: any } = {
				errors: reason && typeof reason === 'object' && 'errors' in reason ? reason.errors : reason,
				response,
			};

			if (reason && typeof reason === 'object' && 'data' in reason) result.data = reason.data;

			return Promise.reject(result);
		});
	});
};
Repository: directus/directus
File: sdk/src/utils/request.ts (TypeScript)

Code before:
import type { FetchInterface } from '../index.js';
import { extractData } from './extract-data.js';

/**
 * Request helper providing default settings
 *
 * @param url The request URL
 * @param options The request options
 *
 * @returns The API result if successful
 */
export const request = async <Output = any>(
	url: string,
	options: RequestInit,
	fetcher: FetchInterface = globalThis.fetch,
): Promise<Output> => {
	options.headers =
		typeof options.headers === 'object' && !Array.isArray(options.headers)
			? (options.headers as Record<string, string>)
			: {};

	return fetcher(url, options).then((response) => {
		return extractData(response).catch((reason) => {
			const errors = reason && typeof reason === 'object' && 'errors' in reason ? reason.errors : reason;
			const data = reason && typeof reason === 'object' && 'data' in reason ? reason.data : null;
			return Promise.reject({ data, errors, response });
		});
	});
};


Review comment (suggestion):
I think we should only return the `data` prop if it is actually available in `reason`

```suggestion
			const result: { response: unknown; errors: any; data?: any } = {
				errors: reason && typeof reason === 'object' && 'errors' in reason ? reason.errors : reason,
				response,
			};

			if (reason && typeof reason === 'object' && 'data' in reason) result.data = reason.data;

			return Promise.reject(result);
```

Code after:
import type { FetchInterface } from '../index.js';
import { extractData } from './extract-data.js';

/**
 * Request helper providing default settings
 *
 * @param url The request URL
 * @param options The request options
 *
 * @returns The API result if successful
 */
export const request = async <Output = any>(
	url: string,
	options: RequestInit,
	fetcher: FetchInterface = globalThis.fetch,
): Promise<Output> => {
	options.headers =
		typeof options.headers === 'object' && !Array.isArray(options.headers)
			? (options.headers as Record<string, string>)
			: {};

	return fetcher(url, options).then((response) => {
		return extractData(response).catch((reason) => {
			const result: { response: unknown; errors: any; data?: any } = {
				errors: reason && typeof reason === 'object' && 'errors' in reason ? reason.errors : reason,
				response,
			};

			if (reason && typeof reason === 'object' && 'data' in reason) result.data = reason.data;

			return Promise.reject(result);
		});
	});
};
Repository: directus/directus
File: api/src/services/roles.ts (TypeScript)

Code before:
						existingIds.push(user['id']);
					}
				}
			}

			await checkIncreasedUserLimits(this.knex, increasedCounts, existingIds);
		}

		return super.createOne(data, opts);
	}

	override async createMany(data: Partial<Item>[], opts?: MutationOptions): Promise<PrimaryKey[]> {
		const needsUserLimitCheck = shouldCheckUserLimits();

		const increasedCounts: AccessTypeCount = {
			admin: 0,
			app: 0,
			api: 0,
		};

		const existingIds: PrimaryKey[] = [];

		for (const partialItem of data) {
			this.assertValidIpAccess(partialItem);

			if ('users' in partialItem && needsUserLimitCheck) {
				const type = this.getRoleAccessType(partialItem);
				increasedCounts[type] += partialItem['users'].length;

				for (const user of partialItem['users']) {
					if (typeof user === 'string') {
						existingIds.push(user);
					} else if (typeof user === 'object' && 'id' in user) {
						existingIds.push(user['id']);
					}
				}
			}
		}

		if (needsUserLimitCheck) {
			await checkIncreasedUserLimits(this.knex, increasedCounts, existingIds);
		}

		return super.createMany(data, opts);
	}

	override async updateOne(key: PrimaryKey, data: Partial<Item>, opts?: MutationOptions): Promise<PrimaryKey> {
		this.assertValidIpAccess(data);

		try {
			if ('users' in data) {

Review comment (suggestion):
I think it would be nicer if the "shouldCheckUserLimits" condition comes first in each of those `if`s, e.g.
```suggestion
			if (needsUserLimitCheck && 'users' in partialItem) {
```

Code after:
						existingIds.push(user['id']);
					}
				}
			}

			await checkIncreasedUserLimits(this.knex, increasedCounts, existingIds);
		}

		return super.createOne(data, opts);
	}

	override async createMany(data: Partial<Item>[], opts?: MutationOptions): Promise<PrimaryKey[]> {
		const needsUserLimitCheck = shouldCheckUserLimits();

		const increasedCounts: AccessTypeCount = {
			admin: 0,
			app: 0,
			api: 0,
		};

		const existingIds: PrimaryKey[] = [];

		for (const partialItem of data) {
			this.assertValidIpAccess(partialItem);

			if (needsUserLimitCheck && 'users' in partialItem) {
				const type = this.getRoleAccessType(partialItem);
				increasedCounts[type] += partialItem['users'].length;

				for (const user of partialItem['users']) {
					if (typeof user === 'string') {
						existingIds.push(user);
					} else if (typeof user === 'object' && 'id' in user) {
						existingIds.push(user['id']);
					}
				}
			}
		}

		if (needsUserLimitCheck) {
			await checkIncreasedUserLimits(this.knex, increasedCounts, existingIds);
		}

		return super.createMany(data, opts);
	}

	override async updateOne(key: PrimaryKey, data: Partial<Item>, opts?: MutationOptions): Promise<PrimaryKey> {
		this.assertValidIpAccess(data);

		try {
			if ('users' in data) {
Repository: directus/directus
File: api/src/middleware/cors.ts (TypeScript)

Code before:
import { useEnv } from '@directus/env';
import cors from 'cors';
import type { RequestHandler } from 'express';

let corsMiddleware: RequestHandler = (_req, _res, next) => next();

const env = useEnv();

if (env['CORS_ENABLED'] === true) {
	corsMiddleware = cors({
		origin: (env['CORS_ORIGIN'] as string) ?? true,
		methods: (env['CORS_METHODS'] as string) ?? 'GET,POST,PATCH,DELETE',
		allowedHeaders: env['CORS_ALLOWED_HEADERS'] as string,
		exposedHeaders: env['CORS_EXPOSED_HEADERS'] as string,
		credentials: (env['CORS_CREDENTIALS'] as boolean) ?? undefined,
		maxAge: (env['CORS_MAX_AGE'] as number) ?? undefined,
	});
}

export default corsMiddleware;


Review comment (suggestion):
```suggestion
		origin: env['CORS_ORIGIN'] as boolean | string,
		methods: env['CORS_METHODS'] as string,
```

(`GET,POST,PATCH,DELETE` is already the default in `@directus/env`)

Code after:
import { useEnv } from '@directus/env';
import cors from 'cors';
import type { RequestHandler } from 'express';

let corsMiddleware: RequestHandler = (_req, _res, next) => next();

const env = useEnv();

if (env['CORS_ENABLED'] === true) {
	corsMiddleware = cors({
		origin: env['CORS_ORIGIN'] as boolean | string,
		methods: env['CORS_METHODS'] as string,
		allowedHeaders: env['CORS_ALLOWED_HEADERS'] as string,
		exposedHeaders: env['CORS_EXPOSED_HEADERS'] as string,
		credentials: env['CORS_CREDENTIALS'] as boolean,
		maxAge: env['CORS_MAX_AGE'] as number,
	});
}

export default corsMiddleware;
Repository: directus/directus
File: api/src/middleware/cors.ts (TypeScript)

Code before:
import { useEnv } from '@directus/env';
import cors from 'cors';
import type { RequestHandler } from 'express';

let corsMiddleware: RequestHandler = (_req, _res, next) => next();

const env = useEnv();

if (env['CORS_ENABLED'] === true) {
	corsMiddleware = cors({
		origin: (env['CORS_ORIGIN'] as string) ?? true,
		methods: (env['CORS_METHODS'] as string) ?? 'GET,POST,PATCH,DELETE',
		allowedHeaders: env['CORS_ALLOWED_HEADERS'] as string,
		exposedHeaders: env['CORS_EXPOSED_HEADERS'] as string,
		credentials: (env['CORS_CREDENTIALS'] as boolean) ?? undefined,
		maxAge: (env['CORS_MAX_AGE'] as number) ?? undefined,
	});
}

export default corsMiddleware;


Review comment (security):
```suggestion
		credentials: env['CORS_CREDENTIALS'] as boolean,
		maxAge: env['CORS_MAX_AGE'] as number,
```

Code after:
import { useEnv } from '@directus/env';
import cors from 'cors';
import type { RequestHandler } from 'express';

let corsMiddleware: RequestHandler = (_req, _res, next) => next();

const env = useEnv();

if (env['CORS_ENABLED'] === true) {
	corsMiddleware = cors({
		origin: env['CORS_ORIGIN'] as boolean | string,
		methods: env['CORS_METHODS'] as string,
		allowedHeaders: env['CORS_ALLOWED_HEADERS'] as string,
		exposedHeaders: env['CORS_EXPOSED_HEADERS'] as string,
		credentials: env['CORS_CREDENTIALS'] as boolean,
		maxAge: env['CORS_MAX_AGE'] as number,
	});
}

export default corsMiddleware;