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

Unable to addSelect with computed result #7008

Open
Asarew opened this issue Nov 3, 2020 · 5 comments
Open

Unable to addSelect with computed result #7008

Asarew opened this issue Nov 3, 2020 · 5 comments

Comments

@Asarew
Copy link

Asarew commented Nov 3, 2020

when using a computed selection the property doesn't get hydrated

@Entity()
export class Post {

    @PrimaryGeneratedColumn()
    id: number;

    @Column()
    title: string;

    @Column({ type: String, nullable: true })
    text: string|null;
}
const query = connection.manager.createQueryBuilder(Post, "post");
const loadedPost = await query
    .select("post.id")
    .addSelect("SUBSTRING(post.text, 1, 20)", "post_text")
    .getOne();

if (!loadedPost.text) {
   throw new Error('text not hydrated');
} else if (loadedPost.length > 20) {
  throw new Error('text length to large');
} else {
  console.log('everything is all good')
}

Expected Behavior

This would output everything is all good.

Actual Behavior

Error is thrown: text not hydrated

Are you willing to resolve this issue by submitting a Pull Request?

I've already fixed this issue in this PR: #4703 but it seems to getting ignored due to the fact that it doesn't support all use cases of the parties involved that are mainly interested in addSelectAndMap.
But because a feature PR for that is currently in the works #6855, i think that we can just merge #4703 as it doesn't introduce any new features but fixes a Very long standing bug.

@nebkat
Copy link
Contributor

nebkat commented Nov 3, 2020

@Asarew I share your frustration at the lack of this feature for so long but this really isn't constructive. The developers are clearly aware of this and you are simply wasting their time by creating new issues. I am working on a solution that will hopefully satisfy all parties because personally I don't think any of the provided solutions are actually going to be good long term.

@Asarew
Copy link
Author

Asarew commented Nov 3, 2020

@nebkat The bug that I'm describing here has nothing to do with addSelectAndMap. So I'm not "waisting their time" except yours, as you clearly haven't looked into the issue. addSelectAndMap works with plain properties and NOT with columns. #6855 (comment)

@nebkat
Copy link
Contributor

nebkat commented Nov 3, 2020

@Asarew I think the fact that you have been able to link to comments, issues and pull requests that all describe your situation shows that this is a known and duplicate issue, which therefore makes this a waste of their time.

I can assure you I have looked into your issue and all of the related issues from the past.

In your example you are using a computed expression SUBSTRING(post.text, 1, 20), and trying to place it in Post.text. Post.text is annotated with @Column, i.e. it is directly connected to a column in the database. This property should only ever contain the intended database value. I know you have said in the other thread that you do not synchronize your entities, so this is not relevant for you, but that is not the typical case. If someone was to run this query, then later run entityManager.save(loadedPost), the column would be overwritten with the computed value. This to me looks like unintended behavior, and can only lead to confusion.

Additionally, your method of achieving this is by artificially recreating the alias that you would expect the standard column selection and mapping process to use, then hoping that it will be correctly mapped back to the entity property. Even if we were to ignore your motivations for mapping a computed expression to the column property and the associated issues, your intention was ultimately to have a Post object with post.text set to the computed value. The alias the mapping system uses internally is not relevant to this intention, but rather a hack, which is why this can't be considered a "fix" at all. The proper solution would involve you specifying the computed expression, and the property path where you would like it to end up.

When you combine these two things, the obvious solution is addSelectAndMap. You define an additional property called shortText, then do .addSelectAndMap('SUBSTRING(post.text, 1, 20)', 'shortText'). If the internal alias mapping system is ever to change, you are unaffected and there is mixing of computed and real values.

As I said, I am also frustrated by the lack of progress, especially when seeing the discussion go all the way back to 2017 - I am currently migrating a full stack to Nest/TypeORM and this is quite a major issue for me. However, the solution is not to merge quick hacks just because a full feature implementation is not ready. You have offered your solution and it was objected for good reasons, not "ignored". The great thing about this being an open source project is that you can fork it and apply whatever fixes you want for use in your own projects.

@Asarew
Copy link
Author

Asarew commented Nov 3, 2020

You are correct that the annotated property should only contain the intended database value. that is one reason why i believe there is a difference between this bug fix and addSelectAndMap. The later would impose the possibility to override the type of property/column in the database when building the query.
An example of this is the Geometry column that use ST_AsGeoJSON that is hard coded in Typeorm. Which has the support for passing in additional options, to return the "intended" value with more declerative output.

Additionally, addSelectAndMap would bypass any and all existing logic when it comes to @Column properties, like the valueTransformer option. in my opinion addSelectAndMap is intended to be used with 'plain' properties and addSelect with @Column properties.

your statement that the internal use of the mapping system is not relevant doesn't make sense to me as the selection of the alias is already used by typeorm to identify which property to populate. addSelect supports the ability to pass in a function to build more expressive select statements, The only possibility would be to 'build' the same single supported property selection to hydrate the entity. Therefor not using the alias to identify the same property seems more like a bug than a conscious decision

You add up the 2 items to come to the conclusion that only addSelectAndMap would be a obvious solution which is not the case. There are 2 distinct issue's at hand here, one with properties annotated with @Column and one without, each should have it's own solution or one that is uniform for both.

@nebkat
Copy link
Contributor

nebkat commented Nov 3, 2020

Please take a look at my proposed pull request for selectAndMap then, I think it achieves precisely what you mentioned since it uses the same type mapping as @Column, which also includes the option for a valueTransformer.

A core part of any ORM is that it does the mapping of raw database output to the object for you (its literally in the name I guess). As far as the designer is concerned, the only thing that really matters is the entity and its properties - how the ORM chooses to map from the object to the database and vice-versa is not nearly as important.

Of course, sometimes we care about the names assigned to columns because they might be read directly from a non-ORM client, which is why we are given the freedom to define the column names ourselves, both manually and through naming schemes. Beyond that however, the ability of the ORM to map my_custom_column_name to user.id should be considered an internal process.

TypeORM happens to do this by generating column aliases in the form ${tableAlias}_${databaseName}, which looks fairly "standard", but it is still an internal process. It could be changed to a hash of this value, use a double underscore, etc, for a variety of reasons. One example for such a change would be to avoid a conflict between ${'user'}_${'post_id'} and ${'user_post'}_${'id'}. This is why it is important to keep this considered an internal process. Right now this change could be made internally and it would have 0 effect on existing setups, meaning it can be made as part of a minor release rather than a major release.

If your proposed change is adopted we then mix the internal mapping process with user input, when in reality you should not care about how the ORM goes about achieving this internally. addSelect does have an alias parameter but it is currently only used within the select statements themselves rather than being mapped to a property. In that case, since both the declaration of the alias and its subsequent use in where, order by, etc, are done by the designer, there is no risk of an internal change affecting it.

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

No branches or pull requests

2 participants