Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Fail more gracefully when option collections cleared #4690

Closed
lammia07 opened this issue May 20, 2016 · 14 comments
Closed

Fail more gracefully when option collections cleared #4690

lammia07 opened this issue May 20, 2016 · 14 comments
Assignees

Comments

@lammia07
Copy link

lammia07 commented May 20, 2016

Hi.
When I try to call Html.TextBox on a decimal? Property in a View, with a null value, I get a NullReferenceException.

at Microsoft.AspNetCore.Mvc.DataAnnotations.Internal.NumericClientModelValidator.GetErrorMessage(ModelMetadata modelMetadata)

Can you please provide me with some help.

regards Michael

@Eilon
Copy link
Member

Eilon commented May 20, 2016

@dougbu can you take a look? Some issue with nullable decimals?

@dougbu
Copy link
Member

dougbu commented May 20, 2016

Sure

@javiercn
Copy link
Member

I gave a quick try to this yesterday, (just rendering a Product with a decimal? property set to null) and it didn't repro for me, I would test the scenario in which you add an error and render the view again

@javiercn
Copy link
Member

@lammia07 Can you provide the full call stack, or even better, a repro for this? What version are you using RC2?

@lammia07
Copy link
Author

Hi.
Yes I am using the rc2.
I hope thats okay. I've only got it in german.
The exception occures because the ModelMetadata.ModelBindingMessageProvider is null.

screenshot-localhost 5000 2016-05-23 16-50-21

@lammia07
Copy link
Author

Yes, I am using rc2.
The exception occurs, because the ViewData.ModelMetadata.ModelBindingMessageProvider is null.
Sry. I have the call stack only in german. I hope it helps.
screenshot-localhost 5000 2016-05-23 16-50-21

@Eilon Eilon added this to the 1.0.0 milestone May 24, 2016
@lammia07
Copy link
Author

Hi.
I have investigated the problem and I think I found the solution.
I have written an own IDisplayMetadataProvider for my DataAnnotations because I want to translate my DataAnnotation with my Database and not with a ResourceFile. Now the problem was calling in my StartupFIle MvcOptions.ModelMetadataDetailsProviders.Clear() and so all Providers were removed. Now i have only replaced the DataAnnotationsMetadataProvider and it works again perfectly.

Sry for bothering you.

Best regards Michael

@rynowak
Copy link
Member

rynowak commented May 25, 2016

Thanks for the update. If this is happening due to clearing something in the options we should give a better error message than just a nullref

@Eilon
Copy link
Member

Eilon commented May 25, 2016

If this is about failing more gracefully, I'm moving it to 1.0.1.

@Eilon Eilon modified the milestones: 1.0.1, 1.0.0 May 25, 2016
@dougbu
Copy link
Member

dougbu commented Jun 27, 2016

Went through all MVC-defined options and broke them in two groups. Will send a PR to improve failure modes for the first group.

Worth improving errors when null or empty
MvcOptions
// BodyModelBinder will always throw an UnsupportedContentTypeException.
options.InputFormatters.Clear();
// ModelBinderFactory will always throw an InvalidOperationException, Could not create binder.
options.ModelBinderProviders.Clear();
// ModelMetadata.ModelBindingMessageProvider left null, leading to an NRE. ModelMetadata.IsEnum and
// IsFlagsEnum never true. In-box e.g. [Bind] and DataAnnotations attributes have no impact. No
// excluded bindings or suppressed validations.
options.ModelMetadataDetailsProviders.Clear();
// CompositeModelValidatorProvider will never provide a validator. ValidationVisitor will mark all
// existing ModelState entries as valid.
options.ModelValidatorProviders.Clear();
// HTTP 406 in all ObjectResult cases.
options.OutputFormatters.Clear();
RazorViewEngineOptions
// DefaultRazorViewEngineFileProviderAccessor will not find or watch any files. RazorViewEngine
// will thus never match. InvalidOperationException with expected searched locations.
options.FileProviders.Clear();
MvcViewOptions
// CompositeViewEngine will never match. InvalidOperationException with empty searched locations.
options.ViewEngines.Clear();
Behaviour when null or empty fine as-is

Completely valid for collection to be empty. Or errors occur and are fairly obviously linked to the cleared collection / null value.

MvcOptions
// Dictionary is normally empty. But, if user code added a profile earlier, ResponseCacheAttribute
// will always throw an InvalidOperationException, Profile is not defined.
options.CacheProfiles.Clear();
// No-op. This collection is empty unless AddWebApiConventions() was called.
options.Conventions.Clear();
// UnsupportedContentTypeException not mapped to HTTP 415. TempData not saved or restored. If using
// Web API compat shim, HttpResponseException not mapped to an ObjectResult.
options.Filters.Clear();
// CompositeValueProvider will never provide a value, leading to model binding errors despite data.
options.ValueProviderFactories.Clear();
RazorViewEngineOptions
// No-op. This collection is normally empty.
options.AdditionalCompilationReferences.Clear();
// RazorViewEngine will always throw an ArgumentException, AreaViewLocationFormats cannot be empty.
options.AreaViewLocationFormats.Clear();
// No-op. This collection is empty unless AddLocalizationServices() was called.
options.ViewLocationExpanders.Clear();
// RazorViewEngine will always throw an ArgumentException, ViewLocationFormats cannot be empty.
options.ViewLocationFormats.Clear();
MvcViewOptions
// CompositeClientModelValidatorProvider will not add data- validation attributes. Quiet.
options.ClientModelValidatorProviders.Clear();
// ArgumentNullException when generating id attribute.
options.HtmlHelperOptions.IdAttributeDotReplacement = null;
// ArgumentException when creating TagBuilder.
options.HtmlHelperOptions.ValidationMessageElement = null;
// ArgumentException when creating TagBuilder.
options.HtmlHelperOptions.ValidationSummaryMessageElement = null;

@dougbu
Copy link
Member

dougbu commented Jun 27, 2016

OutputFormatters was listed in both groups. Fixed.

@dougbu dougbu added bug and removed investigate labels Jun 27, 2016
@dougbu dougbu changed the title NullReferenceException on decimal? Fail more gracefully when option collections cleared Jun 27, 2016
@dougbu
Copy link
Member

dougbu commented Jun 28, 2016

Was actually on the fence about MvcOptions.ModelValidatorProviders. After talking w/ @rynowak, decided not to bother changing the behaviour there. Note that behaviour is pretty similar to validating a model type that has no ValidationAttributes.

@dougbu
Copy link
Member

dougbu commented Jun 29, 2016

Left out checks for MvcOptions.ModelMetadataDetailsProviders. After cleaning up how ModelBindingMessageProvider is initialized, makes sense for this collection to be empty e.g. when using EmptyModelMetadataProvider.

New list of must-not-be-empty options properties:

MvcOptions
// BodyModelBinder will always throw an UnsupportedContentTypeException.
options.InputFormatters.Clear();
// ModelBinderFactory will always throw an InvalidOperationException, Could not create binder.
options.ModelBinderProviders.Clear();
// HTTP 406 in all ObjectResult cases.
options.OutputFormatters.Clear();
RazorViewEngineOptions
// DefaultRazorViewEngineFileProviderAccessor will not find or watch any files. RazorViewEngine
// will thus never match. InvalidOperationException with expected searched locations.
options.FileProviders.Clear();
MvcViewOptions
// CompositeViewEngine will never match. InvalidOperationException with empty searched locations.
options.ViewEngines.Clear();

dougbu added a commit that referenced this issue Jun 29, 2016
- #4690
- move `ModelBindingMessageProvider` init from `DefaultBindingMetadataProvider` to `DefaultModelMetadata`
 - in addition to avoiding error cases, this removes some boilerplate
- add specific errors to `BodyModelBinderProvider`, `CompositeViewEngine`,
  `DefaultRazorViewEngineFileProviderAccessor`, `ModelBinderFactory`, and `ObjectResultExecutor`
dougbu added a commit that referenced this issue Jul 1, 2016
- #4690
- move `ModelBindingMessageProvider` init from `DefaultBindingMetadataProvider` to `DefaultModelMetadata`
 - in addition to avoiding error cases, this removes some boilerplate
- add specific errors to `BodyModelBinderProvider`, `CompilerCache`, `CompositeViewEngine`, `ModelBinderFactory`,
  and `ObjectResultExecutor`
 - `DefaultRazorViewEngineFileProviderAccessor.FileProvider` now a `NullFileProvider` in empty case
@dougbu
Copy link
Member

dougbu commented Jul 1, 2016

42cea41

@dougbu dougbu closed this as completed Jul 1, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants