Efcore: Create resource strings for exception messages

[ajcvickers]

Filing this because some new 3.0 code (especially in query) is using string literals instead of resource strings.

[Muppets]

@ajcvickers I've started to work through this. After looking through about 200+ throw new exception in my search and only being about 1/5 of the way through (Fixed 35 exceptions already though), I thought there must be a better way. A friend at work then suggested, "why not build a code analyser to catch that scenario?". This really clicked with me since even when I finish this ticket, it'll need to be redone in a few years time. A code analyser would keep us on the straight and narrow every commit.

I can see we already have a code analyser project but it is intended for developers using EF, not developers writing the EF code base.

Looking around online, I see FxCop has CA1303: Do not pass literals as localized parameters which is a little too liberal.

What's our appetite for having an internal code analyser project for common best practices for the EF code base? Starting with the rule to not use string literals in exceptions.

[ajcvickers]

@Muppets First, I can't believe you are working on this! 😃 We are normally pretty good at doing this as we go along, but the schedule for 3.0 resulted in technical debt. If you do this for the team then you deserve several beers! 🍻🍻🍻🍻🍻🍻 (12 pints is a lot; you might want to share!)

Second, we typically don't use analyzers or FxCop inside the EF Core code because at least in the past they have been more trouble than they are worth, especially earlier in Core. (This may no longer be true.)

Instead we have ApiConsistencyTests. These does static analysis on our assemblies as part of every test run. This has been pretty flexible in allowing us to have the rules we want without the hassle of FxCop or analyzers.

So if you can figure how to detect when we're doing this wrong, then it would be good to add a test to the API consistency tests, but it's certainly not required.

[Muppets]

Having a dig around ApiConsistencyTests, it looks like it can only reflect on the public API surface. Analysing throw exceptions in the methods themselves doesn't look possible with what's there today.

[ajcvickers]

@Muppets Yes, the semantics available through Roslyn are not available here. That being said, I'm still not convinced that we need a good way to detect this. Normally if someone forgets to do it, then it gets called out in code review. The only reason we have a backlog of cases now is because of the query-rewrite and tight schedule for 3.0 where we focused on just getting working code in rather than doing this.

[ajcvickers]

Review strings are in appropriate assemblies.

[ajcvickers]

@smitpatel I've spent some time looking and in general I'm finding things in the right places. Were there specific sets of strings that you saw were in the wrong place?

[smitpatel]

EntityProjectionExpressionCalledWithIncorrectInterface
NullTypeMappingInSqlTree
UnsupportedBinaryOperator
VisitChildrenMustBeOverridden
IncorrectOperatorType
UnsupportedUnary
UnknownEntity
InvalidSwitch

[ajcvickers]

@smitpatel Thanks! I'll move those.

[ajcvickers]

I think maybe I found the disconnect here. Most of these are used by Relational _and_ Cosmos. Should we be creating duplicates for these? Or are we okay with keeping common strings in Core? /cc @dotnet/efcore

• [ ] EntityProjectionExpressionCalledWithIncorrectInterface - Used in Cosmos/Relational/InMemory
• [ ] NullTypeMappingInSqlTree - Used by Cosmos and Relational
• [ ] UnsupportedBinaryOperator - Used by Cosmos and Relational
• [ ] VisitChildrenMustBeOverridden - Used by Cosmos and Relational
• [ ] IncorrectOperatorType - Used by Cosmos and Relational
• [ ] UnsupportedUnary - Used by Cosmos and Relational
• [ ] UnknownEntity - Relational only, but filed #22073
• [ ] InvalidSwitch - Relational only

[AndriySvyryd]

I am ok with having the shared ones in Core.

[smitpatel]

EntityProjectionExpressionCalledWithIncorrectInterface - Used in Cosmos/Relational/InMemory
NullTypeMappingInSqlTree - Used by Cosmos and Relational
UnsupportedBinaryOperator - Used by Cosmos and Relational
VisitChildrenMustBeOverridden - Used by Cosmos and Relational
IncorrectOperatorType - Used by Cosmos and Relational
UnsupportedUnary - Used by Cosmos and Relational

I am against putting all of above in core. While they have same error message they have differences in data structure used inside query. If SelectExpression and QuerySqlGenerator are not core then the error message occurring from them should not be in core either.