Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use last item in value array in the case of duplicate columns (mssql) #8930

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/driver/sqlserver/SqlServerQueryRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,22 @@ export class SqlServerQueryRunner

if (raw?.hasOwnProperty("recordset")) {
result.records = raw.recordset
// mssql driver will return an array if duplicate columns exist
// The following will search for properties that return as an array, and then update the property with the last value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why exactly the last value? Why not the first, second or n-th?

Copy link
Contributor

@Ginden Ginden Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pleerock I think it's for consistency with other database drivers (this is reasoning given by author).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @Ginden . What's your opinion on this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings. Consistency is good, but underlying issue of duplicated column names looks iffy.

if (result.records?.length > 0) {
const propsToTakeForDuplicate = []
for (let prop in result.records[0]) {
if (Array.isArray(result.records[0][prop])) {
propsToTakeForDuplicate.push(prop)
}
}
for (let record of result.records) {
for (let prop of propsToTakeForDuplicate) {
const lastIdx = record[prop].length - 1
record[prop] = record[prop][lastIdx]
}
}
}
}

if (raw?.hasOwnProperty("rowsAffected")) {
Expand Down
26 changes: 26 additions & 0 deletions test/github-issues/7775/entities/test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import {
Entity,
JoinColumn,
ManyToOne,
OneToMany,
PrimaryGeneratedColumn,
} from "../../../../src"

@Entity("parent")
export class Parent {
@PrimaryGeneratedColumn()
id: number

@OneToMany((type) => Child, (child) => child.parent)
children: Child[]
}

@Entity("child")
export class Child {
@PrimaryGeneratedColumn()
id: number

@ManyToOne((type) => Parent, (parent) => parent.children)
@JoinColumn({ name: "parent_id" })
parent: Parent
}
72 changes: 72 additions & 0 deletions test/github-issues/7775/issue-7775.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { expect } from "chai"
import "reflect-metadata"
import { DataSource } from "../../../src"
import {
closeTestingConnections,
createTestingConnections,
reloadTestingDatabases,
} from "../../utils/test-utils"
import { Child, Parent } from "./entities/test"

describe("github issues > #7775 MSSQL: Duplicate columns names return as an array, potentially breaking mapping", () => {
let connections: DataSource[]
before(async () => {
connections = await createTestingConnections({
enabledDrivers: ["mssql"],
entities: [Parent, Child],
dropSchema: true,
schemaCreate: true,
})
})
beforeEach(() => reloadTestingDatabases(connections))
after(() => closeTestingConnections(connections))

it("should take the last column as the value, consistent with other drivers", () =>
Promise.all(
connections.map(async (connection) => {
const results = await connection.manager.query(
`SELECT 1 as id, 2 as id UNION SELECT 5, NULL`,
)
expect(results.length).to.be.eql(2)
expect(results[0].id).to.be.eql(2)
expect(results[1].id).to.be.eql(null)
}),
))

it("should map the data properly if duplicate columns are present", () =>
Promise.all(
connections.map(async (connection) => {
const parentRepo = await connection.getRepository(Parent)
const childRepo = await connection.getRepository(Child)

const parent = new Parent()
parent.id = 1

await parentRepo.save(parent)

const child = new Child()
child.id = 1
child.parent = parent

await childRepo.save(child)

const results = await connection
.getRepository(Child)
.createQueryBuilder("child")
.andWhere("parent.id = :p0")
.setParameters({ p0: 1 })
.select(
"child.id, child.parent"
.split(",")
.map((i) => i.trim()),
)
.leftJoinAndSelect("child.parent", "parent", "1 = 1")
.getMany()

expect(results.length).to.be.eql(1)
expect(results[0]).to.be.instanceOf(Child)
expect(results[0].parent).to.be.instanceOf(Parent)
expect(results[0].parent.id).to.be.eq(1)
}),
))
})