* Allow null ViewData and TempData #3310

ryanbrandenburg · 2015-10-12T20:33:19Z

If you're manually constructing a ViewResult you shouldn't have to pass in a ViewData and TempData just for us. This change allows them to be left null, the behavior on a resulting page being that references to ViewData just don't put anything out and references to TempData cause a 500.

dnfclas · 2015-10-12T20:33:23Z

Hi @ryanbrandenburg, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

ryanbrandenburg · 2015-10-12T20:34:30Z

test/Microsoft.AspNet.Mvc.Core.Test/PhysicalFileResultTest.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

+using System;


This snuck in from another PR, I'll remove it when I rebase.

pranavkm · 2015-10-13T17:51:29Z

test/Microsoft.AspNet.Mvc.ViewFeatures.Test/ViewResultTest.cs

+        [Fact]
+        public async Task ExecuteResultAsync_ViewResultAllowNullViewDataAndTempData()
+        {
+            var context = GetActionContext();


we usually decorate tests with // Arrange, // Act and // Assert. Have a look at other tests in this class.

Eilon · 2015-10-13T17:54:00Z

Aside from the small comments,

pranavkm · 2015-10-13T17:56:12Z

test/Microsoft.AspNet.Mvc.ViewFeatures.Test/ViewResultTest.cs

+            Assert.Null(controller.ViewContext.TempData);
+        }
+
+        private class TestController


Move this test to ViewExecutorTest.cs. You shouldn't need a Controller to verify this.

Ah that's actually a good point. Can just new up the type and call Execute from within the test.

dougbu · 2015-10-14T06:04:43Z

⌚

rynowak · 2015-10-14T23:27:52Z

I suppose it may very well be better to have dummy ViewData/TempData for those cases.

^^^ - was thinking the same thought earlier about this bug. There's too much stuff that will maybe read ViewData to give a consistent experience around reporting when it's null and not without just requiring ViewExector to create one if needed.

ryanbrandenburg · 2015-10-14T23:42:22Z

Am I signed off then?

pranavkm · 2015-10-15T00:13:19Z

src/Microsoft.AspNet.Mvc.ViewFeatures/Rendering/ViewContext.cs

            {
-                throw new ArgumentNullException(nameof(htmlHelperOptions));
+                tempData = new TempDataDictionary(new HttpContextAccessor(), new SessionStateTempDataProvider());


The HttpContextAccessor might need to point to the current ActionContext.HttpContext

We should be service-locating ITempDataDictionary and IModelMetadataProvider here - this isn't a unit-test-only scenario.

ryanbrandenburg · 2015-10-16T18:46:34Z

@rynowak Do those changes satisfy the dependency injection problems?

rynowak · 2015-10-16T18:49:22Z

src/Microsoft.AspNet.Mvc.ViewFeatures/Rendering/ViewContext.cs

            }
+            var service = actionContext.HttpContext.RequestServices;


ryanbrandenburg · 2015-10-19T19:23:46Z

@rynowak is that better?

dougbu · 2015-10-19T19:29:31Z

src/Microsoft.AspNet.Mvc.ViewFeatures/Rendering/ViewContext.cs

            {
-                throw new ArgumentNullException(nameof(htmlHelperOptions));
+                throw new ArgumentNullException(nameof(tempData));


Restore these null checks in their original order. That nicely matched the parameter order.

dougbu · 2015-10-19T20:05:58Z

Wondering whether we should leave all but the main ViewContext constructor unchanged. That is, place the TempData and ViewData fallbacks in that constructor. May need minor ViewComponentResult and ViewExecutor updates to remove null checks. Then the fallback logic would then be in just one place.

dougbu · 2015-10-19T20:06:03Z

⌚

rynowak · 2015-10-19T20:12:49Z

test/Microsoft.AspNet.Mvc.ViewFeatures.Test/ViewFeatures/ViewExecutorTest.cs

+        public async Task ExecuteAsync_ViewResultAllowNull()
+        {
+            bool tempDataNull = false;
+            bool viewDataNull = false;


rynowak · 2015-10-19T20:13:36Z

⌚

ryanbrandenburg · 2015-10-19T22:03:21Z

Doug, about consolidating into the ViewContext, @rynowak is of the opinion, and he convinced me, that contexts should be dumb bags of objects that don't have any logic in them. Your suggestion would certainly simplify things a bit, but at the expense of having the Context violate its dumbness.

ryanbrandenburg · 2015-10-20T00:26:12Z

I believe I've addressed everything now, is there a ship it?

rynowak · 2015-10-20T00:44:24Z

dougbu · 2015-10-20T01:06:54Z

src/Microsoft.AspNet.Mvc.ViewFeatures/Rendering/ViewContext.cs

@@ -6,6 +6,7 @@
 using Microsoft.AspNet.Mvc.ModelBinding;
 using Microsoft.AspNet.Mvc.ViewEngines;
 using Microsoft.AspNet.Mvc.ViewFeatures;
+using Microsoft.Extensions.DependencyInjection;


Minor but you don't need this anymore. That is, revert everything in this file.

dougbu · 2015-10-20T01:17:42Z

once the CI builds start working. Problem looks like something propagating through our builds and not at Coherence yet. But it could hide something real.

C:\projects\mvc\test\WebSites\FiltersWebSite\Controllers\AuthorizeUserController.cs(31,10): DNX,Version=v4.5.1 error CS0104: 'AllowAnonymous' is an ambiguous reference between 'Microsoft.AspNet.Mvc.AllowAnonymousAttribute' and 'Microsoft.AspNet.Authorization.AllowAnonymousAttribute'

dnfclas added the cla-already-signed label Oct 12, 2015

ryanbrandenburg reviewed Oct 12, 2015
View reviewed changes

ryanbrandenburg force-pushed the rybrande/ViewResultNulls branch from 9067ce7 to b6ae7f3 Compare October 12, 2015 20:51

pranavkm reviewed Oct 13, 2015
View reviewed changes

pranavkm reviewed Oct 15, 2015
View reviewed changes

ryanbrandenburg force-pushed the rybrande/ViewResultNulls branch 3 times, most recently from a4f1507 to ca69d76 Compare October 16, 2015 17:26

rynowak reviewed Oct 16, 2015
View reviewed changes

src/Microsoft.AspNet.Mvc.ViewFeatures/Rendering/ViewContext.cs

}

var service = actionContext.HttpContext.RequestServices;

Copy link

Member

rynowak Oct 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

services

ryanbrandenburg force-pushed the rybrande/ViewResultNulls branch 2 times, most recently from bea9fea to 3e2ec3d Compare October 16, 2015 20:38

dougbu reviewed Oct 19, 2015
View reviewed changes

rynowak reviewed Oct 19, 2015
View reviewed changes

dougbu reviewed Oct 20, 2015
View reviewed changes

ryanbrandenburg force-pushed the rybrande/ViewResultNulls branch 6 times, most recently from f94ae7f to 8565b2f Compare October 23, 2015 00:11

* Allow null ViewData and TempData

0b8fe87

ryanbrandenburg force-pushed the rybrande/ViewResultNulls branch from 8565b2f to 0b8fe87 Compare October 23, 2015 00:28

ryanbrandenburg merged commit 0b8fe87 into dev Oct 23, 2015

ryanbrandenburg mentioned this pull request Oct 23, 2015

ArgumentNullException when returning empty ViewResult #3284

Closed

ryanbrandenburg deleted the rybrande/ViewResultNulls branch October 30, 2015 22:15

dougbu mentioned this pull request Apr 1, 2016

ViewExecutor.ExecuteAsync() should throw if passed viewData: null #4395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

* Allow null ViewData and TempData #3310

* Allow null ViewData and TempData #3310

ryanbrandenburg commented Oct 12, 2015

dnfclas commented Oct 12, 2015

ryanbrandenburg Oct 12, 2015

pranavkm Oct 13, 2015

Eilon commented Oct 13, 2015

pranavkm Oct 13, 2015

Eilon Oct 13, 2015

dougbu commented Oct 14, 2015

rynowak commented Oct 14, 2015

ryanbrandenburg commented Oct 14, 2015

pranavkm Oct 15, 2015

rynowak Oct 15, 2015

ryanbrandenburg commented Oct 16, 2015

rynowak Oct 16, 2015

ryanbrandenburg commented Oct 19, 2015

dougbu Oct 19, 2015

dougbu commented Oct 19, 2015

dougbu commented Oct 19, 2015

rynowak Oct 19, 2015

rynowak commented Oct 19, 2015

ryanbrandenburg commented Oct 19, 2015

ryanbrandenburg commented Oct 20, 2015

rynowak commented Oct 20, 2015

dougbu Oct 20, 2015

dougbu commented Oct 20, 2015

* Allow null ViewData and TempData #3310

* Allow null ViewData and TempData #3310

Conversation

ryanbrandenburg commented Oct 12, 2015

dnfclas commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eilon commented Oct 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Oct 14, 2015

rynowak commented Oct 14, 2015

ryanbrandenburg commented Oct 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg commented Oct 16, 2015

Choose a reason for hiding this comment

ryanbrandenburg commented Oct 19, 2015

Choose a reason for hiding this comment

dougbu commented Oct 19, 2015

dougbu commented Oct 19, 2015

Choose a reason for hiding this comment

rynowak commented Oct 19, 2015

ryanbrandenburg commented Oct 19, 2015

ryanbrandenburg commented Oct 20, 2015

rynowak commented Oct 20, 2015

Choose a reason for hiding this comment

dougbu commented Oct 20, 2015