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

As of EF7 Scaffold-DbContext doesn't handle columns computed from a function #30999

Closed
jkeslinke opened this issue May 30, 2023 · 15 comments
Closed
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@jkeslinke
Copy link

jkeslinke commented May 30, 2023

Per the EF7 breaking changes I understand Computed Columns and tables with Triggers need to be specially configured. Using Scaffold-DbContext handles the triggers and computed columns in most cases.

The one case we've run into that doesn't seem to be handles is a Computed Column that is generated from a function. i.e.

CREATE TABLE [dbo].[SomeTable](
        ...
	[totalCost]  AS ([dbo].[fn_calculateCost]([Id])),
        ...
)

We are scaffolding it like this:

Scaffold-DbContext "ConnectionString" Microsoft.EntityFrameworkCore.SqlServer -StartupProject Infrastructure.Data -Project Infrastructure.Data -OutputDir Scheduling -Context SchedulingContext -Tables SomeTable -Force -NoOnConfiguring -NoPluralize

Which generates this:


    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
		modelBuilder.Entity<SomeTable>(entity =>
        {
            ...
            entity.Property(e => e.TotalCost)
                .HasComputedColumnSql("([dbo].[fn_calculateCost]([Id]))", false)
                .HasColumnType("decimal(19, 4)")
                .HasColumnName("totalUnits");
			...
        });
		
        OnModelCreatingPartial(modelBuilder);
    }

The exception is: Column 'inserted.totalCost' cannot be referenced in the OUTPUT clause because the column definition contains a subquery or references a function that performs user or system data access. A function is assumed by default to perform data access if it is not schemabound. Consider removing the subquery or function from the column definition or removing the column from the OUTPUT clause.

The only way I have determined to work around this for now is to manually add "Triggers" to the EF Context Mapping for the functions used by the computed columns (per this post):

        partial void OnModelCreatingPartial(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<SomeTable>(entity =>
            {
                entity.ToTable(tbl => tbl.HasTrigger("([dbo].[fn_calculateCost]([Id]))"));
            });
        }

But this solution is not ideal. Not only does it only solve this singular issue, leaving other potential issues out there, but if we ever change the column/function we would have to remember to manually go in and update this line.

Ideally, it would be nice if the Scaffold-DbContext command picked up this change like it does so well for everything else.

EF Core version: 7.0.5
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 6.0
Operating system: Windows 10/Server 2016
IDE: Visual Studio 2022 17.4.5

Thank you,
Jeff

@roji
Copy link
Member

roji commented May 30, 2023

@jkeslinke this would require EF to parse the SQL in your computed columns to figure out that it contains a function; we generally don't parse as that's unfeasible.

@ErikEJ
Copy link
Contributor

ErikEJ commented May 30, 2023

@roji are computed columns even handled with regards to trigger detection??

@roji
Copy link
Member

roji commented May 30, 2023

@ErikEJ how do you mean? We do check triggers in reverse engineering to add that config (which avoids OUTPUT), but that isn't related to computed columns..

@ErikEJ
Copy link
Contributor

ErikEJ commented May 30, 2023

The OP wrote "Using Scaffold-DbContext handles the triggers and computed columns in most cases." - but I think that is a misconception?

@roji
Copy link
Member

roji commented May 30, 2023

Ah, right: IIRC we make no attempts to detect what's going on inside computed columns, and never scaffold HasTrigger because of computed columns - only because of triggers.

@jkeslinke
Copy link
Author

Thank you for the responses. As for my misconception, I didn't mean that's generally accepted/documented, just noticed from my observations that it seemed to handle our other scenarios (so far).

As for the solution, @roji is suggesting this is not solvable on the EF side. So would the workaround I posted (stole) from above be the suggested method going forwards or is there a better option (I don't want to enable triggers on all tables per the Breaking Changes Mitigation suggestion)?

@roji
Copy link
Member

roji commented May 31, 2023

@jkeslinke for 8.0, we've already introduced a more targeted way to opt out of the OUTPUT clause (#29916), without forcing you to define a fake trigger on it. However, for now, you'll have to configure fake triggers either on the specific problematic table, or everywhere - but you'll be able to switch from that to the new method in 8.0.

@jkeslinke
Copy link
Author

@roji Thanks for pointing that out, I didn't know it was being addressed in 8.0.

Looking at that item, and the fix for it #29917 I'm not clear on how this will work as it pertains to the Scaffold-DbContext command. Will it be available with that command and if so, will it be another parameter like the "-NoPluralize" which would affect everything in that context?

@roji
Copy link
Member

roji commented May 31, 2023

EF indeed won't scaffold the new UseSqlOutputClause() introduced in 8.0: where triggers are detected, HasTrigger will be scaffolded instead, and as mentioned above we don't parse SQL so we can't detect functions in computed columns. That means it will be up to you to add this in scaffolded models.

If you want to systematically add this to all tables, you can always customize the scaffolding templates or add a bit of bulk model configuration to do that.

@jkeslinke
Copy link
Author

I suppose I may be in the minority here, but we use this technique, re-scaffolding on all DB changes. So customizing the generated templates/OnModelCreating directly is not an option for us, which is why I used the partial in my solution above.

Just so I can put a bow on this: For anyone using the re-scaffold technique (even as of EF8), the only options are:

  1. The manual addition to the partial above on a per-case basis.
  2. Turning off the performance benefit of ignoring triggers by using the mitigation here.
  3. Modify the problematic tables in the DB.

Just wanting to make sure I have all the options covered and am not missing anything.

Thank you!

@ajcvickers
Copy link
Contributor

@jkeslinke

So customizing the generated templates/OnModelCreating directly is not an option for us

Can you provide more details on this? Why is it not an option?

@jkeslinke
Copy link
Author

@ajcvickers We use the re-scaffolding technique described here. As such we will re-scaffold, using the -force option, every time we make an update to the DB. This will re-generate the Context and Entities overwriting any customization we made to those files.

Per the link: "Since the scaffolded code will be overwritten, it is best not to modify it directly, but instead rely on partial classes and methods, and the mechanisms in EF Core that allow configuration to be overridden."

@ajcvickers
Copy link
Contributor

@jkeslinke The T4 templates are not overwritten. The T4 templates determine what gets scaffolded every time you do the scaffolding--that's the point of having them.

OnModelCreating is overwritten, but the partial method implementation in the partial class will not be overwritten.

@jkeslinke
Copy link
Author

@jkeslinke The T4 templates are not overwritten. The T4 templates determine what gets scaffolded every time you do the scaffolding--that's the point of having them.

Ok, yes, I see T4 templates are a new option in EF7 that I haven't considered. Thank you for pointing those out, but from just briefly reviewing them now, that seems pretty analogous to the method I have of using the partial right? Basically, I would put some custom code in the T4 template to generate the HasTrigger call on specific entities instead of putting it in the OnModelCreatingPartial.

To me, using the partial seems like the safer option as I don't include another set of code (the T4 templates) that I would need to maintain with each version upgrade. Or am I missing something here that would make the T4 template a superior (more dynamic choice)?

OnModelCreating is overwritten, but the partial method implementation in the partial class will not be overwritten.

Correct, this is what I'm doing and suggesting as option 1. I'm fine with this option, just trying to make sure I am making the right choice based on all the options. I'll add the T4 suggestion to that list here:

  1. The manual addition to the partial above on a per-case basis. (see OP for this solution)
  2. Turning off the performance benefit of ignoring triggers by using the mitigation here.
  3. Modify the problematic tables in the DB.
  4. Use T4 templates to add the HasTrigger on a per-case basis (not sure if this is significantly different than option 1?)

@roji
Copy link
Member

roji commented Jun 1, 2023

To me, using the partial seems like the safer option as I don't include another set of code (the T4 templates) that I would need to maintain with each version upgrade. Or am I missing something here that would make the T4 template a superior (more dynamic choice)?

I'd generally agree with this. Note that if you want to do this systematically for all tables, the code you add to the partial method can do bulk configuration rather than specify each table (which you then need to maintain).

I'm going to go ahead and close this issue as I think all questions have been answered, but if you have any further one feel free to post back here.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Jun 1, 2023
@roji roji added the closed-no-further-action The issue is closed and no further action is planned. label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

4 participants