v1.0.22#59
Merged
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.
refactor(pagination): move HATEOAS link generator to application layer
Reviewer's GuideIntroduce a transport-agnostic pagination link generation abstraction in the Application layer, backed by a pluggable IPaginationContext with a default null implementation and an ASP.NET Core HTTP-aware adapter, and expose it via ApplicationService and DI changes while removing the old ASP.NET Core–specific implementation. Sequence diagram for HTTP pagination link generation using the new abstractionssequenceDiagram
actor User
participant Controller
participant ApplicationService
participant IPaginationLinkGenerator
participant HttpPaginationContext
User ->> Controller: HTTP GET /items?page=2&pageSize=10
Controller ->> ApplicationService: GetItemsAsync(request)
ApplicationService ->> IPaginationLinkGenerator: CreateHateoasResult(pagedList, routePath)
IPaginationLinkGenerator ->> HttpPaginationContext: IsAvailable, BaseUrl, QueryParameters
HttpPaginationContext -->> IPaginationLinkGenerator: scheme, host, pathBase, query
IPaginationLinkGenerator -->> ApplicationService: HateoasPagedResultDto with PaginationLinks
ApplicationService -->> Controller: HateoasPagedResultDto
Controller -->> User: 200 OK with items and HATEOAS pagination links
Class diagram for new pagination abstractions and implementationsclassDiagram
direction LR
class ApplicationService {
<<abstract>>
+LazyServiceProvider LazyServiceProvider
+ILoggerFactory LoggerFactory
+ILogger Logger
+IClock Clock
+IPaginationLinkGenerator PaginationLinkGenerator
}
class IPaginationContext {
<<interface>>
+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 {
<<interface>>
+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 PaginationLinks {
+string Self
+string First
+string Next
+string Prev
}
class HateoasPagedList~T~ {
+int CurrentPage
+int PageSize
+bool HasNext
+bool HasPrevious
}
class PagedList~T~ {
+int CurrentPage
+int PageSize
+bool HasNext
+bool HasPrevious
+long TotalCount
}
class HateoasPagedResultDto~TDto~ {
+HateoasPagedResultDto(IReadOnlyList~TDto~ items, PaginationLinks links)
+IReadOnlyList~TDto~ Items
+PaginationLinks Links
}
ApplicationService --> IPaginationLinkGenerator : uses
IPaginationContext <|.. NullPaginationContext
IPaginationContext <|.. HttpPaginationContext
IPaginationLinkGenerator <|.. PaginationLinkGenerator
IPaginationLinkGenerator <|.. FixedBaseUrlPaginationLinkGenerator
PaginationLinkGenerator --> IPaginationContext : depends on
FixedBaseUrlPaginationLinkGenerator --> IPaginationContext : depends on
PaginationLinkGenerator --> PaginationLinks : creates
FixedBaseUrlPaginationLinkGenerator --> PaginationLinks : creates
PaginationLinkGenerator --> HateoasPagedList : reads
PaginationLinkGenerator --> PagedList : reads
FixedBaseUrlPaginationLinkGenerator --> HateoasPagedList : reads
FixedBaseUrlPaginationLinkGenerator --> PagedList : reads
PaginationLinkGenerator --> HateoasPagedResultDto : creates
FixedBaseUrlPaginationLinkGenerator --> HateoasPagedResultDto : creates
HateoasPagedResultDto --> PaginationLinks : aggregates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 1 issue, and left some high level feedback:
- The
IPaginationLinkGenerator.RelativeXML docs describe route-only links like"users?page=2", butBuildPageLinkwill emit root-relative links (e.g."/users?...") or even"?page=..."whenroutePathis empty; consider either adjusting the implementation or tightening the docs/parameter validation so behavior and comments are aligned. - In
HttpPaginationContext,BaseUrlsilently falls back to an empty string whenHttpContextor theRequestis unavailable; if this is unexpected in some hosts, it might be safer to expose that situation more explicitly (e.g. viaIsAvailablesemantics or logging) to make misconfiguredIHttpContextAccessorusage easier to diagnose.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `IPaginationLinkGenerator.Relative` XML docs describe route-only links like `"users?page=2"`, but `BuildPageLink` will emit root-relative links (e.g. `"/users?..."`) or even `"?page=..."` when `routePath` is empty; consider either adjusting the implementation or tightening the docs/parameter validation so behavior and comments are aligned.
- In `HttpPaginationContext`, `BaseUrl` silently falls back to an empty string when `HttpContext` or the `Request` is unavailable; if this is unexpected in some hosts, it might be safer to expose that situation more explicitly (e.g. via `IsAvailable` semantics or logging) to make misconfigured `IHttpContextAccessor` usage easier to diagnose.
## Individual Comments
### Comment 1
<location path="framework/src/BBT.Aether.Application/BBT/Aether/Application/Pagination/IPaginationLinkGenerator.cs" line_range="15-16" />
<code_context>
+public interface IPaginationLinkGenerator
+{
+ /// <summary>
+ /// Returns a generator that ignores the ambient transport context and produces
+ /// route-only (relative) links such as <c>"users?page=2&pageSize=10"</c>.
+ /// Query parameters from the context are still preserved.
+ /// </summary>
</code_context>
<issue_to_address>
**issue:** The XML comment for `Relative()` doesn’t match the actual URL shape (root-relative vs. route-only).
`BuildPageLink` always prefixes `/` when `route` is non-empty and `baseUrl` is empty, so the output is actually `"/users?page=2&pageSize=10"`. Please either update the XML doc to describe root-relative links (starting with `/`) or change `BuildPageLink` to omit the leading `/` if you truly intend route-only (no leading slash) URLs.
</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
+15
to
+16
| /// Returns a generator that ignores the ambient transport context and produces | ||
| /// route-only (relative) links such as <c>"users?page=2&pageSize=10"</c>. |
There was a problem hiding this comment.
issue: The XML comment for Relative() doesn’t match the actual URL shape (root-relative vs. route-only).
BuildPageLink always prefixes / when route is non-empty and baseUrl is empty, so the output is actually "/users?page=2&pageSize=10". Please either update the XML doc to describe root-relative links (starting with /) or change BuildPageLink to omit the leading / if you truly intend route-only (no leading slash) URLs.
![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 logic by moving it from the AspNetCore layer to the Application layer, making it transport-agnostic. It introduces IPaginationLinkGenerator and IPaginationContext abstractions, allowing for different implementations based on the host environment. Review feedback identifies a security risk in HttpPaginationContext due to manual parsing of forwarded headers, suggesting the use of standard ASP.NET Core middleware instead. Other feedback includes recommendations for null checks in the PaginationLinkGenerator constructor and defensive handling of the BaseUrl property to avoid potential NullReferenceExceptions.
Comment on lines
+63
to
+83
| 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; | ||
| } | ||
|
|
||
| private static string? GetForwardedHost(HttpRequest request) | ||
| { | ||
| if (request.Headers.TryGetValue("X-Forwarded-Host", out var host) && | ||
| !string.IsNullOrEmpty(host)) | ||
| { | ||
| return host.ToString().Split(',')[0].Trim(); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Manual parsing of X-Forwarded-Proto and X-Forwarded-Host headers is generally discouraged in ASP.NET Core. This approach can lead to Host Header Injection vulnerabilities if the application is not behind a trusted proxy, and it bypasses the security features (such as AllowedHosts and KnownProxies) provided by the standard ForwardedHeadersMiddleware.
It is recommended to rely on request.Scheme and request.Host directly. If the application is deployed behind a reverse proxy, the ForwardedHeadersMiddleware should be configured in the application's startup pipeline to correctly populate these properties.
Comment on lines
+25
to
+28
| public PaginationLinkGenerator(IPaginationContext context) | ||
| { | ||
| _context = context; | ||
| } |
There was a problem hiding this comment.
The constructor should validate that the context parameter is not null to prevent a NullReferenceException when the generator is used.
public PaginationLinkGenerator(IPaginationContext context)
{
_context = context ?? throw new ArgumentNullException(nameof(context));
}| => new(items, GenerateLinks(pagedList, routePath)); | ||
|
|
||
| private string ResolveContextBaseUrl() | ||
| => _context.IsAvailable ? _context.BaseUrl : string.Empty; |
There was a problem hiding this comment.
The BaseUrl property of IPaginationContext might return null depending on the implementation. Since BuildPageLink calls AsSpan() on the result of ResolveContextBaseUrl, a null value would cause a NullReferenceException. Ensuring a non-null string here improves robustness.
=> (_context.IsAvailable ? _context.BaseUrl : string.Empty) ?? string.Empty;
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| 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 by Sourcery
Introduce a transport-agnostic pagination link generation abstraction and move the HATEOAS pagination link generator from the ASP.NET Core layer into the application layer, with context-specific adapters.
New Features:
Enhancements: