ronantakizawa/codereview-bench huggingface dataset:
Repository: typeorm/typeorm
File: src/commands/MigrationGenerateCommand.ts (TypeScript)

Code before:
    protected static getJavascriptTemplate(
        name: string,
        timestamp: number,
        upSqls: string[],
        downSqls: string[],
        esm: boolean,
    ): string {
        const migrationName = `${camelCase(name, true)}${timestamp}`

        const exportMethod = esm ? "export" : "module.exports ="

        return `/**
 * @typedef {import('typeorm').QueryRunner} QueryRunner
 * @typedef {import('typeorm').MigrationInterface} MigrationInterface
 */

/**
 * @class
 * @implements {MigrationInterface}
 */
${exportMethod} class ${migrationName} {
    name = '${migrationName}'

    /**
     * @param {QueryRunner} queryRunner
     * @returns {Promise<void>}
     */
    async up(queryRunner) {
${upSqls.join(`
`)}
    }

    /**
     * @param {QueryRunner} queryRunner
     * @returns {Promise<void>}
     */
    async down(queryRunner) {
${downSqls.join(`
`)}
    }
}
`
    }

    /**
     *
     */
    protected static prettifyQuery(query: string) {
        const formattedQuery = format(query, { indent: "    " })
        return (
            "\n" + formattedQuery.replace(/^/gm, "            ") + "\n        "

Review comment (suggestion):
If you want to add TSDoc for the up/down methods, you might want add it in the `MigrationInterface`, instead of here. I will be propagated to all migrations and won't be any unnecessary boilerplate code generated for the users.

Code after:
        upSqls: string[],
        downSqls: string[],
        esm: boolean,
    ): string {
        const migrationName = `${camelCase(name, true)}${timestamp}`

        const exportMethod = esm ? "export" : "module.exports ="

        return `/**
 * @typedef {import('typeorm').MigrationInterface} MigrationInterface
 */

/**
 * @class
 * @implements {MigrationInterface}
 */
${exportMethod} class ${migrationName} {
    name = '${migrationName}'

    async up(queryRunner) {
${upSqls.join(`
`)}
    }

    async down(queryRunner) {
${downSqls.join(`
`)}
    }
}
`
    }

    /**
     *
     */
    protected static prettifyQuery(query: string) {
        const formattedQuery = format(query, { indent: "    " })
        return (
            "\n" + formattedQuery.replace(/^/gm, "            ") + "\n        "
        )
    }
}
Repository: typeorm/typeorm
File: src/driver/types/GeoJsonTypes.ts (TypeScript)

Code before:
/**
 * Position object.
 * https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.1
 */
export type Position = [longitude: number, latitude: number, altitude?: number]

/**
 * Point geometry object.
 * https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.2
 */
export type Point = {
    type: "Point"
    coordinates: Position
}

/**
 * LineString geometry object.
 * https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.4
 */
export type LineString = {
    type: "LineString"
    coordinates: Position[]
}

/**
 * Polygon geometry object.
 * https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.6
 */
export type Polygon = {
    type: "Polygon"

Review comment (bug):
While this matches the spec, this change requires that everyone types their coordinates as a `Position` (or similar). This will cause errors for most people using GeoJSON.

Consider the following ([ts playground](https://www.typescriptlang.org/play/?#code/C4TwDgpgBACg9gZwJbCXAdlAvFA2gGwwHMUBXAEwgC4p1SBbAIwgCcAaKfAQ1WAutoNm7KF3y9+Afhp0mrALoAoRQDNS6AMaoMUcnADKcehGAALJOiIAKMIhRp0NeMm3oAlFADeiqL6gsTUhZMWxcHRQBfZQB6aKgAQRYiBgh0YCg4FShQSCgAcllhXHk8qCQEWjh0rgRkInQuRnxoYDgoMC4WLmNgVgysnOg853sMPIA6RVi-ABVOohN-CABHUiQAioAmKAhm4zSrBA9GUnSEOCCNaHouEChTLgA3aBUIAHdWSY0MBHTQ+OweAALABWDigpR6Qw9cyWGyIeJuGJxc4wixEKDIehIbgsbJtMzlKBvHH4UT4N63CrMJardYQciKb7oX7tRAAIScdlcgNwoPBIMhBiMJlh1lC7KRiiAA))

```ts
type Position = [longitude: number, latitude: number, altitude?: number]

function doSomething(position: Position) {
    return position
}

// Fails with type error
// Argument of type 'number[]' is not assignable to parameter of type 'Position'.
//   Target requires 2 element(s) but source may have fewer.
const posA = [45, 45]
doSomething(posA)

// Succeeds
const posB: Position = [45, 45]
doSomething(posB)
```

We should follow the lead of the geojson types defined here unless we have a very strong reason to deviate. They have a good explanation as to why Position has remained defined as `number[]`. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/5d7e8c0570c4c64da0133593c0e83edbce4a1b98/types/geojson/index.d.ts#L24-L65

Code after:
/**
 * Position object.
 * https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.1
 */
export type Position = number[]

/**
 * Point geometry object.
 * https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.2
 */
export type Point = {
    type: "Point"
    coordinates: Position
}

/**
 * LineString geometry object.
 * https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.4
 */
export type LineString = {
    type: "LineString"
    coordinates: Position[]
}

/**
 * Polygon geometry object.
 * https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.6
 */
export type Polygon = {
    type: "Polygon"
Repository: typeorm/typeorm
File: src/driver/sap/SapQueryRunner.ts (TypeScript)

Code before:
        return results.map((result) => result["SCHEMA_NAME"])
    }

    /**
     * Checks if database with the given name exist.
     */
    async hasDatabase(database: string): Promise<boolean> {
        const databases = await this.getDatabases()
        return databases.indexOf(database) !== -1
    }

    /**
     * Returns current database.
     */
    async getCurrentDatabase(): Promise<string> {
        const currentDBQuery = await this.query(
            `SELECT "DATABASE_NAME" AS "db_name" FROM "SYS"."M_DATABASE"`,
        )
        return currentDBQuery[0]["db_name"]
    }

    /**
     * Returns the database server version.
     */
    async getVersion(): Promise<string> {
        const currentDBQuery = await this.query(
            `SELECT "VERSION" AS "version" FROM "SYS"."M_DATABASE"`,
        )
        return currentDBQuery[0]["version"]
    }

    /**
     * Checks if schema with the given name exist.
     */
    async hasSchema(schema: string): Promise<boolean> {
        const schemas = await this.getSchemas()
        return schemas.indexOf(schema) !== -1
    }

    /**
     * Returns current schema.
     */
    async getCurrentSchema(): Promise<string> {
        const currentSchemaQuery = await this.query(
            `SELECT CURRENT_SCHEMA AS "schema_name" FROM "SYS"."DUMMY"`,
        )
        return currentSchemaQuery[0]["schema_name"]
    }

    /**
     * Checks if table with the given name exist in the database.

Review comment (suggestion):
```suggestion
        const currentDBQuery: [{ version: string }] = await this.query(
```

Code after:
        return results.map((result) => result["SCHEMA_NAME"])
    }

    /**
     * Checks if database with the given name exist.
     */
    async hasDatabase(database: string): Promise<boolean> {
        const databases = await this.getDatabases()
        return databases.indexOf(database) !== -1
    }

    /**
     * Returns current database.
     */
    async getCurrentDatabase(): Promise<string> {
        const currentDBQuery: [{ dbName: string }] = await this.query(
            `SELECT "DATABASE_NAME" AS "dbName" FROM "SYS"."M_DATABASE"`,
        )

        return currentDBQuery[0].dbName
    }

    /**
     * Returns the database server version.
     */
    async getVersion(): Promise<string> {
        const currentDBQuery: [{ version: string }] = await this.query(
            `SELECT "VERSION" AS "version" FROM "SYS"."M_DATABASE"`,
        )

        return currentDBQuery[0].version
    }

    /**
     * Checks if schema with the given name exist.
     */
    async hasSchema(schema: string): Promise<boolean> {
        const schemas = await this.getSchemas()
        return schemas.indexOf(schema) !== -1
    }

    /**
     * Returns current schema.
     */
    async getCurrentSchema(): Promise<string> {
        const currentSchemaQuery: [{ schemaName: string }] = await this.query(
            `SELECT CURRENT_SCHEMA AS "schemaName" FROM "SYS"."DUMMY"`,
        )

        return currentSchemaQuery[0].schemaName
    }
Repository: typeorm/typeorm
File: test/functional/metadata-builder/relation-join-column-builder/entity/City.ts (TypeScript)

Code before:
import { Column, Entity, PrimaryColumn, Unique } from "../../../../../src"

@Entity()
@Unique(["id", "countryId"])
export class City {
    @PrimaryColumn()
    id: number

    @Column()
    countryId: number

    @Column()
    name: string
}


Review comment (suggestion):
Primary keys (in this case, `id`) already uniquely identify a row. So a unique constraint that includes the primary key is unnecessary.

Code after:
import { Column, Entity, PrimaryColumn } from "../../../../../src"

@Entity()
export class City {
    @PrimaryColumn()
    name: string

    @PrimaryColumn()
    countryName: string

    @Column()
    population: number
}
Repository: typeorm/typeorm
File: test/github-issues/1738/queries.ts (TypeScript)

Code before:
export const CASE_1_UNFORMATTED = `SELECT * FROM tbl`
export const CASE_1_FORMATTED = `SELECT
  *
FROM
  tbl`

export const CASE_2_UNFORMATTED = `SELECT tbl1.col1, tbl2.col2 FROM tbl1 INNER JOIN tbl2 ON tbl1.id = tbl2.id`
export const CASE__FORMATTED = `SELECT
  tbl1.col1,
  tbl2.col2
FROM
  tbl1
  INNER JOIN tbl2 ON tbl1.id = tbl2.id`

export const FORMAT_SQL_TEST_CASES = [
    {
        unformatted: CASE_1_UNFORMATTED,
        formatted: CASE_1_FORMATTED,
    },
    {
        unformatted: CASE_2_UNFORMATTED,
        formatted: CASE__FORMATTED,
    },
]


Review comment (suggestion):
```suggestion
export const CASE_2_FORMATTED = `SELECT
```

Code after:
export const CASE_1_UNFORMATTED = `SELECT * FROM tbl`
export const CASE_1_FORMATTED = `SELECT *
FROM tbl`

export const CASE_2_UNFORMATTED = `SELECT tbl1.col1, tbl2.col2 FROM tbl1 INNER JOIN tbl2 ON tbl1.id = tbl2.id`
export const CASE_2_FORMATTED = `SELECT tbl1.col1,
    tbl2.col2
FROM tbl1
    INNER JOIN tbl2 ON tbl1.id = tbl2.id`

export const CASE_3_UNFORMATTED = `SELECT col1, (SELECT col2 FROM tbl2 WHERE tbl2.id = tbl1.id) AS sub_col FROM tbl1`
export const CASE_3_FORMATTED = `SELECT col1,
    (
        SELECT col2
        FROM tbl2
        WHERE tbl2.id = tbl1.id
    ) AS sub_col
FROM tbl1`

export const FORMAT_SQL_TEST_CASES = [
    {
        unformatted: CASE_1_UNFORMATTED,
        formatted: CASE_1_FORMATTED,
    },
    {
        unformatted: CASE_2_UNFORMATTED,
        formatted: CASE_2_FORMATTED,
    },
    {
        unformatted: CASE_3_UNFORMATTED,
        formatted: CASE_3_FORMATTED,
    },
]
Repository: typeorm/typeorm
File: test/github-issues/1738/queries.ts (TypeScript)

Code before:
export const CASE_1_UNFORMATTED = `SELECT * FROM tbl`
export const CASE_1_FORMATTED = `SELECT
  *
FROM
  tbl`

export const CASE_2_UNFORMATTED = `SELECT tbl1.col1, tbl2.col2 FROM tbl1 INNER JOIN tbl2 ON tbl1.id = tbl2.id`
export const CASE__FORMATTED = `SELECT
  tbl1.col1,
  tbl2.col2
FROM
  tbl1
  INNER JOIN tbl2 ON tbl1.id = tbl2.id`

export const FORMAT_SQL_TEST_CASES = [
    {
        unformatted: CASE_1_UNFORMATTED,
        formatted: CASE_1_FORMATTED,
    },
    {
        unformatted: CASE_2_UNFORMATTED,
        formatted: CASE__FORMATTED,
    },
]


Review comment (suggestion):
```suggestion
        formatted: CASE_2_FORMATTED,
```

Code after:
export const CASE_1_UNFORMATTED = `SELECT * FROM tbl`
export const CASE_1_FORMATTED = `SELECT *
FROM tbl`

export const CASE_2_UNFORMATTED = `SELECT tbl1.col1, tbl2.col2 FROM tbl1 INNER JOIN tbl2 ON tbl1.id = tbl2.id`
export const CASE_2_FORMATTED = `SELECT tbl1.col1,
    tbl2.col2
FROM tbl1
    INNER JOIN tbl2 ON tbl1.id = tbl2.id`

export const CASE_3_UNFORMATTED = `SELECT col1, (SELECT col2 FROM tbl2 WHERE tbl2.id = tbl1.id) AS sub_col FROM tbl1`
export const CASE_3_FORMATTED = `SELECT col1,
    (
        SELECT col2
        FROM tbl2
        WHERE tbl2.id = tbl1.id
    ) AS sub_col
FROM tbl1`

export const FORMAT_SQL_TEST_CASES = [
    {
        unformatted: CASE_1_UNFORMATTED,
        formatted: CASE_1_FORMATTED,
    },
    {
        unformatted: CASE_2_UNFORMATTED,
        formatted: CASE_2_FORMATTED,
    },
    {
        unformatted: CASE_3_UNFORMATTED,
        formatted: CASE_3_FORMATTED,
    },
]
Repository: typeorm/typeorm
File: src/data-source/DataSource.ts (TypeScript)

Code before:
        )
        ObjectUtils.assign(this, { migrations: migrations })

        // validate all created entity metadatas to make sure user created entities are valid and correct
        entityMetadataValidator.validateMany(
            this.entityMetadatas.filter(
                (metadata) => metadata.tableType !== "view",
            ),
            this.driver,
        )

        // set current data source to the entities
        for (const entityMetadata of entityMetadatas) {
            if (
                InstanceChecker.isBaseEntityConstructor(entityMetadata.target)
            ) {
                entityMetadata.target.useDataSource(this)
            }
        }
    }

    /**
     * Get the replication mode SELECT queries should use for this datasource by default
     */
    defaultReplicationModeForReads(): ReplicationMode {
        if ("replication" in this.driver.options && this.driver.options.replication) {
            const value = (
                this.driver.options.replication as {
                    defaultMode?: ReplicationMode
                }
            ).defaultMode
            if (value) {
                return value
            }
        }
        return "slave"
    }
}


Review comment (suggestion):
`if (this.driver.options.replication) {` should be enough.

Code after:
        )
        ObjectUtils.assign(this, { migrations: migrations })

        // validate all created entity metadatas to make sure user created entities are valid and correct
        entityMetadataValidator.validateMany(
            this.entityMetadatas.filter(
                (metadata) => metadata.tableType !== "view",
            ),
            this.driver,
        )

        // set current data source to the entities
        for (const entityMetadata of entityMetadatas) {
            if (
                InstanceChecker.isBaseEntityConstructor(entityMetadata.target)
            ) {
                entityMetadata.target.useDataSource(this)
            }
        }
    }

    /**
     * Get the replication mode SELECT queries should use for this datasource by default
     */
    defaultReplicationModeForReads(): ReplicationMode {
        if (
            "replication" in this.driver.options &&
            this.driver.options.replication
        ) {
            const value = (
                this.driver.options.replication as {
                    defaultMode?: ReplicationMode
                }
            ).defaultMode
            if (value) {
                return value
            }
        }
        return "slave"
    }
}
Repository: typeorm/typeorm
File: test/github-issues/11440/issue-11440.ts (TypeScript)

Code before:
import { expect } from "chai"
import "reflect-metadata"

import { DataSource } from "../../../src"
import {
    closeTestingConnections,
    createTestingConnections,
    reloadTestingDatabases,
} from "../../utils/test-utils"
import { Post } from "./entity/Post"

describe("github issues > #11440", () => {
    let dataSources: DataSource[]

    before(async () => {
        dataSources = await createTestingConnections({
            entities: [Post],
            enabledDrivers: ["postgres", "aurora-postgres", "cockroachdb"], // since the issue is for postgres
        })
    })
    beforeEach(() => reloadTestingDatabases(dataSources))
    after(() => closeTestingConnections(dataSources))

    it("should use table or alias name during upsert or doUpdate when both schema name and skipUpdateIfNoValuesChanged supplied", async () => {
        Promise.all(
            dataSources.map(async (dataSource) => {
                const repository = dataSource.getRepository(Post)
                await repository.save([
                    {
                        id: 1,
                        title: "First Post",
                    },
                    {
                        id: 2,
                        title: "Second Post",
                    },
                    {
                        id: 3,
                        title: "Third Post",
                    },
                ])

                // upsert does not cast alias as class name

Review comment (suggestion):
nit
```suggestion
            enabledDrivers: ["postgres", "aurora-postgres", "cockroachdb"],
```

Code after:
import { expect } from "chai"
import "reflect-metadata"

import { DataSource } from "../../../src"
import {
    closeTestingConnections,
    createTestingConnections,
    reloadTestingDatabases,
} from "../../utils/test-utils"
import { Post } from "./entity/Post"

describe("github issues > #11440", () => {
    let dataSources: DataSource[]

    before(async () => {
        dataSources = await createTestingConnections({
            entities: [Post],
            enabledDrivers: ["postgres", "aurora-postgres", "cockroachdb"],
        })
    })
    beforeEach(() => reloadTestingDatabases(dataSources))
    after(() => closeTestingConnections(dataSources))

    it("should use table or alias name during upsert or doUpdate when both schema name and skipUpdateIfNoValuesChanged supplied", async () => {
        Promise.all(
            dataSources.map(async (dataSource) => {
                const repository = dataSource.getRepository(Post)
                await repository.save([
                    {
                        id: 1,
                        title: "First Post",
                    },
                    {
                        id: 2,
                        title: "Second Post",
                    },
                    {
                        id: 3,
                        title: "Third Post",
                    },
                ])

                // upsert does not cast alias as class name
Repository: typeorm/typeorm
File: src/persistence/SubjectDatabaseEntityLoader.ts (TypeScript)

Code before:
        )

        await Promise.all(promises)
    }

    // ---------------------------------------------------------------------
    // Protected Methods
    // ---------------------------------------------------------------------

    /**
     * Finds subjects where entity like given subject's entity.
     * Comparison made by entity id.
     * Multiple subjects may be returned if duplicates are present in the subject array.
     * This will likely result in the same row being updated multiple times during a transaction.
     */
    protected findByPersistEntityLike(
        entityTarget: Function | string,
        entity: ObjectLiteral,
    ): Subject[] {
        return this.subjects.filter((subject) => {
            if (!subject.entity) return false

            if (subject.entity === entity) return true

            return (
                subject.metadata.target === entityTarget &&
                subject.metadata.compareEntities(
                    subject.entityWithFulfilledIds!,
                    entity,
                )
            )
        })
    }

    /**
     * Groups given Subject objects into groups separated by entity targets.
     */
    protected groupByEntityTargets(): {
        target: Function | string
        subjects: Subject[]
    }[] {
        return this.subjects.reduce((groups, operatedEntity) => {
            let group = groups.find(
                (group) => group.target === operatedEntity.metadata.target,
            )
            if (!group) {
                group = { target: operatedEntity.metadata.target, subjects: [] }
                groups.push(group)
            }
            group.subjects.push(operatedEntity)
            return groups

Review comment (suggestion):
Not needed now because `allSubjects` have a common `target`

Code after:
        await Promise.all(promises)
    }

    // ---------------------------------------------------------------------
    // Protected Methods
    // ---------------------------------------------------------------------

    /**
     * Groups given Subject objects into groups separated by entity targets.
     */
    protected groupByEntityTargets(): {
        target: Function | string
        subjects: Subject[]
    }[] {
        return this.subjects.reduce((groups, operatedEntity) => {
            let group = groups.find(
                (group) => group.target === operatedEntity.metadata.target,
            )
            if (!group) {
                group = { target: operatedEntity.metadata.target, subjects: [] }
                groups.push(group)
            }
            group.subjects.push(operatedEntity)
            return groups
        }, [] as { target: Function | string; subjects: Subject[] }[])
    }
}
Repository: typeorm/typeorm
File: src/util/StringUtils.ts (TypeScript)

Code before:
        return acc
    }, [])

    return shortSegments.join(separator)
}

interface IHashOptions {
    length?: number
}

/**
 * Returns a SHA-1 hex digest for internal IDs/aliases (not for cryptographic security)
 *
 * @param input String to be hashed.
 * @param options.length Optionally, shorten the output to desired length.
 */
export function hash(input: string, options: IHashOptions = {}): string {
    const hashFunction = shajs("sha1")
    hashFunction.update(input, "utf8")
    const hashedInput = hashFunction.digest("hex")
    const len = Number.isFinite(options.length as number)
        ? Math.trunc(options.length as number)
        : 0
    if (len > 0) {
        // slice already caps to hashedInput.length; the Math.min keeps intent explicit
        return hashedInput.slice(0, Math.min(len, hashedInput.length))
    }
    return hashedInput
}


Review comment (performance):
Do we really need this change? The extra validation is just adding unnecessary complexity IMO. 🤔 

This `hash` function is only ever used internally by drivers, which pass hardcoded `maxAliasLength` values. We never pass any user input to it, so there is no vector for exploiting this CVE in TypeORM that I can see.

`String.slice()` already handles `Infinity` and float values without trouble anyway, so the only part adding any safety here is the `len > 0` check against negative numbers. The rest I think can be reverted.

Code after:
        return acc
    }, [])

    return shortSegments.join(separator)
}

interface IHashOptions {
    length?: number
}

/**
 * Returns a SHA-1 hex digest for internal IDs/aliases (not for cryptographic security)
 *
 * @param input String to be hashed.
 * @param options.length Optionally, shorten the output to desired length.
 */
export function hash(input: string, options: IHashOptions = {}): string {
    const hashFunction = shajs("sha1")
    hashFunction.update(input, "utf8")
    const hashedInput = hashFunction.digest("hex")
    if (options.length && options.length > 0) {
        return hashedInput.slice(0, options.length)
    }
    return hashedInput
}