Feature/#252 valid modelstate #316
Conversation
…otNetCore into feature/#252_valid_modelstate
…into feature/#252_valid_modelstate
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <VersionPrefix>2.3.2</VersionPrefix> | ||
| <VersionPrefix>2.3.3</VersionPrefix> |
There was a problem hiding this comment.
Wondering if this should be a major version bump. It could break existing apps if they have annotations but aren't currently checking them. We could release this as a patch version behind a feature flag (e.g. options.ValidateModelState) with a default disabled value. Then in the next major release enable it by default. What do you think?
There was a problem hiding this comment.
You could argue that if they have annotations already on their entities, then its already invalid and it's now being flagged. However, the feature flag route makes sense for a minor release :-D
Made the changes
|
|
||
| if (!_jsonApiContext.Options.AllowClientGeneratedIds && !string.IsNullOrEmpty(entity.StringId)) | ||
| return Forbidden(); | ||
| if (!ModelState.IsValid) return BadRequest(ModelState.ConvertToErrorCollection()); |
There was a problem hiding this comment.
nit: return on newline
| ErrorCollection errors = new ErrorCollection(); | ||
| foreach (var entry in modelState) | ||
| { | ||
| if (!entry.Value.Errors.Any()) continue; |
There was a problem hiding this comment.
nit: continue on newline
jaredcnance
left a comment
jaredcnance
left a comment
There was a problem hiding this comment.
Thanks! Just some minor comments on style (need a xplat style checker 🤔) and a note on versioning.
Feature/#252 valid modelstate
2 participants
Closes #252
FEATURE
Addressing Breaking Change - As there are no implicit Validation providers in MVC core, you need to explicitly add validation to the pipeline e.g. services.AddMvcCore().AddDataAnnotations()
Without this ModelState should always be valid.