-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Conversation
…ssql) Brings mssql driver in line with other drivers in handling resultsets with duplicate columns Closes: typeorm#7775
It looks like this fix will leave the streamed results in the old format. Perhaps there needs to be a transform stream attached to the readable that normalises the streamed rows too? |
We do not change driver output for the |
I remember I had to introduce some changes (not remember where, maybe just in test cases) after mssql version upgrade. But we do not change the output of the underlying driver. If driver returns it this way, there maybe a reason? I can say that I have problems with design decisions on absolutely every driver TypeORM currently supports. Please don't give me another problem. |
I'm not too involved in TypeORM so, my opinion shouldn't hold too much weight, but I'll share it anyway. @blitzmann raises some good points in the linked issue tediousjs/node-mssql#1384. The mssql driver is returning duplicate column names in a way that is not typical of any other database driver at the moment (at least according to him). When there is a duplicate column name, the MSSQL driver is returning all the values for the duplicate column as an array rather than just returning the last value. Whilst I have my views (as the maintainer of the MSSQL lib) about which is "best", my view of an ORM/framework like TypeORM is to homogenise the DB drivers so that swapping database drivers is completely™️ transparent. ie: the idiosyncrasies are dealt with at the ORM level, rather than by the consumer of the ORM. That would mean if some odd-ball DB driver comes along and returns data in a completely unrecognised way (like mssql could return rows as arrays instead of objects) then it's for the ORM to coerce this data into the form that the ORM and the ORM consumer expects. In short, the contract is between the ORM and the ORM consumer, not between the ORM consumer and the DB driver. This change seems pretty sensible in the context of how all the other driver behave. |
@pleerock I can understand wanting to keep direct called to That being said, I fully agree with @dhensby as to the purpose of TypeORM, and couldn't say it better myself, so I'll just reiterate
@pleerock is there another area besides Alternatively, this can be introduced as an option for TypeORM to toggle this bit of code on and off, but I don't see the point in that since I can't imagine a scenario where someone would want TypeORM to return a mapped object like below.
Please advise :) |
@pleerock just seeing if there's been any more thought given to this. I'm still of the opinion that this should be at the ORM level to massage the data into a format that TypeORM is expecting. |
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I feel like I have to close this PR. Because:
Closing it for a better times. |
Description of change
Closes #7775
The
mssql
driver, by default, returns an array when the resultset has duplicate columns. For example:SELECT 1 as id, 2 as id
would restult in{id: [1, 2]}
See tediousjs/node-mssql#1384 upstream issue for more information and discussion. While I can understand the intent behind it, this is in contrast to how other drivers handle duplicate columns (in which they return, by default, the last column value). It seems that
mssql
is hesitant to introduce an option to automatically select for this behavior, citing data loss.For TypeORM, tho, we need drivers that return data in similar fashion consistently. This change will look for any arrays that
mssql
returns, and then loop over the resultset and replace that property with the last value.This is a fairly edge-case scenario - It's not normal for TypeORM to generate a query with duplicate columns, but it can happen (see test case entities for example), and this protects the mapper from returning odd results.
Please also see the issue in which this closes, #7775
Pull-Request Checklist
master
branchnpm run format
to apply prettier formattingnpm run test
passes with this change* Note: there is one failing test case, but it seems to be happening in master as well, so not related to this PR. I only ran tests using
mssql
in ormconfigFixes #0000
* Unsure if this is needed?