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 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; } } } 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); } /// 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); 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) 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; } 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/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); } 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() {