Skip to content

Validations Send source.pointer #371

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Aug 8, 2018
Merged

Validations Send source.pointer #371

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a050acd
feat: Error should accept objects for source
rtablada Aug 7, 2018
79fb285
feat: add 'GetPublicAttributeName' to ContextGraph
rtablada Aug 7, 2018
efa4aa7
Merge branch 'rt/get-context-property' into rt/validations-422
rtablada Aug 7, 2018
30c531d
wip: new 422 validation errors with source.pointer
rtablada Aug 7, 2018
0e17059
feat: validations send source.pointer
rtablada Aug 7, 2018
b841433
fix: handle 'GetPublicAttributeName' null and mock for tests
rtablada Aug 7, 2018
524232e
feat: throw 422 status code when validation fails
rtablada Aug 8, 2018
7e22eeb
feat: obsolete method ConvertToErrorCollection
rtablada Aug 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/JsonApiDotNetCore/Controllers/BaseJsonApiController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

namespace JsonApiDotNetCore.Controllers
{
public class BaseJsonApiController<T>
public class BaseJsonApiController<T>
: BaseJsonApiController<T, int>
where T : class, IIdentifiable<int>
{
Expand Down Expand Up @@ -47,7 +47,7 @@ public class BaseJsonApiController<T, TId>
private readonly ICreateService<T, TId> _create;
private readonly IUpdateService<T, TId> _update;
private readonly IUpdateRelationshipService<T, TId> _updateRelationships;
private readonly IDeleteService<T, TId> _delete;
private readonly IDeleteService<T, TId> _delete;
private readonly IJsonApiContext _jsonApiContext;

public BaseJsonApiController(
Expand Down Expand Up @@ -156,7 +156,7 @@ public virtual async Task<IActionResult> PostAsync([FromBody] T entity)
return Forbidden();

if (_jsonApiContext.Options.ValidateModelState && !ModelState.IsValid)
return BadRequest(ModelState.ConvertToErrorCollection());
return BadRequest(ModelState.ConvertToErrorCollection<T>(_jsonApiContext.ContextGraph));

entity = await _create.CreateAsync(entity);

Expand All @@ -170,7 +170,7 @@ public virtual async Task<IActionResult> PatchAsync(TId id, [FromBody] T entity)
if (entity == null)
return UnprocessableEntity();
if (_jsonApiContext.Options.ValidateModelState && !ModelState.IsValid)
return BadRequest(ModelState.ConvertToErrorCollection());
return BadRequest(ModelState.ConvertToErrorCollection<T>(_jsonApiContext.ContextGraph));

var updatedEntity = await _update.UpdateAsync(id, entity);

Expand Down
13 changes: 11 additions & 2 deletions src/JsonApiDotNetCore/Extensions/ModelStateExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace JsonApiDotNetCore.Extensions
{
public static class ModelStateExtensions
{
public static ErrorCollection ConvertToErrorCollection(this ModelStateDictionary modelState)
public static ErrorCollection ConvertToErrorCollection<T>(this ModelStateDictionary modelState, IContextGraph contextGraph)
Copy link
Contributor

@jaredcnance jaredcnance Aug 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So, that we don't have to release this in a major version, let's leave the original implementation and deprecate it using the [Obsolete("Use the generic overload instead")]. This prevents us from breaking anyone that is calling this API directly.

Also, I've recently been leaning towards the idea of depending on the static ContextGraph.Instance rather than injecting everywhere. I haven't found many reasons the ContextGraph should change after app start. Thoughts? disregard, that's going to be too big of an issue to bite off right now (i.e. there will be broken tests)

Copy link
Contributor Author

@rtablada rtablada Aug 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 for ContextGraph.Instance I wonder if that's safe given possible different versions of the API running at once with different context graphs?

If ContextGraph.Instance is used other places that could still be an issue there, but IDK if we'd want to tie this down too?

{
ErrorCollection collection = new ErrorCollection();
foreach (var entry in modelState)
Expand All @@ -16,10 +16,19 @@ public static ErrorCollection ConvertToErrorCollection(this ModelStateDictionary

foreach (var modelError in entry.Value.Errors)
{
var attrName =contextGraph.GetPublicAttributeName<T>(entry.Key);
Copy link
Contributor

Choose a reason for hiding this comment

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

let's do this lookup outside this loop since entry.Key doesn't change...results in unnecessary enumerations of the graph

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


if (modelError.Exception is JsonApiException jex)
collection.Errors.AddRange(jex.GetError().Errors);
else
collection.Errors.Add(new Error(400, entry.Key, modelError.ErrorMessage, modelError.Exception != null ? ErrorMeta.FromException(modelError.Exception) : null));
collection.Errors.Add(new Error(
status: 422,
title: entry.Key,
detail: modelError.ErrorMessage,
meta: modelError.Exception != null ? ErrorMeta.FromException(modelError.Exception) : null,
source: new {
pointer = $"/data/attributes/{attrName}"
}));
}
}

Expand Down
18 changes: 16 additions & 2 deletions src/JsonApiDotNetCore/Internal/ContextGraph.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace JsonApiDotNetCore.Internal
public interface IContextGraph
{
/// <summary>
/// Gets the value of the navigation property, defined by the relationshipName,
/// Gets the value of the navigation property, defined by the relationshipName,
/// on the provided instance.
/// </summary>
/// <param name="resource">The resource instance</param>
Expand Down Expand Up @@ -42,6 +42,12 @@ public interface IContextGraph
/// </summary>
ContextEntity GetContextEntity(Type entityType);

/// <summary>
/// Get the public attribute name for a type based on the internal attribute name.
/// </summary>
/// <param name="internalAttributeName">The internal attribute name for a <see cref="Attr" />.</param>
string GetPublicAttributeName<TParent>(string internalAttributeName);

/// <summary>
/// Was built against an EntityFrameworkCore DbContext ?
/// </summary>
Expand Down Expand Up @@ -111,5 +117,13 @@ public string GetRelationshipName<TParent>(string relationshipName)
.SingleOrDefault(r => r.Is(relationshipName))
?.InternalRelationshipName;
}
}

public string GetPublicAttributeName<TParent>(string internalAttributeName)
{
return GetContextEntity(typeof(TParent))
.Attributes
.Single(a => a.InternalAttributeName == internalAttributeName)
.PublicAttributeName;
}
}
}
20 changes: 10 additions & 10 deletions src/JsonApiDotNetCore/Internal/Error.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@ public class Error
{
public Error()
{ }

[Obsolete("Use Error constructors with int typed status")]
public Error(string status, string title, ErrorMeta meta = null, string source = null)
public Error(string status, string title, ErrorMeta meta = null, object source = null)
{
Status = status;
Title = title;
Meta = meta;
Source = source;
}

public Error(int status, string title, ErrorMeta meta = null, string source = null)
public Error(int status, string title, ErrorMeta meta = null, object source = null)
{
Status = status.ToString();
Title = title;
Expand All @@ -28,7 +28,7 @@ public Error(int status, string title, ErrorMeta meta = null, string source = nu
}

[Obsolete("Use Error constructors with int typed status")]
public Error(string status, string title, string detail, ErrorMeta meta = null, string source = null)
public Error(string status, string title, string detail, ErrorMeta meta = null, object source = null)
{
Status = status;
Title = title;
Expand All @@ -37,29 +37,29 @@ public Error(string status, string title, string detail, ErrorMeta meta = null,
Source = source;
}

public Error(int status, string title, string detail, ErrorMeta meta = null, string source = null)
public Error(int status, string title, string detail, ErrorMeta meta = null, object source = null)
{
Status = status.ToString();
Title = title;
Detail = detail;
Meta = meta;
Source = source;
}

[JsonProperty("title")]
public string Title { get; set; }

[JsonProperty("detail")]
public string Detail { get; set; }

[JsonProperty("status")]
public string Status { get; set; }

[JsonIgnore]
public int StatusCode => int.Parse(Status);

[JsonProperty("source")]
public string Source { get; set; }
public object Source { get; set; }

[JsonProperty("meta")]
public ErrorMeta Meta { get; set; }
Expand All @@ -73,8 +73,8 @@ public class ErrorMeta
[JsonProperty("stackTrace")]
public string[] StackTrace { get; set; }

public static ErrorMeta FromException(Exception e)
=> new ErrorMeta {
public static ErrorMeta FromException(Exception e)
=> new ErrorMeta {
StackTrace = e.Demystify().ToString().Split(new[] { "\n"}, int.MaxValue, StringSplitOptions.RemoveEmptyEntries)
};
}
Expand Down