From b02445c6e92bfc509abe705f120a9708d4d64a90 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Thu, 17 Oct 2019 18:14:41 +0200 Subject: [PATCH 1/4] fix: #583 --- .../SparseFieldsService.cs | 78 ++++++++++++------- .../Acceptance/Spec/SparseFieldSetTests.cs | 32 +++++++- .../SparseFieldsServiceTests.cs | 27 ++++++- 3 files changed, 109 insertions(+), 28 deletions(-) diff --git a/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs b/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs index e6518bc7e2..f9ae4012ca 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs @@ -40,40 +40,66 @@ public List Get(RelationshipAttribute relationship = null) return fields; } + /// public virtual void Parse(KeyValuePair queryParameter) - { - // expected: fields[TYPE]=prop1,prop2 - var typeName = queryParameter.Key.Split(QueryConstants.OPEN_BRACKET, QueryConstants.CLOSE_BRACKET)[1]; + { // expected: articles?fields=prop1,prop2 + // articles?fields[articles]=prop1,prop2 + // articles?fields[relationship]=prop1,prop2 var fields = new List { nameof(Identifiable.Id) }; + fields.AddRange(((string)queryParameter.Value).Split(QueryConstants.COMMA)); - var relationship = _requestResource.Relationships.SingleOrDefault(a => a.Is(typeName)); - if (relationship == null && string.Equals(typeName, _requestResource.EntityName, StringComparison.OrdinalIgnoreCase) == false) - throw new JsonApiException(400, $"fields[{typeName}] is invalid"); + var keySplitted = queryParameter.Key.Split(QueryConstants.OPEN_BRACKET, QueryConstants.CLOSE_BRACKET); - fields.AddRange(((string)queryParameter.Value).Split(QueryConstants.COMMA)); - foreach (var field in fields) - { - if (relationship != default) - { - var relationProperty = _contextEntityProvider.GetContextEntity(relationship.DependentType); - var attr = relationProperty.Attributes.SingleOrDefault(a => a.Is(field)); - if (attr == null) - throw new JsonApiException(400, $"'{relationship.DependentType.Name}' does not contain '{field}'."); + if (keySplitted.Count() == 1) // input format: fields=prop1,prop2 + foreach (var field in fields) + RegisterRequestResourceField(field); + else + { // input format: fields[articles]=prop1,prop2 + string navigation = keySplitted[1]; + // it is possible that the request resource has a relationship + // that is equal to the resource name, like with self-referering data types (eg directory structures) + // if not, no longer support this type of sparse field selection. + if (navigation == _requestResource.EntityName && !_requestResource.Relationships.Any(a => a.Is(navigation))) + throw new JsonApiException(400, $"Use \"?fields=...\" instead of \"fields[{navigation}]\":" + + $" the square bracket navigations is now reserved " + + $"for relationships only. See https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/555#issuecomment-543100865"); + + var relationship = _requestResource.Relationships.SingleOrDefault(a => a.Is(navigation)); + if (relationship == null) + throw new JsonApiException(400, $"\"{navigation}\" in \"fields[{navigation}]\" is not a valid relationship of {_requestResource.EntityName}"); - if (!_selectedRelationshipFields.TryGetValue(relationship, out var registeredFields)) - _selectedRelationshipFields.Add(relationship, registeredFields = new List()); - registeredFields.Add(attr); - } - else - { - var attr = _requestResource.Attributes.SingleOrDefault(a => a.Is(field)); - if (attr == null) - throw new JsonApiException(400, $"'{_requestResource.EntityName}' does not contain '{field}'."); + foreach (var field in fields) + RegisterRelatedResourceField(field, relationship); - (_selectedFields = _selectedFields ?? new List()).Add(attr); - } } } + + /// + /// Registers field selection queries of the form articles?fields[author]=first-name + /// + private void RegisterRelatedResourceField(string field, RelationshipAttribute relationship) + { + var relationProperty = _contextEntityProvider.GetContextEntity(relationship.DependentType); + var attr = relationProperty.Attributes.SingleOrDefault(a => a.Is(field)); + if (attr == null) + throw new JsonApiException(400, $"'{relationship.DependentType.Name}' does not contain '{field}'."); + + if (!_selectedRelationshipFields.TryGetValue(relationship, out var registeredFields)) + _selectedRelationshipFields.Add(relationship, registeredFields = new List()); + registeredFields.Add(attr); + } + + /// + /// Registers field selection queries of the form articles?fields=title + /// + private void RegisterRequestResourceField(string field) + { + var attr = _requestResource.Attributes.SingleOrDefault(a => a.Is(field)); + if (attr == null) + throw new JsonApiException(400, $"'{_requestResource.EntityName}' does not contain '{field}'."); + + (_selectedFields = _selectedFields ?? new List()).Add(attr); + } } } diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/SparseFieldSetTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/SparseFieldSetTests.cs index feba7e7ce5..ea230a8261 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/SparseFieldSetTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/SparseFieldSetTests.cs @@ -104,7 +104,7 @@ public async Task Fields_Query_Selects_Sparse_Field_Sets() var server = new TestServer(builder); var client = server.CreateClient(); - var route = $"/api/v1/todo-items/{todoItem.Id}?fields[todo-items]=description,created-date"; + var route = $"/api/v1/todo-items/{todoItem.Id}?fields=description,created-date"; var request = new HttpRequestMessage(httpMethod, route); // act @@ -119,6 +119,36 @@ public async Task Fields_Query_Selects_Sparse_Field_Sets() Assert.Equal(todoItem.CreatedDate.ToString("G"), ((DateTime)deserializeBody.SingleData.Attributes["created-date"]).ToString("G")); } + [Fact] + public async Task Fields_Query_Selects_Sparse_Field_Sets_With_Type_As_Navigation() + { + // arrange + var todoItem = new TodoItem + { + Description = "description", + Ordinal = 1, + CreatedDate = DateTime.Now + }; + _dbContext.TodoItems.Add(todoItem); + await _dbContext.SaveChangesAsync(); + + var builder = new WebHostBuilder() + .UseStartup(); + var httpMethod = new HttpMethod("GET"); + var server = new TestServer(builder); + var client = server.CreateClient(); + var route = $"/api/v1/todo-items/{todoItem.Id}?fields[todo-items]=description,created-date"; + var request = new HttpRequestMessage(httpMethod, route); + + // act + var response = await client.SendAsync(request); + var body = await response.Content.ReadAsStringAsync(); + + // assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Contains("relationships only", body); + } + [Fact] public async Task Fields_Query_Selects_All_Fieldset_With_HasOne() { diff --git a/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs b/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs index eeb0648110..6e27f1bf12 100644 --- a/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs +++ b/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs @@ -38,7 +38,7 @@ public void Parse_ValidSelection_CanParse() var attribute = new AttrAttribute(attrName) { InternalAttributeName = internalAttrName }; var idAttribute = new AttrAttribute("id") { InternalAttributeName = "Id" }; - var query = new KeyValuePair($"fields[{type}]", new StringValues(attrName)); + var query = new KeyValuePair($"fields", new StringValues(attrName)); var contextEntity = new ContextEntity { @@ -58,6 +58,31 @@ public void Parse_ValidSelection_CanParse() Assert.Equal(attribute, result[1]); } + [Fact] + public void Parse_TypeNameAsNavigation_ThrowsJsonApiException() + { + // arrange + const string type = "articles"; + const string attrName = "some-field"; + const string internalAttrName = "SomeField"; + var attribute = new AttrAttribute(attrName) { InternalAttributeName = internalAttrName }; + var idAttribute = new AttrAttribute("id") { InternalAttributeName = "Id" }; + + var query = new KeyValuePair($"fields[{type}]", new StringValues(attrName)); + + var contextEntity = new ContextEntity + { + EntityName = type, + Attributes = new List { attribute, idAttribute }, + Relationships = new List() + }; + var service = GetService(contextEntity); + + // act, assert + var ex = Assert.Throws(() => service.Parse(query)); + Assert.Contains("relationships only", ex.Message); + } + [Fact] public void Parse_InvalidField_ThrowsJsonApiException() { From f5d0e8fd398339d2e2e9e4061dffcd7a4e876b71 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Fri, 18 Oct 2019 10:27:02 +0200 Subject: [PATCH 2/4] feat: throw error on deeply nested selections --- .../SparseFieldsService.cs | 7 ++++- .../SparseFieldsServiceTests.cs | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs b/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs index f9ae4012ca..aa7e121c54 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs @@ -51,9 +51,11 @@ public virtual void Parse(KeyValuePair queryParameter) var keySplitted = queryParameter.Key.Split(QueryConstants.OPEN_BRACKET, QueryConstants.CLOSE_BRACKET); - if (keySplitted.Count() == 1) // input format: fields=prop1,prop2 + if (keySplitted.Count() == 1) + { // input format: fields=prop1,prop2 foreach (var field in fields) RegisterRequestResourceField(field); + } else { // input format: fields[articles]=prop1,prop2 string navigation = keySplitted[1]; @@ -65,6 +67,9 @@ public virtual void Parse(KeyValuePair queryParameter) $" the square bracket navigations is now reserved " + $"for relationships only. See https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/555#issuecomment-543100865"); + if (navigation.Contains(QueryConstants.DOT)) + throw new JsonApiException(400, $"fields[{navigation}] is not valid: deeply nested sparse field selection is not yet supported."); + var relationship = _requestResource.Relationships.SingleOrDefault(a => a.Is(navigation)); if (relationship == null) throw new JsonApiException(400, $"\"{navigation}\" in \"fields[{navigation}]\" is not a valid relationship of {_requestResource.EntityName}"); diff --git a/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs b/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs index 6e27f1bf12..8dc0307305 100644 --- a/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs +++ b/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs @@ -83,6 +83,32 @@ public void Parse_TypeNameAsNavigation_ThrowsJsonApiException() Assert.Contains("relationships only", ex.Message); } + [Fact] + public void Parse_DeeplyNestedSelection_ThrowsJsonApiException() + { + // arrange + const string type = "articles"; + const string relationship = "author.employer"; + const string attrName = "some-field"; + const string internalAttrName = "SomeField"; + var attribute = new AttrAttribute(attrName) { InternalAttributeName = internalAttrName }; + var idAttribute = new AttrAttribute("id") { InternalAttributeName = "Id" }; + + var query = new KeyValuePair($"fields[{relationship}]", new StringValues(attrName)); + + var contextEntity = new ContextEntity + { + EntityName = type, + Attributes = new List { attribute, idAttribute }, + Relationships = new List() + }; + var service = GetService(contextEntity); + + // act, assert + var ex = Assert.Throws(() => service.Parse(query)); + Assert.Contains("deeply nested", ex.Message); + } + [Fact] public void Parse_InvalidField_ThrowsJsonApiException() { From b8e628d072a25bcdc90cbb07c509e426580f7844 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 22 Oct 2019 11:47:40 +0200 Subject: [PATCH 3/4] docs: add extra comment --- .../QueryParameterServices/SparseFieldsService.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs b/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs index aa7e121c54..e7ea772582 100644 --- a/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs +++ b/src/JsonApiDotNetCore/QueryParameterServices/SparseFieldsService.cs @@ -39,12 +39,11 @@ public List Get(RelationshipAttribute relationship = null) _selectedRelationshipFields.TryGetValue(relationship, out var fields); return fields; } - /// public virtual void Parse(KeyValuePair queryParameter) { // expected: articles?fields=prop1,prop2 - // articles?fields[articles]=prop1,prop2 + // articles?fields[articles]=prop1,prop2 <-- this form in invalid UNLESS "articles" is actually a relationship on Article // articles?fields[relationship]=prop1,prop2 var fields = new List { nameof(Identifiable.Id) }; fields.AddRange(((string)queryParameter.Value).Split(QueryConstants.COMMA)); From b24bb60aedea406d9cf3a9c982e1d36db64a3056 Mon Sep 17 00:00:00 2001 From: Maurits Moeys Date: Tue, 22 Oct 2019 15:14:49 +0200 Subject: [PATCH 4/4] chore: processed Wisepotatos review --- test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs b/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs index 8dc0307305..03f855d901 100644 --- a/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs +++ b/test/UnitTests/QueryParameters/SparseFieldsServiceTests.cs @@ -59,7 +59,7 @@ public void Parse_ValidSelection_CanParse() } [Fact] - public void Parse_TypeNameAsNavigation_ThrowsJsonApiException() + public void Parse_TypeNameAsNavigation_Throws400ErrorWithRelationshipsOnlyMessage() { // arrange const string type = "articles"; @@ -84,7 +84,7 @@ public void Parse_TypeNameAsNavigation_ThrowsJsonApiException() } [Fact] - public void Parse_DeeplyNestedSelection_ThrowsJsonApiException() + public void Parse_DeeplyNestedSelection_Throws400ErrorWithDeeplyNestedMessage() { // arrange const string type = "articles";