ronantakizawa/codereview-bench huggingface dataset:
Repository: diesel-rs/diesel
File: diesel/src/collation/mod.rs (Rust)

Code before:
//! This module contains types that represent SQL collations.
//!
//! These types are used as arguments for
//! [`TextExpressionMethods::collate`](crate::expression_methods::TextExpressionMethods::collate).

use crate::backend::Backend;
use crate::query_builder::{AstPass, QueryFragment, QueryId};
use crate::result::QueryResult;

/// Trait to identify a valid collation.
pub trait Collation: QueryId + Copy + Send + Sync + 'static {}

/// A custom collation.
///
/// This type wraps a string that represents a collation name.
/// It can be used to use a collation that is not supported by Diesel.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct Custom(pub &'static str);

impl Collation for Custom {}

impl<DB: Backend> QueryFragment<DB> for Custom {
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, DB>) -> QueryResult<()> {
        out.push_sql(self.0);
        Ok(())
    }
}

impl QueryId for Custom {
    type QueryId = ();
    const HAS_STATIC_QUERY_ID: bool = false;
}

/// The `BINARY` collation.
///
/// This collation is binary, case-sensitive, and locale-free.

Review comment (suggestion):
I would rather not have the `Copy` bound here. That might be a future compatibility hazard so I rather would not guarantee it

Code after:
//! This module contains types that represent SQL collations.
//!
//! These types are used as arguments for
//! [`TextExpressionMethods::collate`](crate::expression_methods::TextExpressionMethods::collate).

use crate::backend::Backend;
use crate::query_builder::{AstPass, QueryFragment, QueryId};
use crate::result::QueryResult;

/// Trait to identify a valid collation.
pub trait Collation: QueryId + Clone + Send + Sync + 'static {}

/// A custom collation.
///
/// This type wraps a string that represents a collation name.
/// It can be used to use a collation that is not supported by Diesel.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct Custom(pub &'static str);

impl Collation for Custom {}

impl<DB: Backend> QueryFragment<DB> for Custom {
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, DB>) -> QueryResult<()> {
        out.push_sql(self.0);
        Ok(())
    }
}

impl QueryId for Custom {
    type QueryId = ();
    const HAS_STATIC_QUERY_ID: bool = false;
}

/// The `BINARY` collation.
///
/// This collation is binary, case-sensitive, and locale-free.
Repository: diesel-rs/diesel
File: diesel/src/collation/mod.rs (Rust)

Code before:
#[cfg(feature = "sqlite")]
impl Collation for Binary {}

#[cfg(feature = "sqlite")]
impl QueryFragment<crate::sqlite::Sqlite> for Binary {
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, crate::sqlite::Sqlite>) -> QueryResult<()> {
        out.push_sql("BINARY");
        Ok(())
    }
}

#[cfg(feature = "sqlite")]
impl QueryId for Binary {
    type QueryId = Self;
    const HAS_STATIC_QUERY_ID: bool = true;
}

/// The `C` collation.
///
/// This collation is byte-wise, with only ASCII A-Z treated as letters.
/// It is supported by PostgreSQL.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default)]
#[cfg(feature = "postgres")]
pub struct C;

#[cfg(feature = "postgres")]
impl Collation for C {}

#[cfg(feature = "postgres")]
impl QueryFragment<crate::pg::Pg> for C {
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, crate::pg::Pg>) -> QueryResult<()> {
        out.push_sql("\"C\"");
        Ok(())
    }
}

#[cfg(feature = "postgres")]
impl QueryId for C {
    type QueryId = Self;
    const HAS_STATIC_QUERY_ID: bool = true;
}

/// The `NOCASE` collation.
///
/// This collation is ASCII-only case-insensitive.
/// It is supported by SQLite.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default)]
#[cfg(feature = "sqlite")]
pub struct NoCase;

#[cfg(feature = "sqlite")]

Review comment (suggestion):
```suggestion
#[cfg(feature = "postgres_backend")]
```

otherwise it's not useable with diesel-async

(The same for the other locations and for "mysql" -> "mysql_backend")

Code after:
#[cfg(feature = "sqlite")]
impl Collation for Binary {}

#[cfg(feature = "sqlite")]
impl QueryFragment<crate::sqlite::Sqlite> for Binary {
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, crate::sqlite::Sqlite>) -> QueryResult<()> {
        out.push_sql("BINARY");
        Ok(())
    }
}

/// The `C` collation.
///
/// This collation is byte-wise, with only ASCII A-Z treated as letters.
/// It is supported by PostgreSQL.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default, QueryId)]
#[cfg(feature = "postgres_backend")]
pub struct C;

#[cfg(feature = "postgres_backend")]
impl Collation for C {}

#[cfg(feature = "postgres_backend")]
impl QueryFragment<crate::pg::Pg> for C {
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, crate::pg::Pg>) -> QueryResult<()> {
        out.push_sql("\"C\"");
        Ok(())
    }
}

/// The `NOCASE` collation.
///
/// This collation is ASCII-only case-insensitive.
/// It is supported by SQLite.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default, QueryId)]
#[cfg(feature = "sqlite")]
pub struct NoCase;

#[cfg(feature = "sqlite")]
impl Collation for NoCase {}

#[cfg(feature = "sqlite")]
impl QueryFragment<crate::sqlite::Sqlite> for NoCase {
    fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, crate::sqlite::Sqlite>) -> QueryResult<()> {
        out.push_sql("NOCASE");
        Ok(())
    }
}

/// The `POSIX` collation.
///
Repository: diesel-rs/diesel
File: diesel_compile_tests/tests/fail/derive/bad_serialize_as.rs (Rust)

Code before:
}

#[derive(Insertable)]
#[diesel(table_name = users)]
struct User2 {
    id: i32,
    #[diesel(serialize_as(Foo))]
    //~^ ERROR:  expected `=`
    name: String,
}

#[derive(Insertable)]
#[diesel(table_name = users)]
struct User3 {
    id: i32,
    #[diesel(serialize_as = "foo")]
    //~^ ERROR: expected identifier
    name: String,
}

#[derive(Insertable)]
#[diesel(table_name = users)]
struct User4 {
    id: i32,
    #[diesel(serialize_as = 1omg)]
    //~^ ERROR: expected identifier
    name: String,
}

fn main() {}


Review comment (suggestion):
If we use `parse_eq` instead of `parse_eq_type`, this error message would be

```
   |
42 |     #[diesel(serialize_as = 1omg)]
   |                             ^^^^ Error: expected one of: `for`, parentheses, `fn`, `unsafe`, `extern`, identifier, `::`, `<`, `dyn`, square brackets, `*`, `&`, `!`, `impl`, `_`, lifetime
   |
```

Which I believe would be confusing

Code after:
}

#[derive(Insertable)]
#[diesel(table_name = users)]
struct User2 {
    id: i32,
    #[diesel(serialize_as(Foo))]
    //~^ ERROR:  expected `=`
    name: String,
}

#[derive(Insertable)]
#[diesel(table_name = users)]
struct User3 {
    id: i32,
    #[diesel(serialize_as = "foo")]
    //~^ ERROR: expected type
    name: String,
}

#[derive(Insertable)]
#[diesel(table_name = users)]
struct User4 {
    id: i32,
    #[diesel(serialize_as = 1omg)]
    //~^ ERROR: expected type
    name: String,
}

#[derive(Insertable)]
#[diesel(table_name = users)]
struct User5 {
    id: i32,
    #[diesel(serialize_as = *const u8)]
    //~^ ERROR: `serialize_as` does not support pointer types
    name: String,
}

#[derive(Insertable)]
#[diesel(table_name = users)]
struct User6 {
    id: i32,
    #[diesel(serialize_as = fn() -> i32)]
    //~^ ERROR: `serialize_as` does not support function pointer types
    name: String,
}

#[derive(Insertable)]
#[diesel(table_name = users)]
struct User7 {
    id: i32,
Repository: directus/directus
File: packages/types/src/data/abstract-query.ts (TypeScript)

Code before:
 * @typeParam value - the offset value
 */
interface AbstractQueryNodeOffset {
	type: 'offset';
	value: number;
}

/**
 * Specifies the order of the results
 * @typeParam type - set to 'sort'
 * @typeParam direction - 'ascending' or 'descending'
 * @typeParam target - the node on which the sorting should be applied
 */
interface AbstractQueryNodeSort {
	type: 'sort';
	direction: 'ascending' | 'descending';
	target:
		| AbstractQueryFieldNodePrimitive
		| AbstractQueryFieldNodeFn
		| AbstractQueryFieldNodeRelatedManyToOne
		| AbstractQueryFieldNodeRelatedAnyToOne;
}

/**
 * Used to create logical operations
 * @typeParam type - set to 'sort'
 * @typeParam operator - the logical operator to use
 * @typeParam children - left and right side of the operation.
 * @example
 * ```
 * {
 * 	type: 'logical',
 * 	operator: 'and',
 * 	children: [
 * 		{
 * 			type: 'condition',
 * 			operation: 'eq',
 * 			target: { type: 'field', key: 'a' }
 * 			value: 5
 * 		},
 * 		{
 * 			type: 'condition',
 * 			operation: 'lt',
 * 			target: { type: 'field', key: 'b' }
 * 			value: 5
 * 		}
 *  ]
 * }
 * ```
 */
export interface AbstractQueryNodeLogical {

Review comment (suggestion):
```suggestion
 * @typeParam type - set to 'logical'
```

Code after:
		fields: string[];
	};
}

export interface AbstractQueryFieldNodeRelatedManyToOne extends AbstractQueryNode, AbstractQueryFieldNodeRelatedBase {
	type: 'm2o';

	join: AbstractQueryFieldNodeRelatedJoinMany;
}

export interface AbstractQueryFieldNodeRelatedOneToMany extends AbstractQueryNode, AbstractQueryFieldNodeRelatedBase {
	type: 'o2m';

	join: AbstractQueryFieldNodeRelatedJoinMany;
}

export interface AbstractQueryFieldNodeRelatedAnyToOne extends AbstractQueryNode, AbstractQueryFieldNodeRelatedBase {
	type: 'a2o';

	join: AbstractQueryFieldNodeRelatedJoinAny;
}

export interface AbstractQueryFieldNodeRelatedOneToAny extends AbstractQueryNode, AbstractQueryFieldNodeRelatedBase {
	type: 'o2a';

	join: AbstractQueryFieldNodeRelatedJoinAny;
}

// ======================================== Modifiers =========================================================

/**
 * Optional attributes to customize the query results
 */
export interface AbstractQueryModifiers {
	limit?: AbstractQueryNodeLimit;
	offset?: AbstractQueryNodeOffset;
	sort?: AbstractQueryNodeSort;
	filter?: AbstractQueryNodeLogical | AbstractQueryNodeCondition;
}

interface AbstractQueryModifierNode {
	type: 'limit' | 'offset' | 'sort' | 'logical' | 'condition';
}

/**
 * Specifies the maximum amount of returning results
 */
interface AbstractQueryNodeLimit extends AbstractQueryModifierNode {
	type: 'limit';
	value: number;
}
Repository: directus/directus
File: packages/types/src/data/abstract-query.ts (TypeScript)

Code before:
 * 		{
 * 			type: 'condition',
 * 			operation: 'eq',
 * 			target: { type: 'field', key: 'a' }
 * 			value: 5
 * 		},
 * 		{
 * 			type: 'condition',
 * 			operation: 'lt',
 * 			target: { type: 'field', key: 'b' }
 * 			value: 5
 * 		}
 *  ]
 * }
 * ```
 */
export interface AbstractQueryNodeLogical {
	type: 'logical';
	operator: 'and' | 'or' | 'not';
	childNodes: (AbstractQueryNodeLogical | AbstractQueryNodeCondition)[];
}

/**
 * Used to create conditional operations
 * @typeParam type - set to 'condition'
 * @typeParam target - the node on which the sorting should be applied
 * @typeParam operation - the operation to perform on the target
 * @typeParam value - the conditional value. Might be also a function or sub query in the future
 * @example
 * ```
 * {
 * 		type: 'condition',
 * 		operation: 'lt',
 *		target: { type: 'field', key: 'b' }
 * 		value: 5
 * }
 * ```
 */
export interface AbstractQueryNodeCondition {
	type: 'condition';
	targetNode:
		| AbstractQueryFieldNodePrimitive
		| AbstractQueryFieldNodeFn
		| AbstractQueryFieldNodeRelatedManyToOne
		| AbstractQueryFieldNodeRelatedAnyToOne;
	operation:
		| 'eq'
		| 'lt'
		| 'lte'
		| 'gt'
		| 'gte'

Review comment (suggestion):
```suggestion
 * @typeParam targetNode - the node on which the condition should be applied
```

(this also assumes we decide to use `targetNode`!)

Code after:
	limit?: AbstractQueryNodeLimit;
	offset?: AbstractQueryNodeOffset;
	sort?: AbstractQueryNodeSort;
	filter?: AbstractQueryNodeLogical | AbstractQueryNodeCondition;
}

interface AbstractQueryModifierNode {
	type: 'limit' | 'offset' | 'sort' | 'logical' | 'condition';
}

/**
 * Specifies the maximum amount of returning results
 */
interface AbstractQueryNodeLimit extends AbstractQueryModifierNode {
	type: 'limit';
	value: number;
}

/**
 * Specifies the number of items to skip before returning results
 */
interface AbstractQueryNodeOffset extends AbstractQueryModifierNode {
	type: 'offset';
	value: number;
}

/**
 * Specifies the order of the results
 */
interface AbstractQueryNodeSort extends AbstractQueryModifierNode {
	type: 'sort';

	/** the desired order */
	direction: 'ascending' | 'descending';

	/** the node on which the sorting should be applied */
	target:
		| AbstractQueryFieldNodePrimitive
		| AbstractQueryFieldNodeFn
		| AbstractQueryFieldNodeRelatedManyToOne
		| AbstractQueryFieldNodeRelatedAnyToOne;
}

/**
 * Used to create logical operations.
 * @example
 * Let's say you want to only return rows where two conditions are true.
 * First condition that some field value needs to be qual to a provided value and another condition that one field is less than another provided value.
 * This would look like this:
 * ```
 * {
Repository: directus/directus
File: packages/types/src/data/abstract-query.ts (TypeScript)

Code before:

export interface AbstractQueryFieldNodeRelatedOneToMany extends AbstractQueryNode, AbstractQueryFieldNodeRelatedBase {
	type: 'o2m';

	join: AbstractQueryFieldNodeRelatedJoinMany;
}

export interface AbstractQueryFieldNodeRelatedAnyToOne extends AbstractQueryNode, AbstractQueryFieldNodeRelatedBase {
	type: 'a2o';

	join: AbstractQueryFieldNodeRelatedJoinAny;
}

export interface AbstractQueryFieldNodeRelatedOneToAny extends AbstractQueryNode, AbstractQueryFieldNodeRelatedBase {
	type: 'o2a';

	join: AbstractQueryFieldNodeRelatedJoinAny;
}

// =================================================================================================

/**
 * Optional attributes to customize the query results
 * @typeParam sort - Specifies the order of the results
 * @typeParam limit - Specifies the maximum amount of returning results
 */
export interface AbstractQueryModifiers {
	limit?: AbstractQueryNodeLimit;
	offset?: AbstractQueryNodeOffset;
	sort?: AbstractQueryNodeSort;
	filter?: AbstractQueryNodeLogical | AbstractQueryNodeCondition;
}

/**
 * Specifies the maximum amount of returning results
 * @typeParam type - set to 'limit'
 * @typeParam value - the limit value
 */
interface AbstractQueryNodeLimit {
	type: 'limit';
	value: number;
}

/**
 * Specifies the number of items to skip before returning results
 * @typeParam type - set to 'offset'
 * @typeParam value - the offset value
 */
interface AbstractQueryNodeOffset {
	type: 'offset';
	value: number;

Review comment (suggestion):
based on the interface, should we include `offset` and `filter` in this tsdoc as well? (and possibly order it based on their order in the interface)

Code after:

/**
 * With those Used to build a relational query for m2o and o2m relations.
 */
export type AbstractQueryFieldNodeRelated =
	| AbstractQueryFieldNodeRelatedManyToOne
	| AbstractQueryFieldNodeRelatedOneToMany
	| AbstractQueryFieldNodeRelatedAnyToOne
	| AbstractQueryFieldNodeRelatedOneToAny;

/**
 * Used to build a relational query for m2o and o2m relations.
 * @example
 * ```
 * const functionNode = {
 * 	current: {
 * 		fields: ['id']
 *  },
 * 	external: {
 * 		datastore: 'mongodb',
 * 		collection: 'some-collection',
 * }
 * ```
 */
interface AbstractQueryFieldNodeRelatedJoinMany {
	/** the field of the current collection which has the relational value to an external collection or item */
	current: {
		fields: string[];
	};

	/** the external collection or item which should be pulled/joined/merged into the current collection */
	external: {
		datastore?: string;
		collection: string;
		fields: string[];
	};
}

interface AbstractQueryFieldNodeRelatedJoinAny {
	current: {
		collectionField: string;
		fields: string[];
	};

	external: {
		datastore?: string;
		fields: string[];
	};
}

export interface AbstractQueryFieldNodeRelatedManyToOne extends AbstractQueryNode, AbstractQueryFieldNodeRelatedBase {
Repository: directus/directus
File: packages/types/src/data/abstract-query.ts (TypeScript)

Code before:
 * ```
 * {
 * 		type: 'condition',
 * 		operation: 'lt',
 *		target: { type: 'field', key: 'b' }
 * 		value: 5
 * }
 * ```
 */
export interface AbstractQueryNodeCondition {
	type: 'condition';
	targetNode:
		| AbstractQueryFieldNodePrimitive
		| AbstractQueryFieldNodeFn
		| AbstractQueryFieldNodeRelatedManyToOne
		| AbstractQueryFieldNodeRelatedAnyToOne;
	operation:
		| 'eq'
		| 'lt'
		| 'lte'
		| 'gt'
		| 'gte'
		| 'in'
		| 'contains'
		| 'starts_with'
		| 'end_with'
		| 'intersects'
		| 'intersects_bounding_box';
	value: string | number | boolean;
}

/**
 * Questions:
 * - Should we support "Distinct", if so where does it live (field level vs collection level)
 * - Rethink every / some
 * - Should logical "not" be a node with a single child?
 */


Review comment (suggestion):
```suggestion
		| 'ends_with'
```

(assumed based on the inclusion of trailing "s" in `starts_with`)

Code after:
interface AbstractQueryNodeSort extends AbstractQueryModifierNode {
	type: 'sort';

	/** the desired order */
	direction: 'ascending' | 'descending';

	/** the node on which the sorting should be applied */
	target:
		| AbstractQueryFieldNodePrimitive
		| AbstractQueryFieldNodeFn
		| AbstractQueryFieldNodeRelatedManyToOne
		| AbstractQueryFieldNodeRelatedAnyToOne;
}

/**
 * Used to create logical operations.
 * @example
 * Let's say you want to only return rows where two conditions are true.
 * First condition that some field value needs to be qual to a provided value and another condition that one field is less than another provided value.
 * This would look like this:
 * ```
 * {
 * 	type: 'logical',
 * 	operator: 'and',
 * 	childNodes: [
 * 		{
 * 			type: 'condition',
 * 			operation: 'eq',
 * 			targetNode: { type: 'field', field: 'a' }
 * 			value: 5
 * 		},
 * 		{
 * 			type: 'condition',
 * 			operation: 'lt',
 * 			targetNode: { type: 'field', field: 'b' }
 * 			value: 28
 * 		}
 *  ]
 * }
 * ```
 * It is also possible to nest conditions with the logical operator.
 * ```
 * {
 * 	type: 'logical',
 * 	operator: 'and',
 * 	childNodes: [
 * 		{
 * 			type: 'condition',
 * 			operation: 'eq',
 * 			targetNode: { type: 'field', field: 'a' }
 * 			value: 5
Repository: directus/directus
File: api/src/services/relations.ts (TypeScript)

Code before:
		this.relationsItemService = new ItemsService('directus_relations', {
			knex: this.knex,
			schema: this.schema,
			// We don't set accountability here. If you have read access to certain fields, you are
			// allowed to extract the relations regardless of permissions to directus_relations. This
			// happens in `filterForbidden` down below
		});

		this.systemCache = getCache().systemCache;
		this.helpers = getHelpers(this.knex);
	}

	async foreignKeys(collection?: string) {
		const schemaCacheIsEnabled = Boolean(env['CACHE_SCHEMA']);

		let foreignKeys: ForeignKey[] | null = null;

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

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

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

		if (collection) {
			return foreignKeys.filter((row) => row.table === collection);
		}

		return foreignKeys;
	}

	async readAll(collection?: string, opts?: QueryOptions): Promise<Relation[]> {
		if (this.accountability && this.accountability.admin !== true && this.hasReadAccess === false) {
			throw new ForbiddenError();
		}

		const metaReadQuery: Query = {
			limit: -1,
		};

		if (collection) {
			metaReadQuery.filter = {
				many_collection: {
					_eq: collection,
				},
			};

Review comment (performance):
Let's cache using the `localSchemaCache` instead of `systemCache`? Same as per https://github.com/directus/directus/pull/19480/files#r1689943849 🤔 

Code after:

		this.relationsItemService = new ItemsService('directus_relations', {
			knex: this.knex,
			schema: this.schema,
			// We don't set accountability here. If you have read access to certain fields, you are
			// allowed to extract the relations regardless of permissions to directus_relations. This
			// happens in `filterForbidden` down below
		});

		const cache = getCache();
		this.systemCache = cache.systemCache;
		this.schemaCache = cache.localSchemaCache;
		this.helpers = getHelpers(this.knex);
	}

	async foreignKeys(collection?: string) {
		const schemaCacheIsEnabled = Boolean(env['CACHE_SCHEMA']);

		let foreignKeys: ForeignKey[] | null = null;

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

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

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

		if (collection) {
			return foreignKeys.filter((row) => row.table === collection);
		}

		return foreignKeys;
	}

	async readAll(collection?: string, opts?: QueryOptions): Promise<Relation[]> {
		if (this.accountability && this.accountability.admin !== true && this.hasReadAccess === false) {
			throw new ForbiddenError();
		}

		const metaReadQuery: Query = {
			limit: -1,
		};

		if (collection) {
			metaReadQuery.filter = {
				many_collection: {