Skip to content

fix: include asExpression columns in returning clause #8472

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.

Sign up for GitHub

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

Jump to bottom
Closed
Migushthe2nd wants to merge 5 commits into from
Closed

fix: include asExpression columns in returning clause #8472

Migushthe2nd wants to merge 5 commits into from

Conversation

@Migushthe2nd
Copy link

@Migushthe2nd Migushthe2nd commented Dec 19, 2021

Description

Fixes #8450

It is a bit unclear wether the isGenerated property on TableColumn and ColumnMetadata (and possibly in more places) should be set only if generationStrategy is defined as well. The property description in the ColumnMetadata class mentions it is meant for columns of "generated other way" (I read this as "any type of generation", including asExpression), but most of the code that utilizes the isGenerated field often does not adhere to this.

typeorm/src/metadata/ColumnMetadata.ts

Lines 83 to 86 in cefddd9

/**
* Indicates if this column is generated (auto increment or generated other way).
*/
isGenerated: boolean = false;

Perhaps this comment refers only to other generation strategies, like uuid?

I could think of two ways to solve the issue:

  1. If the isGenerated property is not restricted to generation strategies: go through all references of isGenerated and confirm or rewrite parts to make isGenerated a universal indicator for columns that are generated in some way.
  2. Make it clear that isGenerated should only be used for generation strategies. The fix to the actual issue would then be simple: change line 181 of the condition below to
(needToCheckGenerated && (column.isGenerated || !!column.asExpression))  ||

typeorm/src/query-builder/ReturningResultsEntityUpdator.ts

Lines 172 to 187 in 90a8deb

getInsertionReturningColumns(): ColumnMetadata[] {
// for databases which support returning statement we need to return extra columns like id
// for other databases we don't need to return id column since its returned by a driver already
const needToCheckGenerated = this.queryRunner.connection.driver.isReturningSqlSupported();
// filter out the columns of which we need database inserted values to update our entity
return this.expressionMap.mainAlias!.metadata.columns.filter(column => {
return column.default !== undefined ||
(needToCheckGenerated && column.isGenerated) ||
column.isCreateDate ||
column.isUpdateDate ||
column.isDeleteDate ||
column.isVersion;
});
}

This makes it such that columns with an asExpression are also included in the RETURNING clause when saving an entity.

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
    (I updated documentation in the source files, not docs directory)
  • The new commits follow conventions explained in CONTRIBUTING.md

@Migushthe2nd
Copy link
Author

Migushthe2nd commented Dec 19, 2021 •
edited
Loading

I do not understand why these tests are failing. I get the feeling there are other underlying issues.

@Migushthe2nd Migushthe2nd mentioned this pull request Dec 19, 2021
Closed
7 tasks
@Migushthe2nd Migushthe2nd marked this pull request as draft December 23, 2021 20:10
Migushthe2nd added a commit to ThemezerNX/API that referenced this pull request Dec 30, 2021
@Migushthe2nd
Add temporary patch for typeorm/typeorm#8472
46afb2f
@pleerock pleerock requested a review from AlexMesser January 15, 2022 15:03
@TheNoim TheNoim mentioned this pull request Jan 28, 2022
Closed
7 tasks
@pleerock
Copy link
Member

pleerock commented Feb 15, 2022

I'm going to close this one because I'm doing unfinished PRs cleanup (to prevent conflicts). Please feel free to rebase it on latest master and reopen it with all other changes applied. Thank you.

@pleerock pleerock closed this Feb 15, 2022
@maks-rafalko-unagi maks-rafalko-unagi mentioned this pull request Sep 13, 2023
Closed
@maks-rafalko-unagi
Copy link

maks-rafalko-unagi commented Sep 15, 2023 •
edited
Loading

For anyone interested in a possible solution for 0.3.17 (latest at the moment of writing), we have fixed it manually by applying a patch, see #8450 (comment)

IMO: this PR should be reconsidered and reopened

@maartensijmkens maartensijmkens mentioned this pull request Jan 15, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexMesser AlexMesser Awaiting requested review from AlexMesser

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Generated asExpression columns not in RETURNING clause on save

3 participants

@Migushthe2nd @pleerock @maks-rafalko-unagi