From 2d5bc9920333be513c012714f343911a386310e6 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Mon, 13 Apr 2020 17:22:06 +0200 Subject: [PATCH 01/10] Fixed: stackTrace:null in output when options.IncludeExceptionStackTraceInErrors = false --- .../Models/JsonApiDocuments/ErrorMeta.cs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/JsonApiDotNetCore/Models/JsonApiDocuments/ErrorMeta.cs b/src/JsonApiDotNetCore/Models/JsonApiDocuments/ErrorMeta.cs index f271924768..6d09bd627c 100644 --- a/src/JsonApiDotNetCore/Models/JsonApiDocuments/ErrorMeta.cs +++ b/src/JsonApiDotNetCore/Models/JsonApiDocuments/ErrorMeta.cs @@ -15,8 +15,15 @@ public sealed class ErrorMeta public void IncludeExceptionStackTrace(Exception exception) { - Data["StackTrace"] = exception?.Demystify().ToString() - .Split(new[] {"\n"}, int.MaxValue, StringSplitOptions.RemoveEmptyEntries); + if (exception == null) + { + Data.Remove("StackTrace"); + } + else + { + Data["StackTrace"] = exception.Demystify().ToString() + .Split(new[] { "\n" }, int.MaxValue, StringSplitOptions.RemoveEmptyEntries); + } } } } From fef483d8f38eb0ac68dd1467aad7d829e11a6fe5 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Mon, 13 Apr 2020 17:22:28 +0200 Subject: [PATCH 02/10] Fixed: crash when using obfuscated IDs --- src/JsonApiDotNetCore/Services/DefaultResourceService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/JsonApiDotNetCore/Services/DefaultResourceService.cs b/src/JsonApiDotNetCore/Services/DefaultResourceService.cs index e277f21d65..9df4450cfc 100644 --- a/src/JsonApiDotNetCore/Services/DefaultResourceService.cs +++ b/src/JsonApiDotNetCore/Services/DefaultResourceService.cs @@ -213,7 +213,7 @@ public virtual async Task UpdateAsync(TId id, TResource requestEntity } _repository.FlushFromCache(databaseEntity); - TResource afterEntity = await _repository.Get(databaseEntity.Id).FirstOrDefaultAsync(); + TResource afterEntity = await _repository.Get(id).FirstOrDefaultAsync(); _resourceChangeTracker.SetFinallyStoredAttributeValues(afterEntity); bool hasImplicitChanges = _resourceChangeTracker.HasImplicitChanges(); From 0baf63d0523749de56ff293c7f08a34e4f7c2dd7 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 14 May 2020 17:37:49 +0200 Subject: [PATCH 03/10] Added explanation on why 'id' is added to the attribute set --- src/JsonApiDotNetCore/Builders/ResourceGraphBuilder.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/JsonApiDotNetCore/Builders/ResourceGraphBuilder.cs b/src/JsonApiDotNetCore/Builders/ResourceGraphBuilder.cs index 7536b791eb..558db0ad0a 100644 --- a/src/JsonApiDotNetCore/Builders/ResourceGraphBuilder.cs +++ b/src/JsonApiDotNetCore/Builders/ResourceGraphBuilder.cs @@ -92,9 +92,9 @@ protected virtual List GetAttributes(Type entityType) { var attribute = (AttrAttribute)property.GetCustomAttribute(typeof(AttrAttribute)); - // TODO: investigate why this is added in the exposed attributes list - // because it is not really defined attribute considered from the json:api - // spec point of view. + // Although strictly not correct, 'id' is added to the list of attributes for convenience. + // For example, it enables to filter on id, without the need to special-case existing logic. + // And when using sparse fields, it silently adds 'id' to the set of attributes to retrieve. if (property.Name == nameof(Identifiable.Id) && attribute == null) { var idAttr = new AttrAttribute From 57ad72680561d7cf053182efc30893f258d9a190 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 14 May 2020 17:38:15 +0200 Subject: [PATCH 04/10] Removed TODO after fix was merged. --- .../RequestServices/DefaultResourceChangeTracker.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/JsonApiDotNetCore/RequestServices/DefaultResourceChangeTracker.cs b/src/JsonApiDotNetCore/RequestServices/DefaultResourceChangeTracker.cs index d55f521edd..f61a7a4e07 100644 --- a/src/JsonApiDotNetCore/RequestServices/DefaultResourceChangeTracker.cs +++ b/src/JsonApiDotNetCore/RequestServices/DefaultResourceChangeTracker.cs @@ -80,8 +80,7 @@ private IDictionary CreateAttributeDictionary(TResource resource foreach (var attribute in attributes) { object value = attribute.GetValue(resource); - // TODO: Remove explicit cast to JsonApiOptions after https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/687 has been fixed. - var json = JsonConvert.SerializeObject(value, ((JsonApiOptions) _options).SerializerSettings); + var json = JsonConvert.SerializeObject(value, _options.SerializerSettings); result.Add(attribute.PublicAttributeName, json); } From a7d160786f5f9fc6209e72a40ea1b098624b8fd3 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 14 May 2020 17:42:42 +0200 Subject: [PATCH 05/10] Looks like a left-over from entity-resource-separation, which no longer exists. --- .../Models/Annotation/AttrAttribute.cs | 41 ++++--------------- 1 file changed, 7 insertions(+), 34 deletions(-) diff --git a/src/JsonApiDotNetCore/Models/Annotation/AttrAttribute.cs b/src/JsonApiDotNetCore/Models/Annotation/AttrAttribute.cs index 25b25e8bf0..cd701f64eb 100644 --- a/src/JsonApiDotNetCore/Models/Annotation/AttrAttribute.cs +++ b/src/JsonApiDotNetCore/Models/Annotation/AttrAttribute.cs @@ -95,13 +95,12 @@ public object GetValue(object entity) throw new ArgumentNullException(nameof(entity)); } - var property = GetResourceProperty(entity); - if (property.GetMethod == null) + if (PropertyInfo.GetMethod == null) { - throw new InvalidOperationException($"Property '{property.DeclaringType?.Name}.{property.Name}' is write-only."); + throw new InvalidOperationException($"Property '{PropertyInfo.DeclaringType?.Name}.{PropertyInfo.Name}' is write-only."); } - return property.GetValue(entity); + return PropertyInfo.GetValue(entity); } /// @@ -114,40 +113,14 @@ public void SetValue(object entity, object newValue) throw new ArgumentNullException(nameof(entity)); } - var property = GetResourceProperty(entity); - if (property.SetMethod == null) + if (PropertyInfo.SetMethod == null) { throw new InvalidOperationException( - $"Property '{property.DeclaringType?.Name}.{property.Name}' is read-only."); + $"Property '{PropertyInfo.DeclaringType?.Name}.{PropertyInfo.Name}' is read-only."); } - var convertedValue = TypeHelper.ConvertType(newValue, property.PropertyType); - property.SetValue(entity, convertedValue); - } - - private PropertyInfo GetResourceProperty(object resource) - { - // There are some scenarios, especially ones where users are using a different - // data model than view model, where they may use a repository implementation - // that does not match the deserialized type. For now, we will continue to support - // this use case. - var targetType = resource.GetType(); - if (targetType != PropertyInfo.DeclaringType) - { - var propertyInfo = resource - .GetType() - .GetProperty(PropertyInfo.Name); - - return propertyInfo; - - // TODO: this should throw but will be a breaking change in some cases - //if (propertyInfo == null) - // throw new InvalidOperationException( - // $"'{targetType}' does not contain a member named '{InternalAttributeName}'." + - // $"There is also a mismatch in target types. Expected '{PropertyInfo.DeclaringType}' but instead received '{targetType}'."); - } - - return PropertyInfo; + var convertedValue = TypeHelper.ConvertType(newValue, PropertyInfo.PropertyType); + PropertyInfo.SetValue(entity, convertedValue); } /// From 61befc44988b81fb6fccddc261212669c640ef05 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 14 May 2020 17:51:56 +0200 Subject: [PATCH 06/10] Tracked at #757 --- src/JsonApiDotNetCore/QueryParameterServices/PageService.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/JsonApiDotNetCore/QueryParameterServices/PageService.cs b/src/JsonApiDotNetCore/QueryParameterServices/PageService.cs index ad3d625458..d1d51f0c7b 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/PageService.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/PageService.cs @@ -90,10 +90,6 @@ public virtual void Parse(string parameterName, StringValues parameterValue) { var number = ParsePageNumber(parameterValue, _options.MaximumPageNumber); - // TODO: It doesn't seem right that a negative paging value reverses the sort order. - // A better way would be to allow ?sort=- to indicate reversing results. - // Then a negative paging value, like -5, could mean: "5 pages back from the last page" - Backwards = number < 0; CurrentPage = Backwards ? -number : number; } From d710b4b16c792546e840b5014a9dcf838678d44d Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 14 May 2020 18:31:32 +0200 Subject: [PATCH 07/10] Added missing test for error case --- .../SparseFieldsService.cs | 6 ++-- .../SparseFieldsServiceTests.cs | 35 +++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs b/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs index 18f527e83e..bafa6976d4 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs @@ -130,11 +130,9 @@ private void RegisterRelatedResourceFields(IEnumerable fields, Relations var attr = relationProperty.Attributes.SingleOrDefault(a => a.Is(field)); if (attr == null) { - // TODO: Add unit test for this error, once the nesting limitation is removed and this code becomes reachable again. - throw new InvalidQueryStringParameterException(parameterName, - "Sparse field navigation path refers to an invalid related field.", - $"Related resource '{relationship.RightType.Name}' does not contain an attribute named '{field}'."); + "The specified field does not exist on the requested related resource.", + $"The field '{field}' does not exist on related resource '{relationship.PublicRelationshipName}' of type '{relationProperty.ResourceName}'."); } if (attr.PropertyInfo.SetMethod == null) diff --git a/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs b/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs index d7f17ed8a2..4a5158de98 100644 --- a/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs +++ b/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs @@ -7,6 +7,7 @@ using JsonApiDotNetCoreExample.Models; using Microsoft.Extensions.Primitives; using Xunit; +using Person = UnitTests.TestModels.Person; namespace UnitTests.QueryParameters { @@ -162,6 +163,40 @@ public void Parse_InvalidField_ThrowsJsonApiException() Assert.Equal("fields", exception.Error.Source.Parameter); } + [Fact] + public void Parse_InvalidRelatedField_ThrowsJsonApiException() + { + // Arrange + var idAttribute = new AttrAttribute("id") {PropertyInfo = typeof(Article).GetProperty(nameof(Article.Id))}; + + var query = new KeyValuePair("fields[author]", "invalid"); + + var resourceContext = new ResourceContext + { + ResourceName = "articles", + Attributes = new List {idAttribute}, + Relationships = new List + { + new HasOneAttribute("author") + { + PropertyInfo = typeof(Article).GetProperty(nameof(Article.Author)), + RightType = typeof(Person) + } + } + }; + + var service = GetService(resourceContext); + + // Act, assert + var exception = Assert.Throws(() => service.Parse(query.Key, query.Value)); + + Assert.Equal("fields[author]", exception.QueryParameterName); + Assert.Equal(HttpStatusCode.BadRequest, exception.Error.StatusCode); + Assert.Equal("The specified field does not exist on the requested related resource.", exception.Error.Title); + Assert.Equal("The field 'invalid' does not exist on related resource 'author' of type 'people'.", exception.Error.Detail); + Assert.Equal("fields[author]", exception.Error.Source.Parameter); + } + [Fact] public void Parse_LegacyNotation_ThrowsJsonApiException() { From 72549f18e9fc696735ffc09f26ca6fcc288bbde5 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 14 May 2020 18:32:35 +0200 Subject: [PATCH 08/10] Tracked at #734. --- src/JsonApiDotNetCore/QueryParameterServices/IncludeService.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/JsonApiDotNetCore/QueryParameterServices/IncludeService.cs b/src/JsonApiDotNetCore/QueryParameterServices/IncludeService.cs index a2f54d55f9..f00bd9680b 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/IncludeService.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/IncludeService.cs @@ -12,7 +12,6 @@ namespace JsonApiDotNetCore.Query { public class IncludeService : QueryParameterService, IIncludeService { - /// todo: use read-only lists. private readonly List> _includedChains; public IncludeService(IResourceGraph resourceGraph, ICurrentRequest currentRequest) : base(resourceGraph, currentRequest) From 4d6418289b53392730e2ff1df49e5aec0590ff1c Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 14 May 2020 18:34:53 +0200 Subject: [PATCH 09/10] Removed temporary workaround --- src/JsonApiDotNetCore/Data/DefaultResourceRepository.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/JsonApiDotNetCore/Data/DefaultResourceRepository.cs b/src/JsonApiDotNetCore/Data/DefaultResourceRepository.cs index 7f53bd2124..70d102245a 100644 --- a/src/JsonApiDotNetCore/Data/DefaultResourceRepository.cs +++ b/src/JsonApiDotNetCore/Data/DefaultResourceRepository.cs @@ -187,10 +187,6 @@ private void DetachRelationships(TResource entity) else { _context.Entry(value).State = EntityState.Detached; - - // temporary work around for https://github.com/aspnet/EntityFrameworkCore/issues/18621 - // as soon as ef core 3.1 lands we can get rid of this again. - _context.Entry(entity).State = EntityState.Detached; } } } From 64062825e3a77ccdeb463324452adf99cdb98ae4 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 14 May 2020 18:39:49 +0200 Subject: [PATCH 10/10] Tracked at #758 --- src/JsonApiDotNetCore/QueryParameterServices/FilterService.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/JsonApiDotNetCore/QueryParameterServices/FilterService.cs b/src/JsonApiDotNetCore/QueryParameterServices/FilterService.cs index 0e3c4f3b8d..44d809a1ee 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/FilterService.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/FilterService.cs @@ -74,7 +74,6 @@ private FilterQueryContext GetQueryContexts(FilterQuery query, string parameterN return queryContext; } - /// todo: this could be simplified a bunch private List GetFilterQueries(string parameterName, StringValues parameterValue) { // expected input = filter[id]=1 @@ -100,7 +99,6 @@ private List GetFilterQueries(string parameterName, StringValues pa return queries; } - /// todo: this could be simplified a bunch private (string operation, string value) ParseFilterOperation(string value) { if (value.Length < 3) @@ -117,7 +115,6 @@ private List GetFilterQueries(string parameterName, StringValues pa return (operation, value); } - /// todo: this could be simplified a bunch private string GetFilterOperation(string value) { var values = value.Split(QueryConstants.COLON);