Skip to content

Commit 1ac9d9c

Browse files
maureimaurei
authored
Fix: top-level links missing when paging (#625)
* test: reproduce bug * test: expand bug reproduction * fix: generate page links correctly * fix: linkbuilder unit tests * fix: failing test resource hooks usage with many-to-many * fix: merge backward paging test using [Theory], fix backward paging issue
1 parent 08b786a commit 1ac9d9c

File tree

12 files changed

+187
-200
lines changed

12 files changed

+187
-200
lines changed

‎src/Examples/JsonApiDotNetCoreExample/Resources/LockableResource.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
using JsonApiDotNetCore.Internal;
55
using JsonApiDotNetCore.Internal.Contracts;
66
using JsonApiDotNetCore.Models;
7-
using JsonApiDotNetCore.Services;
87
using JsonApiDotNetCoreExample.Models;
98

109
namespace JsonApiDotNetCoreExample.Resources

‎src/JsonApiDotNetCore/Data/DefaultResourceRepository.cs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,7 @@ public virtual async Task<IEnumerable<TResource>> PageAsync(IQueryable<TResource
309309
entities = entities.PageForward(pageSize, pageNumber);
310310
return entities is IAsyncQueryProvider ? await entities.ToListAsync() : entities.ToList();
311311
}
312+
312313
if (entities is IAsyncEnumerable<TResource>)
313314
{
314315
// since EntityFramework does not support IQueryable.Reverse(), we need to know the number of queried entities
@@ -317,10 +318,12 @@ public virtual async Task<IEnumerable<TResource>> PageAsync(IQueryable<TResource
317318
int virtualFirstIndex = totalCount - pageSize * Math.Abs(pageNumber);
318319
int numberOfElementsInPage = Math.Min(pageSize, virtualFirstIndex + pageSize);
319320

320-
return await ToListAsync(entities.Skip(virtualFirstIndex).Take(numberOfElementsInPage));
321-
} else
321+
var result = await ToListAsync(entities.Skip(virtualFirstIndex).Take(numberOfElementsInPage));
322+
return result.Reverse();
323+
}
324+
else
322325
{
323-
int firstIndex = pageSize * Math.Abs(pageNumber) - 1;
326+
int firstIndex = pageSize * (Math.Abs(pageNumber) - 1);
324327
int numberOfElementsInPage = Math.Min(pageSize, firstIndex + pageSize);
325328
return entities.Reverse().Skip(firstIndex).Take(numberOfElementsInPage);
326329
}

‎src/JsonApiDotNetCore/Hooks/Execution/HookExecutorHelper.cs

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,15 @@ public IResourceHookContainer<TResource> GetResourceHookContainer<TResource>(Res
7474
return (IResourceHookContainer<TResource>)GetResourceHookContainer(typeof(TResource), hook);
7575
}
7676

77-
public IEnumerable LoadDbValues(LeftType entityTypeForRepository, IEnumerable entities, ResourceHook hook, params RelationshipAttribute[] inclusionChain)
77+
public IEnumerable LoadDbValues(LeftType entityTypeForRepository, IEnumerable entities, ResourceHook hook, params RelationshipAttribute[] relationshipsToNextLayer)
7878
{
7979
var idType = TypeHelper.GetIdentifierType(entityTypeForRepository);
8080
var parameterizedGetWhere = GetType()
8181
.GetMethod(nameof(GetWhereAndInclude), BindingFlags.NonPublic | BindingFlags.Instance)
8282
.MakeGenericMethod(entityTypeForRepository, idType);
8383
var casted = ((IEnumerable<object>)entities).Cast<IIdentifiable>();
8484
var ids = casted.Select(e => e.StringId).Cast(idType);
85-
var values = (IEnumerable)parameterizedGetWhere.Invoke(this, new object[] { ids, inclusionChain });
85+
var values = (IEnumerable)parameterizedGetWhere.Invoke(this, new object[] { ids, relationshipsToNextLayer });
8686
if (values == null) return null;
8787
return (IEnumerable)Activator.CreateInstance(typeof(HashSet<>).MakeGenericType(entityTypeForRepository), values.Cast(entityTypeForRepository));
8888
}
@@ -129,11 +129,15 @@ IHooksDiscovery GetHookDiscovery(Type entityType)
129129
return discovery;
130130
}
131131

132-
IEnumerable<TResource> GetWhereAndInclude<TResource, TId>(IEnumerable<TId> ids, RelationshipAttribute[] inclusionChain) where TResource : class, IIdentifiable<TId>
132+
IEnumerable<TResource> GetWhereAndInclude<TResource, TId>(IEnumerable<TId> ids, RelationshipAttribute[] relationshipsToNextLayer) where TResource : class, IIdentifiable<TId>
133133
{
134134
var repo = GetRepository<TResource, TId>();
135135
var query = repo.Get().Where(e => ids.Contains(e.Id));
136-
return repo.Include(query, inclusionChain).ToList();
136+
foreach (var inclusionChainElement in relationshipsToNextLayer)
137+
{
138+
query = repo.Include(query, new RelationshipAttribute[] { inclusionChainElement });
139+
}
140+
return query.ToList();
137141
}
138142

139143
IResourceReadRepository<TResource, TId> GetRepository<TResource, TId>() where TResource : class, IIdentifiable<TId>
@@ -183,12 +187,6 @@ public Dictionary<RelationshipAttribute, IEnumerable> LoadImplicitlyAffected(
183187
}
184188

185189
return implicitlyAffected.ToDictionary(kvp => kvp.Key, kvp => TypeHelper.CreateHashSetFor(kvp.Key.RightType, kvp.Value));
186-
187-
}
188-
189-
private IEnumerable CreateHashSet(Type type, IList elements)
190-
{
191-
return (IEnumerable)Activator.CreateInstance(typeof(HashSet<>).MakeGenericType(type), new object[] { elements });
192190
}
193191

194192
bool IsHasManyThrough(KeyValuePair<RelationshipAttribute, IEnumerable> kvp,
@@ -201,3 +199,4 @@ bool IsHasManyThrough(KeyValuePair<RelationshipAttribute, IEnumerable> kvp,
201199
}
202200
}
203201
}
202+

‎src/JsonApiDotNetCore/QueryParameterServices/Contracts/IPageService.cs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,20 @@ public interface IPageService : IQueryParameterService
1414
/// </summary>
1515
int PageSize { get; set; }
1616
/// <summary>
17-
/// What page are we currently on
17+
/// The page requested. Note that the page number is one-based.
1818
/// </summary>
1919
int CurrentPage { get; set; }
2020
/// <summary>
2121
/// Total amount of pages for request
2222
/// </summary>
2323
int TotalPages { get; }
2424
/// <summary>
25-
/// Checks if pagination is enabled
25+
/// Denotes if pagination is possible for the current request
2626
/// </summary>
27-
bool ShouldPaginate();
27+
bool CanPaginate { get; }
28+
/// <summary>
29+
/// Denotes if pagination is backwards
30+
/// </summary>
31+
bool Backwards { get; }
2832
}
2933
}

‎src/JsonApiDotNetCore/QueryParameterServices/PageService.cs

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -10,52 +10,79 @@ namespace JsonApiDotNetCore.Query
1010
/// <inheritdoc/>
1111
public class PageService : QueryParameterService, IPageService
1212
{
13-
private IJsonApiOptions _options;
13+
private readonly IJsonApiOptions _options;
1414

1515
public PageService(IJsonApiOptions options)
1616
{
1717
_options = options;
1818
PageSize = _options.DefaultPageSize;
1919
}
20+
2021
/// <inheritdoc/>
2122
public int? TotalRecords { get; set; }
23+
2224
/// <inheritdoc/>
2325
public int PageSize { get; set; }
26+
2427
/// <inheritdoc/>
25-
public int CurrentPage { get; set; }
28+
public int CurrentPage { get; set; } = 1;
29+
30+
/// <inheritdoc/>
31+
public bool Backwards { get; set; }
32+
2633
/// <inheritdoc/>
2734
public int TotalPages => (TotalRecords == null || PageSize == 0) ? -1 : (int)Math.Ceiling(decimal.Divide(TotalRecords.Value, PageSize));
2835

36+
/// <inheritdoc/>
37+
public bool CanPaginate { get { return TotalPages > 1; } }
38+
2939
/// <inheritdoc/>
3040
public virtual void Parse(KeyValuePair<string, StringValues> queryParameter)
3141
{
32-
// expected input = page[size]=10
33-
// page[number]=1
42+
// expected input = page[size]=<integer>
43+
// page[number]=<integer > 0>
3444
var propertyName = queryParameter.Key.Split(QueryConstants.OPEN_BRACKET, QueryConstants.CLOSE_BRACKET)[1];
3545

3646
const string SIZE = "size";
3747
const string NUMBER = "number";
3848

3949
if (propertyName == SIZE)
4050
{
41-
if (int.TryParse(queryParameter.Value, out var size))
51+
if (!int.TryParse(queryParameter.Value, out var size))
52+
{
53+
ThrowBadPagingRequest(queryParameter, "value could not be parsed as an integer");
54+
}
55+
else if (size < 1)
56+
{
57+
ThrowBadPagingRequest(queryParameter, "value needs to be greater than zero");
58+
}
59+
else
60+
{
4261
PageSize = size;
43-
else
44-
throw new JsonApiException(400, $"Invalid page size '{queryParameter.Value}'");
62+
}
4563
}
4664
else if (propertyName == NUMBER)
47-
{
48-
if (int.TryParse(queryParameter.Value, out var size))
49-
CurrentPage = size;
65+
{
66+
if (!int.TryParse(queryParameter.Value, out var number))
67+
{
68+
ThrowBadPagingRequest(queryParameter, "value could not be parsed as an integer");
69+
}
70+
else if (number == 0)
71+
{
72+
ThrowBadPagingRequest(queryParameter, "page index is not zero-based");
73+
}
5074
else
51-
throw new JsonApiException(400, $"Invalid page number '{queryParameter.Value}'");
75+
{
76+
Backwards = (number < 0);
77+
CurrentPage = Math.Abs(number);
78+
}
5279
}
5380
}
5481

55-
/// <inheritdoc/>
56-
public bool ShouldPaginate()
82+
private void ThrowBadPagingRequest(KeyValuePair<string, StringValues> parameter, string message)
5783
{
58-
return (PageSize > 0) || ((CurrentPage == 1 || CurrentPage == 0) && TotalPages <= 0);
84+
throw new JsonApiException(400, $"Invalid page query parameter '{parameter.Key}={parameter.Value}': {message}");
5985
}
86+
6087
}
6188
}

‎src/JsonApiDotNetCore/Serialization/Server/Builders/LinkBuilder.cs

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,14 @@ public TopLevelLinks GetTopLevelLinks(ResourceContext primaryResource)
3131
{
3232
TopLevelLinks topLevelLinks = null;
3333
if (ShouldAddTopLevelLink(primaryResource, Link.Self))
34+
{
3435
topLevelLinks = new TopLevelLinks { Self = GetSelfTopLevelLink(primaryResource.ResourceName) };
36+
}
3537

36-
if (ShouldAddTopLevelLink(primaryResource, Link.Paging))
37-
SetPageLinks(primaryResource, ref topLevelLinks);
38+
if (ShouldAddTopLevelLink(primaryResource, Link.Paging) && _pageService.CanPaginate)
39+
{
40+
SetPageLinks(primaryResource, topLevelLinks ??= new TopLevelLinks());
41+
}
3842

3943
return topLevelLinks;
4044
}
@@ -48,29 +52,31 @@ public TopLevelLinks GetTopLevelLinks(ResourceContext primaryResource)
4852
private bool ShouldAddTopLevelLink(ResourceContext primaryResource, Link link)
4953
{
5054
if (primaryResource.TopLevelLinks != Link.NotConfigured)
55+
{
5156
return primaryResource.TopLevelLinks.HasFlag(link);
57+
}
58+
5259
return _options.TopLevelLinks.HasFlag(link);
5360
}
5461

55-
private void SetPageLinks(ResourceContext primaryResource, ref TopLevelLinks links)
62+
private void SetPageLinks(ResourceContext primaryResource, TopLevelLinks links)
5663
{
57-
if (!_pageService.ShouldPaginate())
58-
return;
59-
60-
links = links ?? new TopLevelLinks();
61-
6264
if (_pageService.CurrentPage > 1)
6365
{
64-
links.First = GetPageLink(primaryResource, 1, _pageService.PageSize);
6566
links.Prev = GetPageLink(primaryResource, _pageService.CurrentPage - 1, _pageService.PageSize);
6667
}
6768

6869
if (_pageService.CurrentPage < _pageService.TotalPages)
70+
{
6971
links.Next = GetPageLink(primaryResource, _pageService.CurrentPage + 1, _pageService.PageSize);
70-
72+
}
7173

7274
if (_pageService.TotalPages > 0)
75+
{
76+
links.Self = GetPageLink(primaryResource, _pageService.CurrentPage, _pageService.PageSize);
77+
links.First = GetPageLink(primaryResource, 1, _pageService.PageSize);
7378
links.Last = GetPageLink(primaryResource, _pageService.TotalPages, _pageService.PageSize);
79+
}
7480
}
7581

7682
private string GetSelfTopLevelLink(string resourceName)
@@ -80,6 +86,11 @@ private string GetSelfTopLevelLink(string resourceName)
8086

8187
private string GetPageLink(ResourceContext primaryResource, int pageOffset, int pageSize)
8288
{
89+
if (_pageService.Backwards)
90+
{
91+
pageOffset = -pageOffset;
92+
}
93+
8394
return $"{GetBasePath()}/{primaryResource.ResourceName}?page[size]={pageSize}&page[number]={pageOffset}";
8495
}
8596

@@ -89,7 +100,9 @@ public ResourceLinks GetResourceLinks(string resourceName, string id)
89100
{
90101
var resourceContext = _provider.GetResourceContext(resourceName);
91102
if (ShouldAddResourceLink(resourceContext, Link.Self))
103+
{
92104
return new ResourceLinks { Self = GetSelfResourceLink(resourceName, id) };
105+
}
93106

94107
return null;
95108
}
@@ -101,7 +114,9 @@ public RelationshipLinks GetRelationshipLinks(RelationshipAttribute relationship
101114
var childNavigation = relationship.PublicRelationshipName;
102115
RelationshipLinks links = null;
103116
if (ShouldAddRelationshipLink(parentResourceContext, relationship, Link.Related))
117+
{
104118
links = new RelationshipLinks { Related = GetRelatedRelationshipLink(parentResourceContext.ResourceName, parent.StringId, childNavigation) };
119+
}
105120

106121
if (ShouldAddRelationshipLink(parentResourceContext, relationship, Link.Self))
107122
{
@@ -137,7 +152,9 @@ private string GetRelatedRelationshipLink(string parent, string parentId, string
137152
private bool ShouldAddResourceLink(ResourceContext resourceContext, Link link)
138153
{
139154
if (resourceContext.ResourceLinks != Link.NotConfigured)
155+
{
140156
return resourceContext.ResourceLinks.HasFlag(link);
157+
}
141158
return _options.ResourceLinks.HasFlag(link);
142159
}
143160

@@ -151,16 +168,24 @@ private bool ShouldAddResourceLink(ResourceContext resourceContext, Link link)
151168
private bool ShouldAddRelationshipLink(ResourceContext resourceContext, RelationshipAttribute relationship, Link link)
152169
{
153170
if (relationship.RelationshipLinks != Link.NotConfigured)
171+
{
154172
return relationship.RelationshipLinks.HasFlag(link);
173+
}
155174
if (resourceContext.RelationshipLinks != Link.NotConfigured)
175+
{
156176
return resourceContext.RelationshipLinks.HasFlag(link);
177+
}
178+
157179
return _options.RelationshipLinks.HasFlag(link);
158180
}
159181

160182
protected string GetBasePath()
161183
{
162184
if (_options.RelativeLinks)
185+
{
163186
return string.Empty;
187+
}
188+
164189
return _currentRequest.BasePath;
165190
}
166191
}

‎src/JsonApiDotNetCore/Services/DefaultResourceService.cs

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ public class DefaultResourceService<TResource, TId> :
2323
IResourceService<TResource, TId>
2424
where TResource : class, IIdentifiable<TId>
2525
{
26-
private readonly IPageService _pageManager;
26+
private readonly IPageService _pageService;
2727
private readonly IJsonApiOptions _options;
2828
private readonly IFilterService _filterService;
2929
private readonly ISortService _sortService;
@@ -44,7 +44,7 @@ public DefaultResourceService(
4444
{
4545
_includeService = queryParameters.FirstOrDefault<IIncludeService>();
4646
_sparseFieldsService = queryParameters.FirstOrDefault<ISparseFieldsService>();
47-
_pageManager = queryParameters.FirstOrDefault<IPageService>();
47+
_pageService = queryParameters.FirstOrDefault<IPageService>();
4848
_sortService = queryParameters.FirstOrDefault<ISortService>();
4949
_filterService = queryParameters.FirstOrDefault<IFilterService>();
5050
_options = options;
@@ -99,7 +99,7 @@ public virtual async Task<IEnumerable<TResource>> GetAsync()
9999
}
100100

101101
if (_options.IncludeTotalRecordCount)
102-
_pageManager.TotalRecords = await _repository.CountAsync(entityQuery);
102+
_pageService.TotalRecords = await _repository.CountAsync(entityQuery);
103103

104104
// pagination should be done last since it will execute the query
105105
var pagedEntities = await ApplyPageQueryAsync(entityQuery);
@@ -195,19 +195,24 @@ public virtual async Task UpdateRelationshipsAsync(TId id, string relationshipNa
195195

196196
protected virtual async Task<IEnumerable<TResource>> ApplyPageQueryAsync(IQueryable<TResource> entities)
197197
{
198-
if (!(_pageManager.PageSize > 0))
198+
if (!(_pageService.PageSize > 0))
199199
{
200200
var allEntities = await _repository.ToListAsync(entities);
201201
return allEntities as IEnumerable<TResource>;
202202
}
203203

204+
int pageOffset = _pageService.CurrentPage;
205+
if (_pageService.Backwards)
206+
{
207+
pageOffset = -pageOffset;
208+
}
204209
if (_logger?.IsEnabled(LogLevel.Information) == true)
205210
{
206-
_logger?.LogInformation($"Applying paging query. Fetching page {_pageManager.CurrentPage} " +
207-
$"with {_pageManager.PageSize} entities");
211+
_logger?.LogInformation($"Applying paging query. Fetching page {pageOffset} " +
212+
$"with {_pageService.PageSize} entities");
208213
}
209214

210-
return await _repository.PageAsync(entities, _pageManager.PageSize, _pageManager.CurrentPage);
215+
return await _repository.PageAsync(entities, _pageService.PageSize, pageOffset);
211216
}
212217

213218
/// <summary>

0 commit comments

Comments
 (0)