Skip to content

Add an analyzer that complains when using using model binding attributes with a minimal action delegate (#35359) #35480

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

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Aug 19, 2021

  • Complain when using MVC binding attributes with minimal action

Backport of #35359

Customer Impact

Easier to diagnose issues when using minimal APIs incorrectly, via an analyzer.

Testing

Automated tests for both positive and negative cases.

Risk

Low

…tes with a minimal action delegate (#35359)

* [x] Complain when using MVC binding attributes with minimal action

Fixes #35088
@davidfowl davidfowl added api-approved API was approved in API review, it can be implemented and removed api-approved API was approved in API review, it can be implemented labels Aug 19, 2021
@davidfowl
Copy link
Member

Is the risk really low since we're doing this ship with the SDK thing? How well is this tested?

@pranavkm
Copy link
Contributor

I would say this is medium risk. There is a new servicing burden since we haven't really understood what it takes to service analyzers in the targeting pack. That said, we already have one in there (the logging source generator), so there's some sunk cost fallacy here.

@davidfowl
Copy link
Member

How well is this change tested though?

@pranavkm
Copy link
Contributor

We have automated tests, and that gives us code coverage for the implementation. Unfortunately the end-to-end scenarios require a build of the installer with the changes. I verified things by manually patching things in, but that does leave much to be desired. An actual build with all things in place isn't available as yet.

If we're feeling iffy about it, we could punt it until rc2 and give it a little more time to bake.

@davidfowl
Copy link
Member

I rather do it in RC1 than RC2 but I I want to make sure we aren't flowing things all the way to the installer to find basic problems. We should spend time verifying (as hacky as we need to) to make sure we're not missing basic things.

@pranavkm
Copy link
Contributor

In that sense - we have reasonable coverage outside of a legit installer build.

Like I mentioned earlier, I was able to manually verify by extracting the targeting pack over an existing SDK. Our helix tests also do something to this effect (by updating the contents of the locally installed .dotnet) and we discovered a packaging issue because of this.

@davidfowl davidfowl added the Servicing-approved Shiproom has approved the issue label Aug 19, 2021
@davidfowl
Copy link
Member

Approved. I await the outcome of the dependency flow.

@pranavkm
Copy link
Contributor

@wtgodbe could you merge this please? Thanks!

@wtgodbe wtgodbe merged commit a1bbdc4 into release/6.0-rc1 Aug 19, 2021
@wtgodbe wtgodbe deleted the brecon/bpanalyzer branch August 19, 2021 20:40
@ghost ghost added this to the 6.0-rc1 milestone Aug 19, 2021
@pranavkm
Copy link
Contributor

Circling back - the analyzer is available as part of the 6.0-rc1 installer and appears to be working great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants