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

feat: implement new virtual decorator #9339

Merged
merged 5 commits into from Sep 20, 2022

Conversation

CollinCashio
Copy link
Contributor

Description of change

Add a new feature that Fixes #9323

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run format to apply prettier formatting
  • 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
  • The new commits follow conventions explained in CONTRIBUTING.md

This new feature change bahviour of typeorm to allow use new calculated decorator

Closes typeorm#9323
package.json Outdated Show resolved Hide resolved
@Ginden
Copy link
Collaborator

Ginden commented Sep 1, 2022

"Calculated column" is term associated with PowerBI. "Computed column" refers to specific feature in multiple databases. Maybe call it "virtual column"?

Generally, good job!

@CollinCashio
Copy link
Contributor Author

CollinCashio commented Sep 1, 2022

"Calculated column" is term associated with PowerBI. "Computed column" refers to specific feature in multiple databases. Maybe call it "virtual column"?

Generally, good job!

Thank you. I really think this feature will add value to the community.

If you look at the history of the issue that I posted I originally called the calculated column decorator a "VirtualColumn" decorator because I believe that this is a better name myself, but the entity column Metadata already had a property being used for IsVirtual, so I figured I should go with a new name. I can for sure update this to VirtualColumn especially since like you said it is a better name, and I can add a new property called isVirtualDecorator to the entity column Metadata.

This feature has been renamed @Ginden back to to VirtualColumn

This new feature change bahviour of typeorm to allow use new virtual decorator

Closes typeorm#9323
This new feature change bahviour of typeorm to allow use new virtual decorator

Closes typeorm#9323
@CollinCashio CollinCashio changed the title feat: implement new calculated decorator feat: implement new virtual decorator Sep 1, 2022
docs/decorator-reference.md Outdated Show resolved Hide resolved
This new feature change bahviour of typeorm to allow use new calculated decorator

Closes typeorm#9323
@CollinCashio
Copy link
Contributor Author

How are we looking on this @Ginden ? It has been a week since our last discussion, and I want to ensure that this PR doesn't get lost in the backlog. Can this get merged in or is there something outstanding I need to do on my end?

@Ginden
Copy link
Collaborator

Ginden commented Sep 9, 2022

I'm merely helping with issues and reviews, to get them streamlined faster. At the end of day, decision, especially design decisions, belongs to @pleerock and other higher level maintainers.

@bjoveski
Copy link

thank you for doing this, this would be an extremely useful feature for us!

@H6LS1S
Copy link

H6LS1S commented Sep 17, 2022

@pleerock Сan this be released as a standalone release if it doesn't make it into the main release soon? This is a very useful feature for our project ( i think for others too ) after migrating from mikro-orm to typeorm

src/util/DateUtils.ts Outdated Show resolved Hide resolved
This new feature change behavior of typeorm to allow use of the new virtual column decorator

Closes typeorm#9323
@pleerock pleerock merged commit d305e5f into typeorm:master Sep 20, 2022
@pleerock
Copy link
Member

Thank you a lot for this contribution! Quite a solid implementation!

@joeruello
Copy link

joeruello commented Nov 22, 2022

@pleerock Was looking for something like this today and found it hasn't been released yet :(. Any ETA on when the next release will be cut with this PR included?

@pleerock
Copy link
Member

you can actually start using it in the latest dev version (check npm versions)

@kerhong
Copy link

kerhong commented Dec 7, 2022

This PR introduced a very breaking change about default behavior for number columns without specific type added.

class Foo {
    @Column()
    bar!: number;
}

This is now always pushed through parseInt (https://github.com/typeorm/typeorm/pull/9339/files#diff-3346a7798b4db06d6debd87138bb8322626b91c1c325b730d51ee3afc1b4c2bbR798-R800) and is therefore an integer, previously it worked fine with a floating point.

Was this change intended? Seems completely unrelated to the original function of this PR.

cc @pleerock

@pleerock
Copy link
Member

pleerock commented Dec 7, 2022

I'm not sure why these changes were needed, let's ask @CollinCashio

But in what cases previously it worked? @Column() bar: number always maps to int, if you want to use floating numbers, you must specify a type, e.g. @Column("double") bar: number

@kerhong
Copy link

kerhong commented Dec 7, 2022

@pleerock Than's for your quick response. Here is the use where I found this issue.

  1. typeorm@0.3.10
  2. Migration with
    new TableColumn({
      name: 'foo',
      type: 'float',
    }),
  3. Entity with
    @Column()
    foo!: number;

This would not round/floor the value, entity would contain the full float on fetch.

I am not sure to what extent should @Column() contain TableColumnOptions, it can be omitted, and all fields are optional.
Is there some documentation that explains how strictly the column details must be preserved between migrations and entities? Is there a way to validate entities against schema and/or migrations?

@pleerock
Copy link
Member

pleerock commented Dec 7, 2022

I would avoid complicating the report with migrations. I'm not sure where did you get the migration you wrote - if it's manually written, then it's not valid since it differs from the entity definition.

The best if you always specify column type in @Column decorator to be explicit. By default, again number type without @Column type specification maps to int. So in your case for your migration the valid column definition must be @Column("float") foo!: number;.

@kerhong
Copy link

kerhong commented Dec 8, 2022

@pleerock Thanks for the clarification, I wasn't aware that number is treated as int.

@PrimaryColumn("varchar", { length: 50 })
name: string;

@VirtualColumn({ query: (alias) => `SELECT COUNT("name") FROM "employees" WHERE "companyName" = ${alias}.name` })
Copy link

@Abobos Abobos Jan 4, 2023

Choose a reason for hiding this comment

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

I have a question. Can't the query be optional?.. I don't need to perform a subquery when I use a virtualColumn.

For context: https://pietrzakadrian.com/blog/virtual-column-solutions-for-typeorm#5-decorator-method

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

what if the virtualcolumn is not in the database at all but some other computed value?

Copy link
Contributor

Choose a reason for hiding this comment

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

background would be graphql since if you want a unique id field and have a composite key for a table you want to cramp that into a single key maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @Abobos and @sschneider-ihre-pvs ,

This functionality was designed specifically for subqueries. This solves the need of allowing your database engine to return queried result sets and put the data into your defined model (also allows you to use the DB engine to filter data from the model).

If you are just looking for a readonly property that has no need to pull database information and possibly pull from a third party API or some other data source, you could just use a regular property without any column decorator. IE your model could look like:

@Entity({ name: "timesheets" })
export default class TimeSheet extends BaseEntity {
    @PrimaryGeneratedColumn()
    id: number;

    hours: number;

    @AfterLoad()
    afterLoad() {
         // perform calculations here to set property of hours
    }
}

Copy link

@Abobos Abobos Jan 6, 2023

Choose a reason for hiding this comment

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

@CollinCashio VIrtual Column suffices for my use-case.. but I was thinking you can just add the column name as some alias in a query without having to do a subquery.

Thanks

@effective-giggle
Copy link

This PR created bug #10026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VirtualColumn Decorator
10 participants