Merge pull request #759 from bart-degreed/todos

Resolved most ToDo's in code

Author: Bart Koelman

src/JsonApiDotNetCore/Builders/ResourceGraphBuilder.cs

@@ -92,9 +92,9 @@ protected virtual List<AttrAttribute> 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

src/JsonApiDotNetCore/Data/DefaultResourceRepository.cs

@@ -200,10 +200,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;
                 }
             }
         }

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);
         }
 
         /// <summary>
@@ -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);
         }
 
         /// <summary>

src/JsonApiDotNetCore/QueryParameterServices/FilterService.cs

@@ -81,7 +81,6 @@ private FilterQueryContext GetQueryContexts(FilterQuery query, string parameterN
             return queryContext;
         }
 
-        /// todo: this could be simplified a bunch 
         private List<FilterQuery> GetFilterQueries(string parameterName, StringValues parameterValue)
         {
             // expected input = filter[id]=1
@@ -107,7 +106,6 @@ private List<FilterQuery> 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)
@@ -124,7 +122,6 @@ private List<FilterQuery> 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);

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<List<RelationshipAttribute>> _includedChains;
 
         public IncludeService(IResourceGraph resourceGraph, ICurrentRequest currentRequest) : base(resourceGraph, currentRequest)

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;
             }

src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs

@@ -130,11 +130,9 @@ private void RegisterRelatedResourceFields(IEnumerable<string> 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)

src/JsonApiDotNetCore/RequestServices/DefaultResourceChangeTracker.cs

@@ -80,8 +80,7 @@ private IDictionary<string, string> 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);
             }
 

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<string, StringValues>("fields[author]", "invalid");
+
+            var resourceContext = new ResourceContext
+            {
+                ResourceName = "articles",
+                Attributes = new List<AttrAttribute> {idAttribute},
+                Relationships = new List<RelationshipAttribute>
+                {
+                    new HasOneAttribute("author")
+                    {
+                        PropertyInfo = typeof(Article).GetProperty(nameof(Article.Author)),
+                        RightType = typeof(Person)
+                    }
+                }
+            };
+
+            var service = GetService(resourceContext);
+
+            // Act, assert
+            var exception = Assert.Throws<InvalidQueryStringParameterException>(() => 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()
         {