dotnet ef dbcontext scaffold doesn't work with computed columns unless user is db_owner

[yanbu0]

To reproduce:
Create a new DB called TestDB, add table Table_1, add column "test" make primary key, run alter table Table_1 add CalcField as (test + 'test-o-rama')

then from ef core project run

Result, note .HasComputedColumnSql is there

Now run with a user that has only db_datareader and db_datawriter:

Everything still scaffolds, and there is no error, but the .HasComputedColumn is gone, which really messes things up when trying to write back to the database since EF now tries to overwrite that column resulting in errors.

One would think that db_datareader would either be enough to scaffold correctly, or an error would throw. It would be vastly preferable to not have to give everyone db_owner if they need to scaffold the DB.

[smitpatel]

I think @ErikEJ did similar investigation for some other issue.

If whatever permission user has is not enough for us to get required data then schema generated will be based on data we got from server.

[ErikEJ]

@smitpatel Yes, this was my findings: ErikEJ/EFCorePowerTools#515 (comment)

[ErikEJ]

@yanbu0 no need for db_owner or even db_datareader, just VIEW DEFINITION right is needed.

[yanbu0]

@smitpatel @ErikEJ Understood its a permissions issue, I guess my Issue submission here is more related to the fact that a lower than required level of permissions does not result in a scaffolding error, it results in incorrect scaffolding which will cause errors in the application using the model at runtime. Seems like throwing an error when not having the view definition right would be the better scenario here. It took me forever to figure out what was going on.

[smitpatel]

Seems like throwing an error when not having the view definition right would be the better scenario here.

Not in all cases. If a user is scaffolding a database which does not have objects requiring view definition, should be able to get scaffolded code. There won't be any error for such user. If we start throwing an error then it is breaking a working case scenario. If view definition is not granted then we cannot see that if there is something hiding behind it to throw conditionally.

[ErikEJ]

Agree with @smitpatel - only an issue with the definition column in sys.default_constraints

[yanbu0]

I'd probably take the stance that its better to require additional permissions than build the tool to allow bugs to hit production.

In the scenario where a developer (who has full rights to his local db and can scaffold properly) is using some sort of pipeline to push to prod, where the pipeline scaffolding has least permissions, then someone adds a computed column, this will almost certainly pass all integration tests and hit production. Maybe a flag in the cli would be the best of both worlds? If you know you don't have anything that requires view definition you can override with a '--noVewDef' or something, but default is that it is required? Maybe something in the docs at least?

Just suggesting this since I was chasing my tail for a while figuring this out and if we hadn't been doing quite a bit of manual testing this would have hit prod.

[ErikEJ]

this will almost certainly pass all integration tests and hit production

Why is that? Having an integration test doing inserts does not seem unreasonable?

Also seems a bit odd to change source code in the pipeline !?

[smitpatel]

• Generated code should not be pushed to prod automatically. It is machine generated code. You should always inspect the code to verify it matches your expectation and test it properly before pushing to prod.
• If someone adds a computed column then there should also be a integration test verifying that computed column works correctly. That will easily capture that EF Core scaffolded model is missing something.

[yanbu0]

Trying to help make it better here guys, IMO leaving this to (maybe, hopefully) be caught by tests isn't the way to go, but its your product. Lord knows I've been using and liking it for a long time and I'm not going to quit just because of this, haha. Thanks.

[smitpatel]

We will discuss about a flag in next team meeting. Just blocking scenario which would be working does not feel like right way to go.

[ajcvickers]

I'm pretty sure we investigated this back in the EF6 days. I don't think there is a way to check for the permissions without that check itself running into issues. So ideally we would know that SQL Server is silently not returned things due to permissions, but in reality we can't determine that. However, I don't remember the details around this.

[ErikEJ]

Maybe log (Information?) if no default constraints found, that there could be a potential problem?

This seems to work as well:

Log on a limited user "nwreader" to Northwind (public role only):

SELECT HAS_PERMS_BY_NAME(DB_NAME(), 'DATABASE', 'VIEW DEFINITION');

Returns 0

As admin run in Northwind:

GRANT VIEW DEFINITION to nwreader

As limited user in Northwind run:

SELECT HAS_PERMS_BY_NAME(DB_NAME(), 'DATABASE', 'VIEW DEFINITION');

returns 1

[ErikEJ]

This also includes default constraint definitions.

[MikeYeager]

I'd like to add that we just ran into something very similar. In our case a dev ran the scaffolding with a login that had limited permissions. It ran, but didn't generate the usual warnings about bools with default store value of true and to consider using nullable CLR types. (Which is a nasty problem covered in dozens of issues already). In any case, he didn't notice tested his code, did his check in and broke all kinds of things. It created nullable CLR types in the models, defaulted strings to null instead of empty string. Turns out, when we inspected the generated code, it generated different code in the dbcontext. It couldn't read the default values, but didn't complain. It just spit out different code depending on which login we used. This is really NOT GOOD. I understand it can be difficult to determine if an error occurred in SQL Server or there just isn't any data, but I think this needs to be investigated and fixed. We're using EF (scaffolding) Tools v3.1.7. This cost a LOT of hours and money to track down.

[ErikEJ]

@MikeYeager I added a warning dialog to EF Core Power Tools to address this: https://github.com/ErikEJ/EFCorePowerTools/blob/master/src/GUI/EFCorePowerTools/Helpers/ReverseEngineerHelper.cs#L30

[MikeYeager]

Thank you! I'll make sure we're using this and checking for it from now on.

[ajcvickers]

@ErikEJ Is this something you might be able to submit a PR for? Or, if not, at least give some guidance on doing this based on your experience with the Power Tools?

[ErikEJ]

Sure, happy to. Would it be the proposed check with a warning (or even error) suggested here: #22842 (comment)

[ajcvickers]

@ErikEJ Warning is fine.

[ErikEJ]

Maybe the title should be changed to something like: unless user has been granted 'VIEW DEFINITION'

[IanKemp]

@ErikEJ There is an additional bug here as raised on SO (cross-posting for posterity in case that answer is deleted):

I got the same error and noticed, that when my DB name contains a dot ("Some.Database" for example) it triggers this bug. EF Core does:

SELECT HAS_PERMS_BY_NAME(DB_NAME(), 'DATABASE', 'VIEW DEFINITION');

On new empty db named "Some.Database" this returns 0, if i change the name to "Some_Database" it returns 1 making EF happy.

I have confirmed this is an issue on my local SQL 2019 developer instance, and that it is resolved by adding quotename:

SELECT HAS_PERMS_BY_NAME(quotename(DB_NAME()), 'DATABASE', 'VIEW DEFINITION');

Should a separate issue be filed for this?

[ErikEJ]

@IanKemp It has been fixed in 8.0 see #29610

[IanKemp]

You're way ahead of me as usual. Wouldn't it be a good fix to backport to EF 6 and 7 though?

[ErikEJ]

@IanKemp I doubt it reaches the high bar for backporting