From 0170a34067485f3c4431ec0eb324d6ae845d9144 Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 23 Jan 2020 19:06:54 +0100 Subject: [PATCH] Fixed invalid top-level self link. Removed dependency on ResourceContext in the process, because it was no longer needed. Additionally I changed the decision to render which links to be taken from the related entity (from Article in a request like /people/1/articles instead of Person), because I believe that is more correct than taking it from the primary entity, which is the old behavior. --- .../Serialization/Client/RequestSerializer.cs | 7 ++- .../Serialization/Common/DocumentBuilder.cs | 5 +- .../Server/Builders/LinkBuilder.cs | 58 +++++++++++++------ .../Server/Contracts/ILinkBuilder.cs | 4 +- .../Server/ResponseSerializer.cs | 7 +-- .../Spec/FetchingRelationshipsTests.cs | 4 +- test/UnitTests/Builders/LinkBuilderTests.cs | 36 +++++++++--- .../Common/DocumentBuilderTests.cs | 2 +- .../Serialization/SerializerTestsSetup.cs | 19 ++---- 9 files changed, 86 insertions(+), 56 deletions(-) diff --git a/src/JsonApiDotNetCore/Serialization/Client/RequestSerializer.cs b/src/JsonApiDotNetCore/Serialization/Client/RequestSerializer.cs index 178fb7c2d4..b47bfca5c4 100644 --- a/src/JsonApiDotNetCore/Serialization/Client/RequestSerializer.cs +++ b/src/JsonApiDotNetCore/Serialization/Client/RequestSerializer.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections; using System.Collections.Generic; using System.Linq; @@ -15,9 +15,10 @@ public class RequestSerializer : BaseDocumentBuilder, IRequestSerializer { private Type _currentTargetedResource; private readonly IResourceGraph _resourceGraph; + public RequestSerializer(IResourceGraph resourceGraph, IResourceObjectBuilder resourceObjectBuilder) - : base(resourceObjectBuilder, resourceGraph) + : base(resourceObjectBuilder) { _resourceGraph = resourceGraph; } @@ -96,4 +97,4 @@ private List GetRelationshipsToSerialize(IIdentifiable en return RelationshipsToSerialize.ToList(); } } -} \ No newline at end of file +} diff --git a/src/JsonApiDotNetCore/Serialization/Common/DocumentBuilder.cs b/src/JsonApiDotNetCore/Serialization/Common/DocumentBuilder.cs index 54e4066ecf..2e4a2444f4 100644 --- a/src/JsonApiDotNetCore/Serialization/Common/DocumentBuilder.cs +++ b/src/JsonApiDotNetCore/Serialization/Common/DocumentBuilder.cs @@ -11,12 +11,11 @@ namespace JsonApiDotNetCore.Serialization /// public abstract class BaseDocumentBuilder { - protected readonly IResourceContextProvider _provider; protected readonly IResourceObjectBuilder _resourceObjectBuilder; - protected BaseDocumentBuilder(IResourceObjectBuilder resourceObjectBuilder, IResourceContextProvider provider) + + protected BaseDocumentBuilder(IResourceObjectBuilder resourceObjectBuilder) { _resourceObjectBuilder = resourceObjectBuilder; - _provider = provider; } /// diff --git a/src/JsonApiDotNetCore/Serialization/Server/Builders/LinkBuilder.cs b/src/JsonApiDotNetCore/Serialization/Server/Builders/LinkBuilder.cs index 8fe73a289d..7307d6438e 100644 --- a/src/JsonApiDotNetCore/Serialization/Server/Builders/LinkBuilder.cs +++ b/src/JsonApiDotNetCore/Serialization/Server/Builders/LinkBuilder.cs @@ -1,3 +1,4 @@ +using System.Text; using JsonApiDotNetCore.Configuration; using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Internal.Contracts; @@ -27,17 +28,19 @@ public LinkBuilder(ILinksConfiguration options, } /// - public TopLevelLinks GetTopLevelLinks(ResourceContext primaryResource) + public TopLevelLinks GetTopLevelLinks() { + ResourceContext resourceContext = _currentRequest.GetRequestResource(); + TopLevelLinks topLevelLinks = null; - if (ShouldAddTopLevelLink(primaryResource, Link.Self)) + if (ShouldAddTopLevelLink(resourceContext, Link.Self)) { - topLevelLinks = new TopLevelLinks { Self = GetSelfTopLevelLink(primaryResource.ResourceName) }; + topLevelLinks = new TopLevelLinks { Self = GetSelfTopLevelLink(resourceContext) }; } - if (ShouldAddTopLevelLink(primaryResource, Link.Paging) && _pageService.CanPaginate) + if (ShouldAddTopLevelLink(resourceContext, Link.Paging) && _pageService.CanPaginate) { - SetPageLinks(primaryResource, topLevelLinks ??= new TopLevelLinks()); + SetPageLinks(resourceContext, topLevelLinks ??= new TopLevelLinks()); } return topLevelLinks; @@ -48,50 +51,69 @@ public TopLevelLinks GetTopLevelLinks(ResourceContext primaryResource) /// configuration on the , and if not configured, by checking with the /// global configuration in . /// + /// /// - private bool ShouldAddTopLevelLink(ResourceContext primaryResource, Link link) + private bool ShouldAddTopLevelLink(ResourceContext resourceContext, Link link) { - if (primaryResource.TopLevelLinks != Link.NotConfigured) + if (resourceContext.TopLevelLinks != Link.NotConfigured) { - return primaryResource.TopLevelLinks.HasFlag(link); + return resourceContext.TopLevelLinks.HasFlag(link); } return _options.TopLevelLinks.HasFlag(link); } - private void SetPageLinks(ResourceContext primaryResource, TopLevelLinks links) + private void SetPageLinks(ResourceContext resourceContext, TopLevelLinks links) { if (_pageService.CurrentPage > 1) { - links.Prev = GetPageLink(primaryResource, _pageService.CurrentPage - 1, _pageService.CurrentPageSize); + links.Prev = GetPageLink(resourceContext, _pageService.CurrentPage - 1, _pageService.CurrentPageSize); } if (_pageService.CurrentPage < _pageService.TotalPages) { - links.Next = GetPageLink(primaryResource, _pageService.CurrentPage + 1, _pageService.CurrentPageSize); + links.Next = GetPageLink(resourceContext, _pageService.CurrentPage + 1, _pageService.CurrentPageSize); } if (_pageService.TotalPages > 0) { - links.Self = GetPageLink(primaryResource, _pageService.CurrentPage, _pageService.CurrentPageSize); - links.First = GetPageLink(primaryResource, 1, _pageService.CurrentPageSize); - links.Last = GetPageLink(primaryResource, _pageService.TotalPages, _pageService.CurrentPageSize); + links.Self = GetPageLink(resourceContext, _pageService.CurrentPage, _pageService.CurrentPageSize); + links.First = GetPageLink(resourceContext, 1, _pageService.CurrentPageSize); + links.Last = GetPageLink(resourceContext, _pageService.TotalPages, _pageService.CurrentPageSize); } } - private string GetSelfTopLevelLink(string resourceName) + private string GetSelfTopLevelLink(ResourceContext resourceContext) { - return $"{GetBasePath()}/{resourceName}"; + var builder = new StringBuilder(); + builder.Append(GetBasePath()); + builder.Append("/"); + builder.Append(resourceContext.ResourceName); + + string resourceId = _currentRequest.BaseId; + if (resourceId != null) + { + builder.Append("/"); + builder.Append(resourceId); + } + + if (_currentRequest.RequestRelationship != null) + { + builder.Append("/"); + builder.Append(_currentRequest.RequestRelationship.PublicRelationshipName); + } + + return builder.ToString(); } - private string GetPageLink(ResourceContext primaryResource, int pageOffset, int pageSize) + private string GetPageLink(ResourceContext resourceContext, int pageOffset, int pageSize) { if (_pageService.Backwards) { pageOffset = -pageOffset; } - return $"{GetBasePath()}/{primaryResource.ResourceName}?page[size]={pageSize}&page[number]={pageOffset}"; + return $"{GetBasePath()}/{resourceContext.ResourceName}?page[size]={pageSize}&page[number]={pageOffset}"; } diff --git a/src/JsonApiDotNetCore/Serialization/Server/Contracts/ILinkBuilder.cs b/src/JsonApiDotNetCore/Serialization/Server/Contracts/ILinkBuilder.cs index a4bd87195a..7e3224969e 100644 --- a/src/JsonApiDotNetCore/Serialization/Server/Contracts/ILinkBuilder.cs +++ b/src/JsonApiDotNetCore/Serialization/Server/Contracts/ILinkBuilder.cs @@ -1,4 +1,3 @@ -using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Models; using JsonApiDotNetCore.Models.Links; @@ -12,8 +11,7 @@ public interface ILinkBuilder /// /// Builds the links object that is included in the top-level of the document. /// - /// The primary resource of the response body - TopLevelLinks GetTopLevelLinks(ResourceContext primaryResource); + TopLevelLinks GetTopLevelLinks(); /// /// Builds the links object for resources in the primary data. /// diff --git a/src/JsonApiDotNetCore/Serialization/Server/ResponseSerializer.cs b/src/JsonApiDotNetCore/Serialization/Server/ResponseSerializer.cs index e54a0abd0a..dc2ef0264b 100644 --- a/src/JsonApiDotNetCore/Serialization/Server/ResponseSerializer.cs +++ b/src/JsonApiDotNetCore/Serialization/Server/ResponseSerializer.cs @@ -40,9 +40,8 @@ public ResponseSerializer(IMetaBuilder metaBuilder, ILinkBuilder linkBuilder, IIncludedResourceObjectBuilder includedBuilder, IFieldsToSerialize fieldsToSerialize, - IResourceObjectBuilder resourceObjectBuilder, - IResourceContextProvider provider) : - base(resourceObjectBuilder, provider) + IResourceObjectBuilder resourceObjectBuilder) : + base(resourceObjectBuilder) { _fieldsToSerialize = fieldsToSerialize; _linkBuilder = linkBuilder; @@ -158,7 +157,7 @@ private List GetRelationshipsToSerialize(Type resourceTyp /// private void AddTopLevelObjects(Document document) { - document.Links = _linkBuilder.GetTopLevelLinks(_provider.GetResourceContext()); + document.Links = _linkBuilder.GetTopLevelLinks(); document.Meta = _metaBuilder.GetMeta(); document.Included = _includedBuilder.Build(); } diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/FetchingRelationshipsTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/FetchingRelationshipsTests.cs index f5a8921a02..a3ec7d83d0 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/FetchingRelationshipsTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Spec/FetchingRelationshipsTests.cs @@ -1,4 +1,4 @@ -using System.Net; +using System.Net; using System.Net.Http; using System.Threading.Tasks; using Bogus; @@ -43,7 +43,7 @@ public async Task Request_UnsetRelationship_Returns_Null_DataObject() var server = new TestServer(builder); var client = server.CreateClient(); var request = new HttpRequestMessage(httpMethod, route); - var expectedBody = "{\"meta\":{\"copyright\":\"Copyright 2015 Example Corp.\",\"authors\":[\"Jared Nance\",\"Maurits Moeys\",\"Harro van der Kroft\"]},\"links\":{\"self\":\"http://localhost/api/v1/people\"},\"data\":null}"; + var expectedBody = "{\"meta\":{\"copyright\":\"Copyright 2015 Example Corp.\",\"authors\":[\"Jared Nance\",\"Maurits Moeys\",\"Harro van der Kroft\"]},\"links\":{\"self\":\"http://localhost" + route + "\"},\"data\":null}"; // Act var response = await client.SendAsync(request); diff --git a/test/UnitTests/Builders/LinkBuilderTests.cs b/test/UnitTests/Builders/LinkBuilderTests.cs index bfb1bc5ff1..5408fbf809 100644 --- a/test/UnitTests/Builders/LinkBuilderTests.cs +++ b/test/UnitTests/Builders/LinkBuilderTests.cs @@ -1,3 +1,4 @@ +using System; using JsonApiDotNetCore.Configuration; using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Internal.Contracts; @@ -18,6 +19,8 @@ public class LinkBuilderTests private readonly IPageService _pageService; private readonly Mock _provider = new Mock(); private const string _host = "http://www.example.com"; + private const int _baseId = 123; + private const string _relationshipName = "author"; private const string _topSelf = "http://www.example.com/articles"; private const string _resourceSelf = "http://www.example.com/articles/123"; private const string _relSelf = "http://www.example.com/articles/123/relationships/author"; @@ -47,7 +50,7 @@ public void BuildResourceLinks_GlobalAndResourceConfiguration_ExpectedResult(Lin var builder = new LinkBuilder(config, GetRequestManager(), null, _provider.Object); // Act - var links = builder.GetResourceLinks("articles", "123"); + var links = builder.GetResourceLinks("articles", _baseId.ToString()); // Assert if (expectedResult == null) @@ -96,7 +99,7 @@ public void BuildRelationshipLinks_GlobalResourceAndAttrConfiguration_ExpectedLi var attr = new HasOneAttribute(links: relationship) { RightType = typeof(Author), PublicRelationshipName = "author" }; // Act - var links = builder.GetRelationshipLinks(attr, new Article { Id = 123 }); + var links = builder.GetRelationshipLinks(attr, new Article { Id = _baseId }); // Assert if (expectedSelfLink == null && expectedRelatedLink == null) @@ -131,9 +134,17 @@ public void BuildRelationshipLinks_GlobalResourceAndAttrConfiguration_ExpectedLi [InlineData(Link.None, Link.Self, _topSelf, false)] [InlineData(Link.None, Link.Paging, null, true)] [InlineData(Link.None, Link.None, null, false)] + [InlineData(Link.All, Link.Self, _resourceSelf, false)] + [InlineData(Link.Self, Link.Self, _resourceSelf, false)] + [InlineData(Link.Paging, Link.Self, _resourceSelf, false)] + [InlineData(Link.None, Link.Self, _resourceSelf, false)] + [InlineData(Link.All, Link.Self, _relRelated, false)] + [InlineData(Link.Self, Link.Self, _relRelated, false)] + [InlineData(Link.Paging, Link.Self, _relRelated, false)] + [InlineData(Link.None, Link.Self, _relRelated, false)] public void BuildTopLevelLinks_GlobalAndResourceConfiguration_ExpectedLinks(Link global, Link resource, - object expectedSelfLink, + string expectedSelfLink, bool pages) { // Arrange @@ -141,10 +152,14 @@ public void BuildTopLevelLinks_GlobalAndResourceConfiguration_ExpectedLinks(Link var primaryResource = GetResourceContext
(topLevelLinks: resource); _provider.Setup(m => m.GetResourceContext
()).Returns(primaryResource); - var builder = new LinkBuilder(config, GetRequestManager(), _pageService, _provider.Object); + bool useBaseId = expectedSelfLink != _topSelf; + string relationshipName = expectedSelfLink == _relRelated ? _relationshipName : null; + ICurrentRequest currentRequest = GetRequestManager(primaryResource, useBaseId, relationshipName); + + var builder = new LinkBuilder(config, currentRequest, _pageService, _provider.Object); // Act - var links = builder.GetTopLevelLinks(primaryResource); + var links = builder.GetTopLevelLinks(); // Assert if (!pages && expectedSelfLink == null) @@ -153,11 +168,11 @@ public void BuildTopLevelLinks_GlobalAndResourceConfiguration_ExpectedLinks(Link } else { - Assert.True(CheckPages(links, pages)); + Assert.True(CheckLinks(links, pages, expectedSelfLink)); } } - private bool CheckPages(TopLevelLinks links, bool pages) + private bool CheckLinks(TopLevelLinks links, bool pages, string expectedSelfLink) { if (pages) { @@ -167,13 +182,16 @@ private bool CheckPages(TopLevelLinks links, bool pages) && links.Next == $"{_host}/articles?page[size]=10&page[number]=3" && links.Last == $"{_host}/articles?page[size]=10&page[number]=3"; } - return links.First == null && links.Prev == null && links.Next == null && links.Last == null; + + return links.Self == expectedSelfLink && links.First == null && links.Prev == null && links.Next == null && links.Last == null; } - private ICurrentRequest GetRequestManager(ResourceContext resourceContext = null) + private ICurrentRequest GetRequestManager(ResourceContext resourceContext = null, bool useBaseId = false, string relationshipName = null) { var mock = new Mock(); mock.Setup(m => m.BasePath).Returns(_host); + mock.Setup(m => m.BaseId).Returns(useBaseId ? _baseId.ToString() : null); + mock.Setup(m => m.RequestRelationship).Returns(relationshipName != null ? new HasOneAttribute(relationshipName) : null); mock.Setup(m => m.GetRequestResource()).Returns(resourceContext); return mock.Object; } diff --git a/test/UnitTests/Serialization/Common/DocumentBuilderTests.cs b/test/UnitTests/Serialization/Common/DocumentBuilderTests.cs index 78fb2f53ef..75cc825ad3 100644 --- a/test/UnitTests/Serialization/Common/DocumentBuilderTests.cs +++ b/test/UnitTests/Serialization/Common/DocumentBuilderTests.cs @@ -18,7 +18,7 @@ public BaseDocumentBuilderTests() { var mock = new Mock(); mock.Setup(m => m.Build(It.IsAny(), It.IsAny>(), It.IsAny>())).Returns(new ResourceObject()); - _builder = new TestDocumentBuilder(mock.Object, _resourceGraph); + _builder = new TestDocumentBuilder(mock.Object); } diff --git a/test/UnitTests/Serialization/SerializerTestsSetup.cs b/test/UnitTests/Serialization/SerializerTestsSetup.cs index 133482a5d1..e5869a53ad 100644 --- a/test/UnitTests/Serialization/SerializerTestsSetup.cs +++ b/test/UnitTests/Serialization/SerializerTestsSetup.cs @@ -1,7 +1,6 @@ -using System; -using System.Collections; +using System; +using System.Collections; using System.Collections.Generic; -using JsonApiDotNetCore.Internal; using JsonApiDotNetCore.Internal.Contracts; using JsonApiDotNetCore.Managers.Contracts; using JsonApiDotNetCore.Models; @@ -49,9 +48,8 @@ protected ResponseSerializer GetResponseSerializer(List(meta, link, includedBuilder, fieldsToSerialize, resourceObjectBuilder, provider); + return new ResponseSerializer(meta, link, includedBuilder, fieldsToSerialize, resourceObjectBuilder); } protected ResponseResourceObjectBuilder GetResponseResourceObjectBuilder(List> inclusionChains = null, ResourceLinks resourceLinks = null, RelationshipLinks relationshipLinks = null) @@ -74,11 +72,6 @@ protected IResourceObjectBuilderSettingsProvider GetSerializerSettingsProvider() return mock.Object; } - private IResourceGraph GetResourceContextProvider() - { - return _resourceGraph; - } - protected IMetaBuilder GetMetaBuilder(Dictionary meta = null) where T : class, IIdentifiable { var mock = new Mock>(); @@ -96,7 +89,7 @@ protected ICurrentRequest GetRequestManager() where T : class, IIdentifiable protected ILinkBuilder GetLinkBuilder(TopLevelLinks top = null, ResourceLinks resource = null, RelationshipLinks relationship = null) { var mock = new Mock(); - mock.Setup(m => m.GetTopLevelLinks(It.IsAny())).Returns(top); + mock.Setup(m => m.GetTopLevelLinks()).Returns(top); mock.Setup(m => m.GetResourceLinks(It.IsAny(), It.IsAny())).Returns(resource); mock.Setup(m => m.GetRelationshipLinks(It.IsAny(), It.IsAny())).Returns(relationship); return mock.Object; @@ -131,7 +124,7 @@ protected IIncludeService GetIncludedRelationships(List protected class TestDocumentBuilder : BaseDocumentBuilder { - public TestDocumentBuilder(IResourceObjectBuilder resourceObjectBuilder, IResourceContextProvider provider) : base(resourceObjectBuilder, provider) { } + public TestDocumentBuilder(IResourceObjectBuilder resourceObjectBuilder) : base(resourceObjectBuilder) { } public new Document Build(IIdentifiable entity, List attributes = null, List relationships = null) { @@ -144,4 +137,4 @@ public TestDocumentBuilder(IResourceObjectBuilder resourceObjectBuilder, IResour } } } -} \ No newline at end of file +}