-
Notifications
You must be signed in to change notification settings - Fork 26
Add pagination to resources API #805
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: future
Are you sure you want to change the base?
Changes from all commits
a01d7f1
504e064
35911a8
345f97a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // Copyright (c) 2025, Phoenix Contact GmbH & Co. KG | ||
| // Licensed under the Apache License, Version 2.0 | ||
|
|
||
| using Microsoft.AspNetCore.Mvc.Abstractions; | ||
| using Microsoft.AspNetCore.Mvc.ActionConstraints; | ||
| using Microsoft.AspNetCore.Routing; | ||
|
|
||
| namespace Moryx.AbstractionLayer.Resources.Endpoints; | ||
|
|
||
| /// <summary> | ||
| /// The attribute controls the selection of an action method based on the existance of a query parameter <paramref name="paramName"/>. | ||
| /// </summary> | ||
| /// <param name="paramName">The query parameter to mark this method as invalid for selection</param> | ||
| public class InvalidQueryParameterAttribute(string paramName) : ActionMethodSelectorAttribute | ||
| { | ||
| /// <inheritdoc /> | ||
| public override bool IsValidForRequest(RouteContext routeContext, ActionDescriptor action) => | ||
| !routeContext.HttpContext.Request.Query.ContainsKey(paramName); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // Copyright (c) 2025, Phoenix Contact GmbH & Co. KG | ||
| // Licensed under the Apache License, Version 2.0 | ||
|
|
||
| namespace Moryx.AbstractionLayer.Resources.Endpoints; | ||
|
|
||
| /// <summary> | ||
| /// Generic paged result returned from endpoints providing the <see cref="Items"/> | ||
| /// with corresponding metadata from the request. | ||
| /// </summary> | ||
| /// <typeparam name="ItemType">Type of the items to be returned</typeparam> | ||
| public sealed class PagedResult<ItemType> | ||
| { | ||
| /// <summary> | ||
| /// The requested page from the total set of items | ||
| /// </summary> | ||
| public int PageNumber { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The maximum page size provided for the request | ||
| /// </summary> | ||
| public int PageSize { get; set; } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry, I messed this up... This is what my idea was:: That way, everything countable could be |
||
|
|
||
| /// <summary> | ||
| /// The total number of items available | ||
| /// </summary> | ||
| public long TotalCount { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The items returned for the request | ||
| /// </summary> | ||
| public ItemType[] Items { get; set; } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| // Copyright (c) 2025, Phoenix Contact GmbH & Co. KG | ||
| // Licensed under the Apache License, Version 2.0 | ||
|
|
||
| namespace Moryx.AbstractionLayer.Resources.Endpoints; | ||
|
|
||
| /// <summary> | ||
| /// Extension methods on the <see cref="PagedResult{ItemType}"/> for fluent instance creation | ||
| /// </summary> | ||
| public static class PagedResultExtensions | ||
| { | ||
| extension<ItemType>(PagedResult<ItemType> pagedResult) | ||
| { | ||
| /// <summary> | ||
| /// Adds the <paramref name="pagination"/> meta informtaion to this <paramref name="pagedResult"/> | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public PagedResult<ItemType> With(PaginationParameters pagination) | ||
| { | ||
| pagedResult.PageNumber = pagination.PageNumber; | ||
| pagedResult.PageSize = pagination.PageSize; | ||
| // ToDo: Verify that already added results match the metadata | ||
| return pagedResult; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Populates this <paramref name="pagedResult"/> with the <paramref name="fullSet"/> | ||
| /// of items taking already configured <see cref="PagedResult{ItemType}.PageNumber"/> | ||
| /// and <see cref="PagedResult{ItemType}.PageSize"/> into account | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public PagedResult<ItemType> Of(IEnumerable<ItemType> fullSet) | ||
| { | ||
| // ToDo: Account for unset metadata | ||
| pagedResult.TotalCount = fullSet.Count(); | ||
| var skip = (pagedResult.PageNumber - 1) * pagedResult.PageSize; | ||
| pagedResult.Items = [.. fullSet.Skip(skip).Take(pagedResult.PageSize)]; | ||
| return pagedResult; | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| // Copyright (c) 2025, Phoenix Contact GmbH & Co. KG | ||
| // Licensed under the Apache License, Version 2.0 | ||
|
|
||
| namespace Moryx.AbstractionLayer.Resources.Endpoints; | ||
|
|
||
| /// <summary> | ||
| /// Parameters used for paging requests. | ||
| /// </summary> | ||
| public sealed class PaginationParameters | ||
| { | ||
| /// <summary> | ||
| /// The page number for a query returning a list of items; default to 1 | ||
| /// </summary> | ||
| public int PageNumber { get; set => field = Math.Max(value, 1); } = 1; | ||
|
|
||
| /// <summary> | ||
| /// The number of items provided on the page; defaults to 20 with a minimum of 0 and a maximum of 100 | ||
| /// </summary> | ||
| public int PageSize { get; set => field = Math.Min(Math.Max(value, 0), 100); } = 20; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure about numbers. I think common values are 25, 50, 100, 200 (or 250?). While the max value depends on the use case, I'd tend to go for 200 and would also use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, should be 1 and 200 (or something reasonable that also works in the tree structures we have) 👍 |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // Copyright (c) 2025, Phoenix Contact GmbH & Co. KG | ||
| // Licensed under the Apache License, Version 2.0 | ||
|
|
||
| namespace Moryx.AbstractionLayer.Resources.Endpoints; | ||
|
|
||
| /// <summary> | ||
| /// Extension methods for handling pagination | ||
| /// </summary> | ||
| public static class PaginationParametersExtensions | ||
| { | ||
| extension(PaginationParameters pagination) | ||
1nf0rmagician marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| /// <summary> | ||
| /// Calculates the number of items to be skipped with the request | ||
| /// </summary> | ||
| public int Skip() => (pagination.PageNumber - 1) * pagination.PageSize; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // Copyright (c) 2025, Phoenix Contact GmbH & Co. KG | ||
| // Licensed under the Apache License, Version 2.0 | ||
|
|
||
| using Microsoft.AspNetCore.Mvc.Abstractions; | ||
| using Microsoft.AspNetCore.Mvc.ActionConstraints; | ||
| using Microsoft.AspNetCore.Routing; | ||
|
|
||
| namespace Moryx.AbstractionLayer.Resources.Endpoints; | ||
|
|
||
| /// <summary> | ||
| /// The attribute controls the selection of an action method based on the existance of a query parameter <paramref name="paramName"/>. | ||
| /// </summary> | ||
| /// <param name="paramName">The required query parameter to mark this method as valid for selection</param> | ||
| public class RequiresQueryParameterAttribute(string paramName) : ActionMethodSelectorAttribute | ||
| { | ||
| /// <inheritdoc /> | ||
| public override bool IsValidForRequest(RouteContext routeContext, ActionDescriptor action) => | ||
| routeContext.HttpContext.Request.Query.ContainsKey(paramName); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,21 @@ | ||
| // Copyright (c) 2025, Phoenix Contact GmbH & Co. KG | ||
| // Licensed under the Apache License, Version 2.0 | ||
|
|
||
| using System.ComponentModel.DataAnnotations; | ||
| using System.Net; | ||
| using System.Reflection; | ||
| using System.Runtime.Serialization; | ||
| using Microsoft.AspNetCore.Authorization; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Mvc; | ||
| using Microsoft.AspNetCore.Mvc.ModelBinding; | ||
| using Moryx.AbstractionLayer.Activities; | ||
| using Moryx.AbstractionLayer.Resources.Endpoints.Properties; | ||
| using Moryx.Asp.Extensions; | ||
| using Moryx.Configuration; | ||
| using Moryx.Runtime.Modules; | ||
| using Moryx.Serialization; | ||
| using Moryx.Tools; | ||
| using Moryx.Runtime.Modules; | ||
| using Moryx.Configuration; | ||
| using System.Runtime.Serialization; | ||
| using System.ComponentModel.DataAnnotations; | ||
| using Moryx.AbstractionLayer.Resources.Endpoints.Properties; | ||
|
|
||
| namespace Moryx.AbstractionLayer.Resources.Endpoints | ||
| { | ||
|
|
@@ -68,11 +69,14 @@ public ActionResult<ResourceModel[]> GetDetailsBatch([FromQuery] long[] ids) | |
| } | ||
|
|
||
| [HttpPost] | ||
| [InvalidQueryParameter(nameof(PaginationParameters.PageNumber))] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status417ExpectationFailed)] | ||
| [Route("query")] | ||
| [Authorize(Policy = ResourcePermissions.CanViewTree)] | ||
| public ActionResult<ResourceModel[]> GetResources(ResourceQuery query) | ||
| public ActionResult<ResourceModel[]> GetResources(ResourceQuery query) => QueryResourceModels(query); | ||
|
|
||
| private ResourceModel[] QueryResourceModels(ResourceQuery query) | ||
| { | ||
| var filter = new ResourceQueryFilter(query, _resourceTypeTree); | ||
| var resourceProxies = _resourceManagement.GetAllResources<IResource>(r => filter.Match(r as Resource)).ToArray(); | ||
|
|
@@ -82,6 +86,17 @@ public ActionResult<ResourceModel[]> GetResources(ResourceQuery query) | |
| return values; | ||
| } | ||
|
|
||
| [HttpPost("query")] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we put the query parameters to the query instead of the request body and make these
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not completely sure, but as far as I understand no, because the body is a deep object 🤔 |
||
| [RequiresQueryParameter(nameof(PaginationParameters.PageNumber))] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it necessary to require a page number? Isn't
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, could go for page size. We just need a parameter here for the minor conform change. I would drop the duplicate action (and the corresponding attributes) in the next major then 🙂
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some Ideas:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good ideas:
|
||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [Authorize(Policy = ResourcePermissions.CanViewTree)] | ||
| public ActionResult<PagedResult<ResourceModel>> GetResources([FromBody] ResourceQuery query, [FromQuery] PaginationParameters parameters) | ||
| { | ||
| var resourceModels = QueryResourceModels(query); | ||
| return new PagedResult<ResourceModel>().With(parameters).Of(resourceModels); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, I see this MR is right now rather defining the API and doesn't have any further logic yet.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes exactly, up till now I just wanted to prove that we can make it Minor conform. Nevertheless, I agree the metadata should be updated according to the actual content 🙂 |
||
| } | ||
|
|
||
| [HttpGet] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be possible to extend a paged result with other meta data? For example errors instead of items?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For errors I would prefer to commit to using the RFC7807 standard conform
ProblemDetailsfrom now on, I found a nice blog post explaining it and I would definitely prefer it to any MORYX custom error API responses 🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that! Regarding my comment: Erros might have been a bad example. I just want to make sure that there is absolutely no reason to derive from the PagedResult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsealing the class is a lot easier than the other way around, thats why i sealed it for now 😅