Add analyzer/codefix for usage of interpolated strings in raw query methods #30835
Conversation
DoctorKrolic
changed the title
DoctorKrolic
changed the title
|
Thanks for the quality work on this @DoctorKrolic! Before doing any extra work, let me confirm with the team that we want to go ahead with this. There are cases where string interpolation with the Raw functions is OK, even required (e.g. interpolating a column/table name, which cannot be parameterized in SQL), so this would generate false positive warnings that would need to be suppressed. I still think this makes sense, but I'd like to make sure that everyone else does too. A couple quick notes: in EF 7 we introduced FromSql, which is a synonym for FromSqlInterpolated but without the long name. We did this to steer users towards using the safer, FormattableString variant by default - so the code fix should correct to that. Also, EF 7.0 introduced SqlQuery/SqlQueryRaw for querying scalars - we should do the same analysis and code fix for that too. (but again, better to wait for confirmation before spending more time on this) |
I already excluded constant interpolation from reported cases. We can add specific terms for other things, like
Several lines fix. Also smells like a good refactoring opportunity to me) |
Makes sense, and exempting nameof definitely sounds right. But the problem at the end of the day is that there are valid cases for interpolation of non-constants, e.g. when the table name is passed as some method parameter, or even read from a UI (and then properly sanitized!). Though again, I don't believe that should prevent us from warning. |
|
@roji A week has passed. Any updates? |
|
@DoctorKrolic sorry for the delay - I am currently abroad and so we haven't yet had a design discussion on this. It's definitely on my list and we should have an answer by the end of the week. |
|
@roji Was there a design discussion about this change? |
|
@DoctorKrolic yes - we just had a discussion on this in our design meeting, and there's consensus in the team that this is a good idea. We raised the possibility of reporting a similar warning for string cancatenation, but decided to defer that for now. So please feel free to continue working on this based on my comments above (please also rebase and make the build pass). I'll also try to give this a proper review in the coming days. |
This reverts commit 79346f2.
|
@roji Adressed your previous feedback, this is ready for a review pass. Also don't forget about better message suggestions if there are any, thanks! |
There was a problem hiding this comment.
Thanks @DoctorKrolic, this looks really good (and has good test coverage too).
| Debug.Assert(targetMethod.Name == "FromSqlRaw"); | ||
|
|
||
| // Correct `FromSqlRaw` is an extension method, therefore it must be static | ||
| if (!targetMethod.IsStatic) |
There was a problem hiding this comment.
Rather than all the verification steps below, why not just extract the method symbol from the DbSet type symbol (and cache it), and then just compare the invocation's target method against that? This should simplify the code considerably.
There was a problem hiding this comment.
Yes, it simpliffies things a lot, but I am not sure about the caching. I believe it is incorrect to hold IMethodSymbol and then compare it to another IMethodSymbol, that come from different Compilation object. LMK what kind of caching did you mean
src/EFCore.Analyzers/InterpolatedStringUsageInRawQueriesDiagnosticAnalyzer.cs
Show resolved
Hide resolved
src/EFCore.Analyzers/InterpolatedStringUsageInRawQueriesCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Analyzers/InterpolatedStringUsageInRawQueriesCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
test/EFCore.Analyzers.Tests/InterpolatedStringUsageInRawQueriesTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @DoctorKrolic, this looks really good (and has good test coverage too).
Co-authored-by: Shay Rojansky <roji@roji.org>
Co-authored-by: Shay Rojansky <roji@roji.org>
src/EFCore.Analyzers/InterpolatedStringUsageInRawQueriesDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Analyzers/InterpolatedStringUsageInRawQueriesDiagnosticAnalyzer.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for quality work and for the quick turnaround @DoctorKrolic! Looking forward to seeing more work from you!
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
DoctorKrolic
deleted the
interpolated-strings-in-raw-queries-analyzer-codefix
branch
Closes #30965
FromSqlRaw,ExecuteSqlRawandExecuteSqlRawAsyncmethodsAddedThis thing randomly stopped working for me, so reverted the changeEFCore.Analyzers.Deploymentproject, that represents a VS extension, that references analyzers project. This enables VS users to not only verify analyzers/code fixes in unit tests, but also in a VS instance as well. I am aware, that most (if not the whole) EF Team members use Rider, but I don't think this is gonna be an issue. If you accept this change, I will need infra help since this project should not be built in CI!