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

TryValidateModel is slow with large arrays #5887

Closed
CommonGuy opened this issue Mar 3, 2017 · 9 comments
Closed

TryValidateModel is slow with large arrays #5887

CommonGuy opened this issue Mar 3, 2017 · 9 comments
Assignees
Labels
2 - Working cost: M Will take from 3 - 5 days to complete enhancement Perf PRI: 1 - Required Must be handled in a reasonable time

Comments

@CommonGuy
Copy link

CommonGuy commented Mar 3, 2017

The code

TryValidateModel(model);

is very slow when i pass a model with huge arrays. My model looks roughly like this:

public class MyModel
{
    [Required]
    public string Id { get; set; }
    [Required]
    public byte[] Data { get; set; }
    [Required]
    public Dictionary<string, string> Metadata { get; set; }
}

If Data is only a few KB, everything is fine. However, if Data is filled with ~5MB, the validation takes nearly 2 seconds on my machine. I'm using .NET Core 1.1.

Of course, I could disable validation for MyModel and validate the properties myself... But I still think this is a bug, since I could not find any information on this topic (e.g. not to use validation on large arrays).

Note: Removing the [Required] from Data doesn't make any difference.

@kichalla
Copy link
Member

kichalla commented Mar 3, 2017

This bug was actually fixed recently and is available in nightly builds. ce53675
With this commit you can decorate the property with a ValidateNever attribute.

Meanwhile you could update the MVC options to exclude byte array from validation (this effects the entire app but I can't see why we do not do this by default. @dougbu ?)

options.ModelMetadataDetailsProviders.Add(new SuppressChildValidationMetadataProvider(typeof(byte[])));

@kichalla
Copy link
Member

kichalla commented Mar 3, 2017

Correcting my previous comment :-)...We don't know if this a bug yet but there is a new way (through ValidateNever) through which you can selectively disable validation.

@CommonGuy
Copy link
Author

Yes, I've read about ValidateNever and it looks cool. But I think of it as a workaround in this situation. I think it is dangerous to globally exclude byte arrays from validation...

By the way, validation is also very slow for lists with a few thousand entries. If I change my model to

public class MyModel
{
    [Required]
    public string Id { get; set; }
    [Required]
    public List<byte> Data { get; set; }
    [Required]
    public Dictionary<string, string> Metadata { get; set; }
}

the problem still exists.

@rynowak
Copy link
Member

rynowak commented Mar 3, 2017

@kichalla - using [ValidateNever] is a workaround - we need to be smart enough to do this by default when there's nothing to validate.

@Yves57
Copy link
Contributor

Yves57 commented Mar 5, 2017

Isn't it something like:

...
else if (IsEnumerableType && ElementType.GetTypeInfo().IsPrimitive)
{
     _validateChildren = false;
}
...

in DefaultModelMetadata.ValidateChildren?
Of course, with this kind of modification, the validation time decreases to nothing.

@dougbu
Copy link
Member

dougbu commented Mar 5, 2017

@Yves57 it would be similar to what you've got.

Should also confirm no validators are configured for ElementType and it might be better to do it in an IValidationMetadataProvider implementation. While unusual, primitive types can certainly have associated validators e.g., an IValidationMetadataProvider implementation could add [Range(0, 20] to the byte metadata.

Put another way, your suggestion is a good way to avoid recursion through the metadata to find validators. Still need to look for those validators.

@rynowak
Copy link
Member

rynowak commented Jul 17, 2017

Looking at this issue again, this is definitely something we should profile and try to improve.

Secondly, we need to hold on to this piece of design feedback: it should be possible for model binding to determine that a graph is a 'no-op'. Today we can't/don't due to that due to the flexibility to the validator providers have in MVC, and we're contributing a lot to global warming. We should make sure that any new validation system can determine when not to explore an object graph with greater vigour.

@Eilon Eilon added the Perf label Sep 1, 2017
@mkArtakMSFT mkArtakMSFT added this to the 2.2.0 milestone May 4, 2018
@mkArtakMSFT mkArtakMSFT modified the milestones: 2.2.0, 2.2.0-preview1 May 18, 2018
@mkArtakMSFT mkArtakMSFT added the PRI: 1 - Required Must be handled in a reasonable time label Jun 12, 2018
@mkArtakMSFT mkArtakMSFT assigned pranavkm and unassigned kichalla Aug 20, 2018
@mkArtakMSFT mkArtakMSFT modified the milestones: 2.2.0-preview2, 2.2.0-preview3, 2.2.0-rc1 Aug 20, 2018
@pranavkm
Copy link
Contributor

Moving this to 3.0

@pranavkm pranavkm modified the milestones: 2.2.0-preview3, 3.0.0 Sep 27, 2018
@pranavkm pranavkm removed this from the 3.0.0 milestone Oct 3, 2018
@pranavkm pranavkm added this to the 2.2.0-preview3 milestone Oct 3, 2018
@pranavkm
Copy link
Contributor

pranavkm commented Oct 3, 2018

Back to 2.2.0-preview3

@pranavkm pranavkm added bug enhancement cost: M Will take from 3 - 5 days to complete and removed needs design task bug labels Oct 11, 2018
pranavkm added a commit that referenced this issue Oct 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2 - Working cost: M Will take from 3 - 5 days to complete enhancement Perf PRI: 1 - Required Must be handled in a reasonable time
Projects
None yet
Development

No branches or pull requests

8 participants