refactor(pagination): move HATEOAS link generator to application layer#58
Merged
Conversation
Invert dependency so the link generator lives in BBT.Aether.Application and AspNetCore only supplies the transport adapter: - Add IPaginationContext abstraction (BaseUrl, QueryParameters, IsAvailable) with NullPaginationContext as the non-HTTP default. - Add HttpPaginationContext in AspNetCore, backed by IHttpContextAccessor and X-Forwarded-* headers; replaces the null context via Replace(). - Move IPaginationLinkGenerator and the link-building algorithm into Application; expose it on ApplicationService for shared use across hosts. - Replace the implicit null/empty baseUrl convention with explicit fluent factories: Relative() for root-relative links, WithBaseUrl() for an absolute override; FixedBaseUrlPaginationLinkGenerator decorator reuses the core builder so URL composition stays in one place. - Always emit a leading '/' between base URL and route, and trim trailing '/' from caller-supplied base URLs to avoid '//'. BREAKING CHANGE: BBT.Aether.AspNetCore.Pagination.IPaginationLinkGenerator and PaginationLinkGenerator are removed. Consumers must reference BBT.Aether.Application.Pagination.IPaginationLinkGenerator instead, and swap any baseUrl parameter for the Relative() / WithBaseUrl() factories.
Reviewer's GuideRefactors HATEOAS pagination so the transport-agnostic link generator and its abstractions live in the Application layer, with a new pagination context abstraction and factories for relative/absolute links, while AspNetCore only provides an HTTP-aware context adapter and updated DI wiring; removes the old AspNetCore-specific generator, making this a breaking change for consumers. Sequence diagram for HTTP pagination link generation with context overridesequenceDiagram
actor Client
participant Controller
participant ApplicationService
participant IPaginationLinkGenerator
participant HttpPaginationContext
participant IHttpContextAccessor
Client->>Controller: HTTP GET /items?page=2&pageSize=10
Controller->>ApplicationService: GetItems(request)
ApplicationService->>IPaginationLinkGenerator: Relative()
IPaginationLinkGenerator-->>ApplicationService: IPaginationLinkGenerator (FixedBaseUrlPaginationLinkGenerator)
ApplicationService->>IPaginationLinkGenerator: CreateHateoasResult(pagedList, items, routePath)
IPaginationLinkGenerator->>HttpPaginationContext: get QueryParameters
HttpPaginationContext->>IHttpContextAccessor: HttpContext
IHttpContextAccessor-->>HttpPaginationContext: HttpContext with Request
HttpPaginationContext-->>IPaginationLinkGenerator: QueryParameters (from current request)
IPaginationLinkGenerator->>IPaginationLinkGenerator: BuildLinks(baseUrl="", routePath, currentPage, pageSize, queryParams)
IPaginationLinkGenerator-->>ApplicationService: HateoasPagedResultDto
ApplicationService-->>Controller: HateoasPagedResultDto
Controller-->>Client: 200 OK with HATEOAS pagination links
Class diagram for new transport-agnostic pagination abstractionsclassDiagram
direction LR
class IPaginationContext {
+bool IsAvailable
+string BaseUrl
+IReadOnlyList~KeyValuePair~string,string?~~ QueryParameters
}
class NullPaginationContext {
+static NullPaginationContext Instance
+bool IsAvailable
+string BaseUrl
+IReadOnlyList~KeyValuePair~string,string?~~ QueryParameters
}
class HttpPaginationContext {
-IHttpContextAccessor httpContextAccessor
+bool IsAvailable
+string BaseUrl
+IReadOnlyList~KeyValuePair~string,string?~~ QueryParameters
-string? GetForwardedScheme(HttpRequest request)
-string? GetForwardedHost(HttpRequest request)
}
class IPaginationLinkGenerator {
+IPaginationLinkGenerator Relative()
+IPaginationLinkGenerator WithBaseUrl(string baseUrl)
+PaginationLinks GenerateLinks~T~(HateoasPagedList~T~ pagedList, string routePath)
+PaginationLinks GenerateLinks~T~(PagedList~T~ pagedList, string routePath)
+HateoasPagedResultDto~TDto~ CreateHateoasResult~TEntity,TDto~(HateoasPagedList~TEntity~ pagedList, IReadOnlyList~TDto~ items, string routePath)
+HateoasPagedResultDto~TDto~ CreateHateoasResult~TEntity,TDto~(PagedList~TEntity~ pagedList, IReadOnlyList~TDto~ items, string routePath)
}
class PaginationLinkGenerator {
-IPaginationContext context
-static HashSet~string~ PaginationKeys
+PaginationLinkGenerator(IPaginationContext context)
+IPaginationLinkGenerator Relative()
+IPaginationLinkGenerator WithBaseUrl(string baseUrl)
+PaginationLinks GenerateLinks~T~(HateoasPagedList~T~ pagedList, string routePath)
+PaginationLinks GenerateLinks~T~(PagedList~T~ pagedList, string routePath)
+HateoasPagedResultDto~TDto~ CreateHateoasResult~TEntity,TDto~(HateoasPagedList~TEntity~ pagedList, IReadOnlyList~TDto~ items, string routePath)
+HateoasPagedResultDto~TDto~ CreateHateoasResult~TEntity,TDto~(PagedList~TEntity~ pagedList, IReadOnlyList~TDto~ items, string routePath)
-string ResolveContextBaseUrl()
+static PaginationLinks BuildLinks(string baseUrl, int currentPage, int pageSize, bool hasNext, bool hasPrevious, string routePath, IReadOnlyList~KeyValuePair~string,string?~~ queryParams)
-static string BuildPageLink(string baseUrl, string route, int page, int pageSize, IReadOnlyList~KeyValuePair~string,string?~~ queryParams)
}
class FixedBaseUrlPaginationLinkGenerator {
-IPaginationContext context
-string baseUrl
+FixedBaseUrlPaginationLinkGenerator(IPaginationContext context, string baseUrl)
+IPaginationLinkGenerator Relative()
+IPaginationLinkGenerator WithBaseUrl(string baseUrl)
+PaginationLinks GenerateLinks~T~(HateoasPagedList~T~ pagedList, string routePath)
+PaginationLinks GenerateLinks~T~(PagedList~T~ pagedList, string routePath)
+HateoasPagedResultDto~TDto~ CreateHateoasResult~TEntity,TDto~(HateoasPagedList~TEntity~ pagedList, IReadOnlyList~TDto~ items, string routePath)
+HateoasPagedResultDto~TDto~ CreateHateoasResult~TEntity,TDto~(PagedList~TEntity~ pagedList, IReadOnlyList~TDto~ items, string routePath)
}
class ApplicationService {
#IPaginationLinkGenerator PaginationLinkGenerator
}
IPaginationContext <|.. NullPaginationContext
IPaginationContext <|.. HttpPaginationContext
IPaginationLinkGenerator <|.. PaginationLinkGenerator
IPaginationLinkGenerator <|.. FixedBaseUrlPaginationLinkGenerator
PaginationLinkGenerator o-- IPaginationContext
FixedBaseUrlPaginationLinkGenerator o-- IPaginationContext
ApplicationService o-- IPaginationLinkGenerator
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis change relocates pagination abstractions from AspNetCore to the Application layer by introducing a transport-agnostic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The XML doc on ApplicationService.PaginationLinkGenerator still refers to overriding behavior by passing a baseUrl argument, which no longer exists on the public API; update this comment to describe using Relative()/WithBaseUrl instead.
- The documentation for IPaginationLinkGenerator.Relative suggests route-only links like "users?...", but BuildPageLink always prepends "/" (root-relative); consider aligning the comment and examples with the actual leading-slash behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The XML doc on ApplicationService.PaginationLinkGenerator still refers to overriding behavior by passing a baseUrl argument, which no longer exists on the public API; update this comment to describe using Relative()/WithBaseUrl instead.
- The documentation for IPaginationLinkGenerator.Relative suggests route-only links like "users?...", but BuildPageLink always prepends "/" (root-relative); consider aligning the comment and examples with the actual leading-slash behavior.
## Individual Comments
### Comment 1
<location path="framework/src/BBT.Aether.Application/BBT/Aether/Application/Pagination/PaginationLinkGenerator.cs" line_range="106-118" />
<code_context>
+ // (e.g. when callers pass "https://x.com/" via WithBaseUrl).
+ sb.Append(baseUrl.AsSpan().TrimEnd('/'));
+
+ if (!string.IsNullOrEmpty(route))
+ {
+ // Always separate base URL from route with '/'. When baseUrl is empty,
+ // this produces a root-relative link (e.g. "/users?...") which browsers
+ // and HTTP clients resolve consistently regardless of current path.
+ sb.Append('/');
+ sb.Append(route);
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Relative links are root-relative (start with '/') rather than pure route-only as the XML docs suggest
When `baseUrl` is empty (e.g. via `Relative()`), the generator still prefixes routes with `'/'`, so `"users"` becomes `"/users?..."` rather than `"users?..."`. The XML docs for `Relative()` and `IPaginationLinkGenerator` describe route-only links, which doesn’t match this behavior. Please either drop the leading `'/'` when `baseUrl` is empty, or update the XML docs/naming to clarify that `Relative()` returns root-relative links.
```suggestion
var sb = new StringBuilder();
// Trim a trailing '/' from baseUrl so we never produce '//' between base and route
// (e.g. when callers pass "https://x.com/" via WithBaseUrl).
var trimmedBaseUrl = baseUrl.AsSpan().TrimEnd('/');
if (trimmedBaseUrl.Length > 0)
{
sb.Append(trimmedBaseUrl);
}
if (!string.IsNullOrEmpty(route))
{
// When a non-empty base URL is present, separate it from the route with '/'.
// When baseUrl is empty (e.g. Relative()), emit a route-only link ("users?..."),
// matching the XML documentation for IPaginationLinkGenerator.
if (sb.Length > 0)
{
sb.Append('/');
}
sb.Append(route);
}
```
</issue_to_address>
### Comment 2
<location path="framework/src/BBT.Aether.AspNetCore/BBT/Aether/AspNetCore/Pagination/HttpPaginationContext.cs" line_range="63-71" />
<code_context>
+ }
+ }
+
+ private static string? GetForwardedScheme(HttpRequest request)
+ {
+ if (request.Headers.TryGetValue("X-Forwarded-Proto", out var proto) &&
+ !string.IsNullOrEmpty(proto))
+ {
+ return proto.ToString().Split(',')[0].Trim();
+ }
+
+ return null;
+ }
+
</code_context>
<issue_to_address>
**🚨 issue (security):** Using X-Forwarded-* headers directly can be unsafe if proxies are not trusted or not consistently configured
`BaseUrl` currently prefers `X-Forwarded-Proto` / `X-Forwarded-Host` whenever present. In environments where the app is also directly reachable or the proxy isn’t fully trusted/sanitizing, clients can spoof these headers and affect generated links (e.g., host/scheme in pagination or Location-like URLs). If this helper is intended to be reusable, consider making use of `X-Forwarded-*` opt-in or wiring it to existing trust configuration (e.g., `ForwardedHeadersOptions` / `UseForwardedHeaders`) so these headers are only honored when explicitly trusted.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+106
to
+118
| var sb = new StringBuilder(); | ||
| // Trim a trailing '/' from baseUrl so we never produce '//' between base and route | ||
| // (e.g. when callers pass "https://x.com/" via WithBaseUrl). | ||
| sb.Append(baseUrl.AsSpan().TrimEnd('/')); | ||
|
|
||
| if (!string.IsNullOrEmpty(route)) | ||
| { | ||
| // Always separate base URL from route with '/'. When baseUrl is empty, | ||
| // this produces a root-relative link (e.g. "/users?...") which browsers | ||
| // and HTTP clients resolve consistently regardless of current path. | ||
| sb.Append('/'); | ||
| sb.Append(route); | ||
| } |
There was a problem hiding this comment.
suggestion: Relative links are root-relative (start with '/') rather than pure route-only as the XML docs suggest
When baseUrl is empty (e.g. via Relative()), the generator still prefixes routes with '/', so "users" becomes "/users?..." rather than "users?...". The XML docs for Relative() and IPaginationLinkGenerator describe route-only links, which doesn’t match this behavior. Please either drop the leading '/' when baseUrl is empty, or update the XML docs/naming to clarify that Relative() returns root-relative links.
Suggested change
| var sb = new StringBuilder(); | |
| // Trim a trailing '/' from baseUrl so we never produce '//' between base and route | |
| // (e.g. when callers pass "https://x.com/" via WithBaseUrl). | |
| sb.Append(baseUrl.AsSpan().TrimEnd('/')); | |
| if (!string.IsNullOrEmpty(route)) | |
| { | |
| // Always separate base URL from route with '/'. When baseUrl is empty, | |
| // this produces a root-relative link (e.g. "/users?...") which browsers | |
| // and HTTP clients resolve consistently regardless of current path. | |
| sb.Append('/'); | |
| sb.Append(route); | |
| } | |
| var sb = new StringBuilder(); | |
| // Trim a trailing '/' from baseUrl so we never produce '//' between base and route | |
| // (e.g. when callers pass "https://x.com/" via WithBaseUrl). | |
| var trimmedBaseUrl = baseUrl.AsSpan().TrimEnd('/'); | |
| if (trimmedBaseUrl.Length > 0) | |
| { | |
| sb.Append(trimmedBaseUrl); | |
| } | |
| if (!string.IsNullOrEmpty(route)) | |
| { | |
| // When a non-empty base URL is present, separate it from the route with '/'. | |
| // When baseUrl is empty (e.g. Relative()), emit a route-only link ("users?..."), | |
| // matching the XML documentation for IPaginationLinkGenerator. | |
| if (sb.Length > 0) | |
| { | |
| sb.Append('/'); | |
| } | |
| sb.Append(route); | |
| } |
Comment on lines
+63
to
+71
| private static string? GetForwardedScheme(HttpRequest request) | ||
| { | ||
| if (request.Headers.TryGetValue("X-Forwarded-Proto", out var proto) && | ||
| !string.IsNullOrEmpty(proto)) | ||
| { | ||
| return proto.ToString().Split(',')[0].Trim(); | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
🚨 issue (security): Using X-Forwarded-* headers directly can be unsafe if proxies are not trusted or not consistently configured
BaseUrl currently prefers X-Forwarded-Proto / X-Forwarded-Host whenever present. In environments where the app is also directly reachable or the proxy isn’t fully trusted/sanitizing, clients can spoof these headers and affect generated links (e.g., host/scheme in pagination or Location-like URLs). If this helper is intended to be reusable, consider making use of X-Forwarded-* opt-in or wiring it to existing trust configuration (e.g., ForwardedHeadersOptions / UseForwardedHeaders) so these headers are only honored when explicitly trusted.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the HATEOAS pagination link generation to be transport-agnostic by moving the core logic from the AspNetCore layer to the Application layer. It introduces an IPaginationContext abstraction, allowing the generator to resolve base URLs and query parameters from various sources, such as HTTP requests or background workers. Feedback provided focuses on improving the robustness of the PaginationLinkGenerator by ensuring that potential null values for routePath and baseUrl are handled safely to prevent runtime exceptions.
| string routePath, | ||
| IReadOnlyList<KeyValuePair<string, string?>> queryParams) | ||
| { | ||
| var route = routePath.TrimStart('/'); |
There was a problem hiding this comment.
routePath.TrimStart('/') will throw a NullReferenceException if routePath is null. While this parameter is typically provided as a string literal, it is safer to handle potential null values to avoid runtime errors.
var route = (routePath ?? string.Empty).TrimStart('/');
Comment on lines
+106
to
+109
| var sb = new StringBuilder(); | ||
| // Trim a trailing '/' from baseUrl so we never produce '//' between base and route | ||
| // (e.g. when callers pass "https://x.com/" via WithBaseUrl). | ||
| sb.Append(baseUrl.AsSpan().TrimEnd('/')); |
There was a problem hiding this comment.
This block can be optimized for performance and robustness. Initializing the StringBuilder with an estimated capacity reduces reallocations. Additionally, baseUrl.AsSpan() will throw if baseUrl is null; using a null-coalescing operator ensures the generator remains resilient even if the transport context returns a null base URL.
var sb = new StringBuilder((baseUrl?.Length ?? 0) + (route?.Length ?? 0) + 128);
// Trim a trailing '/' from baseUrl so we never produce '//' between base and route
// (e.g. when callers pass "https://x.com/" via WithBaseUrl).
sb.Append((baseUrl ?? string.Empty).AsSpan().TrimEnd('/'));
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 24 |
| Duplication | 4 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BBT.Aether.Application, whileBBT.Aether.AspNetCoreonly supplies the HTTP transport adapter.IPaginationContextso the link-building algorithm stays transport-agnostic and reusable across hosts (HTTP, workers, CLI, tests).null/ emptybaseUrlconvention with explicit fluent factories —Relative()for root-relative links andWithBaseUrl()for an absolute override./between base URL and route, and trim a trailing/from caller-supplied base URLs to avoid//.Changes
BBT/Aether/Application/Pagination/IPaginationContext.csBBT/Aether/Application/Pagination/NullPaginationContext.csBBT/Aether/Application/Pagination/IPaginationLinkGenerator.csBBT/Aether/Application/Pagination/PaginationLinkGenerator.csBBT/Aether/Application/Pagination/FixedBaseUrlPaginationLinkGenerator.cs(decorator poweringRelative()/WithBaseUrl())BBT/Aether/Application/ApplicationService.cs— exposesPaginationLinkGeneratorlazy property to all derived services.Microsoft/Extensions/DependencyInjection/AetherApplicationModuleServiceCollectionExtensions.cs— registersNullPaginationContextandPaginationLinkGeneratoras the defaults.BBT/Aether/AspNetCore/Pagination/HttpPaginationContext.cs—IHttpContextAccessor-backed adapter withX-Forwarded-Proto/X-Forwarded-Hostsupport.BBT/Aether/AspNetCore/Pagination/IPaginationLinkGenerator.csBBT/Aether/AspNetCore/Pagination/PaginationLinkGenerator.csMicrosoft/Extensions/DependencyInjection/AetherAspNetCoreModuleServiceCollectionExtensions.cs— replacesIPaginationContextwithHttpPaginationContext; the link generator itself is unchanged.Test Plan
Notes
BREAKING CHANGE: `BBT.Aether.AspNetCore.Pagination.IPaginationLinkGenerator` and `PaginationLinkGenerator` are removed. Migration steps for consumers:
Made with Cursor
Summary by Sourcery
Move HATEOAS pagination link generation into the application layer behind a transport-agnostic abstraction and adapt ASP.NET Core to supply the HTTP-specific context.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Release Notes
New Features
Refactor