Skip to content

Asp.Net Core - Consider Changing Dependency from Microsoft.AspNetCore.Mvc to Microsoft.AspNetCore.Mvc.Core #66

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

Closed
AdamDotNet opened this issue Dec 9, 2016 · 7 comments
Assignees
Milestone

Comments

@AdamDotNet
Copy link

I was updating my Asp.Net Core app from 1.0.1 to 1.1.0, and while I was at it, I was able to add API versioning. At first I was taking a dependency on the NuGet package Microsoft.AspNetCore.Mvc, but decided to move to Microsoft.AspNetCore.Mvc.Core because I only use the "Web API" part of the Mvc package.

To my dismay, when I published my work, I still found Razor, TagHelpers and more in my publish output. I tracked it down to Microsoft.AspNetCore.Mvc.Versioning taking a dependency on Microsoft.AspNetCore.Mvc (>= 1.0.0). The result is having 1.1.0 files mixed with a bunch of 1.0.0 files (not good)!

Microsoft.AspNetCore.Mvc has a dependency on Microsoft.AspNetCore.Mvc.Core, so I assume it is safe for versioning to also take a dependency on Microsoft.AspNetCore.Mvc.Core and no one will break whether they reference the full meta package for Mvc or just the Core meta package. By depending on the Core meta package, "Web Api only" projects can get versioning and stay lean at publish time. I guess it depends on if your package truly needs bits out of the full Mvc meta package.

@AdamDotNet
Copy link
Author

I cloned the repository and took a stab at this. The only changes were project.json from the main .Net Core project:
"Microsoft.AspNetCore.Mvc": "1.0.0" => "Microsoft.AspNetCore.Mvc.Core": "1.0.0"

And the two unit test projects had some
IApplicationBuilder.AddMvc() => IApplicationBuilder.AddMvcCore()
class [TestController] : Controller => class [TestController] : ControllerBase

Everything compiled, but 34 tests of tests started failed because of getting NotAcceptable (406 I think) status codes back from the server instead of whatever code the test was expecting. I don't understand why at the moment.

@AdamDotNet
Copy link
Author

At this point, I've seen two results:

Take the following test:
_a_query_string_versioned_Controller_per_namespace._get_should_return_200

At one point, I saw that it returned 4 possible actions to select with the error message (notice all 4 are exactly the same):

Multiple actions matched. The following actions matched route data and had all constraints satisfied:

Microsoft.AspNetCore.Mvc.ByNamespace.Controllers.V1.AgreementsController.Get (Microsoft.AspNetCore.Mvc.Acceptance.Tests)
Microsoft.AspNetCore.Mvc.ByNamespace.Controllers.V1.AgreementsController.Get (Microsoft.AspNetCore.Mvc.Acceptance.Tests)
Microsoft.AspNetCore.Mvc.ByNamespace.Controllers.V1.AgreementsController.Get (Microsoft.AspNetCore.Mvc.Acceptance.Tests)
Microsoft.AspNetCore.Mvc.ByNamespace.Controllers.V1.AgreementsController.Get (Microsoft.AspNetCore.Mvc.Acceptance.Tests)

For now, I might leave that as a debugging oddity. The second result I've seen is perhaps what should have worked. See the class:

Microsoft.AspNetCore.Mvc.Versioning.ApiVersionActionSelector.SelectBestCandidate

The branch else if ( finalMatches.Count == 1 ) is hit and a single action descriptor is returned. After that, ReportApiVersionsAttribute does its thing, then the debugger reports I'm in external code, and suddenly the test fails with HTTP 406, Not Acceptable.

This is a head scratcher for me because by all means the code appeared to succeed.

@commonsensesoftware
Copy link
Collaborator

I'm definitely in support of the fewest number of dependencies. I'm trying to remember the details, but I seem to remember there was some reason why *.Mvc was needed over *.MvcCore. It's possible that things have changed in 1.1 from 1.0. It's definitely worth investigating.

Do you have the latest changes? There were a couple of flakey tests that took me a long time to figure out why. Ultimately, it was a mistake and bug on my part. That has since been resolved. The latest codebase should run the tests reliably for any changes you make.

I'll look at dropping the dependencies down a level in the next release. There's no sense in claiming the library needs more than it does.

@AdamDotNet
Copy link
Author

I'll try latest code again when I get settled back in after the holiday break and then report back.

@AdamDotNet
Copy link
Author

AdamDotNet commented Jan 9, 2017

I'm fairly novice to Git style source control... but I just did a fork of the Master branch to https://github.com/AdamDotNet/aspnet-api-versioning (note that I didn't check in my changes to my fork yet)

I then reapplied the changes to MvcCore.

The same result of the 34 unit tests failing still occurs.

@AdamDotNet
Copy link
Author

One thing I forgot to mention is I didn't have the exact same .Net Core SDK installed, so global.json was changed to

"sdk": { "version": "1.0.0-preview2-003131" }

This is just a slightly newer version of the .Net Core 1.0.0 SDK.

@commonsensesoftware
Copy link
Collaborator

A quick update on this issue. It appears that referencing only Microsoft.AspNetCore.Mvc.Core should be fine. Updating the references with in project.json model can be a pain. Due to transitive dependencies, a few things had to change for test projects. I'm looking forward to converting back to *.csproj when VS2017 is released as soon as I have time.

I've got a few items queued up. I'll include this change and it should go out in the 1.1.0 release. I'm hoping to have that done within the next week (maybe lesss).

commonsensesoftware pushed a commit that referenced this issue Mar 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants