-
-
Notifications
You must be signed in to change notification settings - Fork 158
Return 404 or 200 on error/non existent resource #636
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.
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
Changes from all commits
8524da6
94c1c6a
00ac030
b499d9c
a6238e8
d12d1c2
5b40e7e
63c9e6e
3d051d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ | |
using System.Text; | ||
using System.Threading.Tasks; | ||
using JsonApiDotNetCore.Internal; | ||
using JsonApiDotNetCore.Internal.Contracts; | ||
using JsonApiDotNetCore.Managers.Contracts; | ||
using JsonApiDotNetCore.Serialization.Server; | ||
using Microsoft.AspNetCore.Mvc; | ||
using Microsoft.AspNetCore.Mvc.Formatters; | ||
|
@@ -18,52 +20,70 @@ namespace JsonApiDotNetCore.Formatters | |
public class JsonApiWriter : IJsonApiWriter | ||
{ | ||
private readonly ILogger<JsonApiWriter> _logger; | ||
private readonly ICurrentRequest _currentRequest; | ||
private readonly IJsonApiSerializer _serializer; | ||
|
||
public JsonApiWriter(IJsonApiSerializer serializer, | ||
ILoggerFactory loggerFactory) | ||
ILoggerFactory loggerFactory, | ||
ICurrentRequest currentRequest) | ||
{ | ||
_currentRequest = currentRequest; | ||
_serializer = serializer; | ||
_logger = loggerFactory.CreateLogger<JsonApiWriter>(); | ||
} | ||
|
||
public async Task WriteAsync(OutputFormatterWriteContext context) | ||
{ | ||
if (context == null) | ||
{ | ||
throw new ArgumentNullException(nameof(context)); | ||
} | ||
|
||
var response = context.HttpContext.Response; | ||
using var writer = context.WriterFactory(response.Body, Encoding.UTF8); | ||
string responseContent; | ||
|
||
if (_serializer == null) | ||
if (response.StatusCode == 404) | ||
{ | ||
responseContent = JsonConvert.SerializeObject(context.Object); | ||
var requestedModel = _currentRequest.GetRequestResource(); | ||
var errors = new ErrorCollection(); | ||
errors.Add(new Error(404, $"The resource with type '{requestedModel.ResourceName}' and id '{_currentRequest.BaseId}' could not be found")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The resource of type... |
||
responseContent = _serializer.Serialize(errors); | ||
response.StatusCode = 404; | ||
} | ||
else | ||
{ | ||
response.ContentType = Constants.ContentType; | ||
try | ||
if (_serializer == null) | ||
{ | ||
if (context.Object is ProblemDetails pd) | ||
responseContent = JsonConvert.SerializeObject(context.Object); | ||
} | ||
else | ||
{ | ||
response.ContentType = Constants.ContentType; | ||
try | ||
{ | ||
if (context.Object is ProblemDetails pd) | ||
{ | ||
var errors = new ErrorCollection(); | ||
errors.Add(new Error(pd.Status.Value, pd.Title, pd.Detail)); | ||
responseContent = _serializer.Serialize(errors); | ||
} | ||
else | ||
{ | ||
responseContent = _serializer.Serialize(context.Object); | ||
} | ||
} | ||
catch (Exception e) | ||
{ | ||
_logger?.LogError(new EventId(), e, "An error ocurred while formatting the response"); | ||
var errors = new ErrorCollection(); | ||
errors.Add(new Error(pd.Status.Value, pd.Title, pd.Detail)); | ||
errors.Add(new Error(500, e.Message, ErrorMeta.FromException(e))); | ||
responseContent = _serializer.Serialize(errors); | ||
} else | ||
{ | ||
responseContent = _serializer.Serialize(context.Object); | ||
response.StatusCode = 500; | ||
} | ||
} | ||
catch (Exception e) | ||
{ | ||
_logger?.LogError(new EventId(), e, "An error ocurred while formatting the response"); | ||
var errors = new ErrorCollection(); | ||
errors.Add(new Error(500, e.Message, ErrorMeta.FromException(e))); | ||
responseContent = _serializer.Serialize(errors); | ||
response.StatusCode = 500; | ||
} | ||
} | ||
|
||
await writer.WriteAsync(responseContent); | ||
await writer.FlushAsync(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
using System; | ||
|
||
namespace JsonApiDotNetCore.Internal | ||
{ | ||
public class ResourceNotFoundException : Exception | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This type appears to be unused and can be removed. |
||
{ | ||
private readonly ErrorCollection _errors = new ErrorCollection(); | ||
|
||
public ResourceNotFoundException() | ||
{ } | ||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,9 @@ | ||
using JsonApiDotNetCore.Internal; | ||
using JsonApiDotNetCore.Internal; | ||
using JsonApiDotNetCore.Managers.Contracts; | ||
using Microsoft.AspNetCore.Mvc; | ||
using Microsoft.AspNetCore.Mvc.Filters; | ||
using Microsoft.Extensions.Logging; | ||
using System.Collections.Generic; | ||
|
||
namespace JsonApiDotNetCore.Middleware | ||
{ | ||
|
@@ -10,19 +12,19 @@ namespace JsonApiDotNetCore.Middleware | |
/// </summary> | ||
public class DefaultExceptionFilter : ActionFilterAttribute, IExceptionFilter | ||
{ | ||
private readonly ICurrentRequest _currentRequest; | ||
private readonly ILogger _logger; | ||
|
||
public DefaultExceptionFilter(ILoggerFactory loggerFactory) | ||
public DefaultExceptionFilter(ILoggerFactory loggerFactory, ICurrentRequest currentRequest) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The private field that is assigned from this parameter is never used. |
||
{ | ||
_currentRequest = currentRequest; | ||
_logger = loggerFactory.CreateLogger<DefaultExceptionFilter>(); | ||
} | ||
|
||
public void OnException(ExceptionContext context) | ||
{ | ||
_logger?.LogError(new EventId(), context.Exception, "An unhandled exception occurred during the request"); | ||
|
||
var jsonApiException = JsonApiExceptionFactory.GetException(context.Exception); | ||
|
||
var error = jsonApiException.GetError(); | ||
var result = new ObjectResult(error) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,6 +110,10 @@ public ResourceLinks GetResourceLinks(string resourceName, string id) | |
/// <inheritdoc/> | ||
public RelationshipLinks GetRelationshipLinks(RelationshipAttribute relationship, IIdentifiable parent) | ||
{ | ||
if(parent == null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there should be a space between the |
||
{ | ||
return null; | ||
} | ||
var parentResourceContext = _provider.GetResourceContext(parent.GetType()); | ||
var childNavigation = relationship.PublicRelationshipName; | ||
RelationshipLinks links = null; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
using JsonApiDotNetCore.Internal.Contracts; | ||
using JsonApiDotNetCore.Query; | ||
using JsonApiDotNetCore.Extensions; | ||
using System.Collections; | ||
|
||
namespace JsonApiDotNetCore.Services | ||
{ | ||
|
@@ -124,6 +125,7 @@ public virtual async Task<TResource> GetAsync(TId id) | |
return entity; | ||
} | ||
|
||
|
||
// triggered by GET /articles/1/relationships/{relationshipName} | ||
public virtual async Task<TResource> GetRelationshipsAsync(TId id, string relationshipName) | ||
{ | ||
|
@@ -135,15 +137,36 @@ public virtual async Task<TResource> GetRelationshipsAsync(TId id, string relati | |
// TODO: it would be better if we could distinguish whether or not the relationship was not found, | ||
// vs the relationship not being set on the instance of T | ||
|
||
var entityQuery = ApplyInclude(_repository.Get(id), chainPrefix: new List<RelationshipAttribute> { relationship }); | ||
var baseQuery = _repository.Get(id); | ||
var entityQuery = ApplyInclude(baseQuery, chainPrefix: new List<RelationshipAttribute> { relationship }); | ||
|
||
var entity = await _repository.FirstOrDefaultAsync(entityQuery); | ||
if (entity == null) | ||
|
||
|
||
// lol | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unclear comment |
||
var relationshipValue = typeof(TResource).GetProperty(relationship.InternalRelationshipName).GetValue(entity) ; | ||
var relEmpty = relationshipValue == null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused variable |
||
|
||
if(relationshipValue == null) | ||
{ | ||
/// TODO: this does not make sense. If the **parent** entity is not found, this error is thrown? | ||
/// this error should be thrown when the relationship is not found. | ||
throw new JsonApiException(404, $"Relationship '{relationshipName}' not found."); | ||
return null; | ||
} | ||
var listCast = (IList) relationshipValue; | ||
if(listCast != null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Expression is always true. And should test for |
||
{ | ||
if(listCast.Count == 0) | ||
{ | ||
return null; | ||
} | ||
} | ||
|
||
//if (entity == null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this block commented out? Should it be removed instead? |
||
//{ | ||
// /// TODO: this does not make sense. If the **parent** entity is not found, this error is thrown? | ||
// /// this error should be thrown when the relationship is not found. | ||
// throw new JsonApiException(404, $"Relationship '{relationshipName}' not found."); | ||
//} | ||
|
||
if (!IsNull(_hookExecutor, entity)) | ||
{ // AfterRead and OnReturn resource hook execution. | ||
_hookExecutor.AfterRead(AsList(entity), ResourcePipeline.GetRelationship); | ||
|
@@ -339,7 +362,9 @@ private RelationshipAttribute GetRelationship(string relationshipName) | |
{ | ||
var relationship = _currentRequestResource.Relationships.Single(r => r.Is(relationshipName)); | ||
if (relationship == null) | ||
{ | ||
throw new JsonApiException(422, $"Relationship '{relationshipName}' does not exist on resource '{typeof(TResource)}'."); | ||
} | ||
return relationship; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is duplicated in bothtrue
andfalse
blocks of the outerif
statement. I believe it can be moved to the top, outside theif
clauses, so it appears only once.They are actually different: Relationship vs Relationships