Add cross-cutting Watermark, Stamp, Rotation, and Split options#73
Merged
Add cross-cutting Watermark, Stamp, Rotation, and Split options#73
Conversation
Introduce four new cross-cutting facets on BuildRequestBase, available on all request types: RotationOptions, SplitOptions, WatermarkOptions, and StampOptions. Add DDD value objects: RotationAngle (90/180/270), PageRanges (validated format like "1-3,5"), OverlaySource enum (text/image/pdf), and SplitMode enum (intervals/pages). All enforce domain constraints at creation time. Wire into BaseBuilder with SetRotationOptions, SetSplitOptions, SetWatermarkOptions, and SetStampOptions fluent builder methods.
Add console example demonstrating watermark and rotation options. Add README sections for watermark/rotation and split features.
Contributor
|
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 (5)
📝 WalkthroughWalkthroughThis pull request introduces fluent builder methods and request facet classes for four cross-cutting PDF modification features: rotation, split, watermark, and stamp. New builder classes ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
examples/WatermarkAndRotate/Program.cs (2)
53-58: Consider checkingresponsebefore writing to file.If
HtmlToPdfAsyncreturnsnullor the request fails,CopyToAsyncwill throw. Adding a null check or try-catch would make the example more robust for users copying this pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/WatermarkAndRotate/Program.cs` around lines 53 - 58, The code assumes sharpClient.HtmlToPdfAsync(request) always returns a non-null stream; add a null-check and error handling before calling response.CopyToAsync: verify the returned response from HtmlToPdfAsync is not null, and wrap the file write (using resultPath and destinationStream) in a try-catch to log or rethrow a meaningful error if the request failed or the stream is null; ensure you dispose/close response and destinationStream properly in the error path.
25-34: Potential double-disposal ofHttpClientHandlerwhen usingBasicAuthHandler.When
authHandleris created (lines 26-28), it takes ownership ofhandlerviaInnerHandler = handler. WhenauthHandleris disposed, it will disposehandleras its inner handler. The separateusing var handlerdeclaration will then attempt to dispose an already-disposed object.While
HttpClientHandler.Dispose()is typically idempotent, the pattern is fragile. Consider restructuring to avoid the double-dispose:Suggested fix
- using var handler = new HttpClientHandler(); - using var authHandler = !string.IsNullOrWhiteSpace(options.BasicAuthUsername) && !string.IsNullOrWhiteSpace(options.BasicAuthPassword) - ? new BasicAuthHandler(options.BasicAuthUsername, options.BasicAuthPassword) { InnerHandler = handler } - : null; - - using var httpClient = new HttpClient(authHandler ?? (HttpMessageHandler)handler) + var handler = new HttpClientHandler(); + HttpMessageHandler effectiveHandler = handler; + + if (!string.IsNullOrWhiteSpace(options.BasicAuthUsername) && !string.IsNullOrWhiteSpace(options.BasicAuthPassword)) + { + effectiveHandler = new BasicAuthHandler(options.BasicAuthUsername, options.BasicAuthPassword) { InnerHandler = handler }; + } + + using var httpClient = new HttpClient(effectiveHandler, disposeHandler: true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/WatermarkAndRotate/Program.cs` around lines 25 - 34, The current pattern double-disposes HttpClientHandler because handler is declared with "using var handler" and also assigned as InnerHandler of BasicAuthHandler (authHandler) which will dispose it; fix by only disposing the outermost handler: remove the separate using on HttpClientHandler and instead create handler as a plain variable and wrap only the final top-level HttpMessageHandler/HttpClient in a using; alternatively, instantiate either BasicAuthHandler or the raw HttpClientHandler inside a single using block so only the top-level handler (authHandler or handler) is disposed when constructing HttpClient (refer to HttpClientHandler, BasicAuthHandler, authHandler, handler, and HttpClient).test/GotenbergSharpClient.Tests/CrossCuttingOptionsTests.cs (2)
166-171: Consider using async/await instead of.Result.Using
.Resulton async methods can cause deadlocks in certain synchronization contexts. While this is generally safe in unit tests without a synchronization context, the idiomatic approach would be to make the test method async.♻️ Suggested refactor to use async pattern
[Test] -public void RotationOptions_SerializesCorrectly() +public async Task RotationOptions_SerializesCorrectly() { var options = new RotationOptions { RotateAngle = RotationAngle.Degrees90, RotatePages = PageRanges.Create("1-3") }; var httpContents = options.ToHttpContent().ToList(); - httpContents.FirstOrDefault(c => - c.Headers.ContentDisposition?.Name == "rotateAngle")! - .ReadAsStringAsync().Result.Should().Be("90"); - httpContents.FirstOrDefault(c => - c.Headers.ContentDisposition?.Name == "rotatePages")! - .ReadAsStringAsync().Result.Should().Be("1-3"); + var angleContent = httpContents.FirstOrDefault(c => + c.Headers.ContentDisposition?.Name == "rotateAngle")!; + (await angleContent.ReadAsStringAsync()).Should().Be("90"); + + var pagesContent = httpContents.FirstOrDefault(c => + c.Headers.ContentDisposition?.Name == "rotatePages")!; + (await pagesContent.ReadAsStringAsync()).Should().Be("1-3"); }Apply similar changes to other serialization tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/GotenbergSharpClient.Tests/CrossCuttingOptionsTests.cs` around lines 166 - 171, The test currently blocks on async reads using ReadAsStringAsync().Result; change the test method (e.g., in CrossCuttingOptionsTests) to async Task and replace .Result calls with await ReadAsStringAsync() for the two assertions that inspect httpContents entries with ContentDisposition.Name "rotateAngle" and "rotatePages"; update the assertions to await the string results before calling Should().Be("90") and Should().Be("1-3"), and apply the same async/await pattern to other serialization tests that use ReadAsStringAsync().Result.
240-254: Consider marking integration tests with a category attribute.These tests require a running Gotenberg instance at
localhost:3000and will fail in CI environments without one. Adding a[Category("Integration")]or[Explicit]attribute would allow these to be excluded from standard unit test runs.♻️ Suggested addition of category attribute
+[Category("Integration")] [Test] public async Task HtmlToPdf_WithRotation_Succeeds() {Apply similar changes to
HtmlToPdf_WithWatermark_Succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/GotenbergSharpClient.Tests/CrossCuttingOptionsTests.cs` around lines 240 - 254, The two integration tests HtmlToPdf_WithRotation_Succeeds and HtmlToPdf_WithWatermark_Succeeds are currently runnable in CI and require a Gotenberg instance; mark them to be excluded from standard unit runs by adding a test attribute such as [Category("Integration")] or [Explicit] above each NUnit [Test] declaration (i.e., annotate the HtmlToPdf_WithRotation_Succeeds and HtmlToPdf_WithWatermark_Succeeds methods) so test runners can filter or skip them when integration services are unavailable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/WatermarkAndRotate/Program.cs`:
- Around line 53-58: The code assumes sharpClient.HtmlToPdfAsync(request) always
returns a non-null stream; add a null-check and error handling before calling
response.CopyToAsync: verify the returned response from HtmlToPdfAsync is not
null, and wrap the file write (using resultPath and destinationStream) in a
try-catch to log or rethrow a meaningful error if the request failed or the
stream is null; ensure you dispose/close response and destinationStream properly
in the error path.
- Around line 25-34: The current pattern double-disposes HttpClientHandler
because handler is declared with "using var handler" and also assigned as
InnerHandler of BasicAuthHandler (authHandler) which will dispose it; fix by
only disposing the outermost handler: remove the separate using on
HttpClientHandler and instead create handler as a plain variable and wrap only
the final top-level HttpMessageHandler/HttpClient in a using; alternatively,
instantiate either BasicAuthHandler or the raw HttpClientHandler inside a single
using block so only the top-level handler (authHandler or handler) is disposed
when constructing HttpClient (refer to HttpClientHandler, BasicAuthHandler,
authHandler, handler, and HttpClient).
In `@test/GotenbergSharpClient.Tests/CrossCuttingOptionsTests.cs`:
- Around line 166-171: The test currently blocks on async reads using
ReadAsStringAsync().Result; change the test method (e.g., in
CrossCuttingOptionsTests) to async Task and replace .Result calls with await
ReadAsStringAsync() for the two assertions that inspect httpContents entries
with ContentDisposition.Name "rotateAngle" and "rotatePages"; update the
assertions to await the string results before calling Should().Be("90") and
Should().Be("1-3"), and apply the same async/await pattern to other
serialization tests that use ReadAsStringAsync().Result.
- Around line 240-254: The two integration tests HtmlToPdf_WithRotation_Succeeds
and HtmlToPdf_WithWatermark_Succeeds are currently runnable in CI and require a
Gotenberg instance; mark them to be excluded from standard unit runs by adding a
test attribute such as [Category("Integration")] or [Explicit] above each NUnit
[Test] declaration (i.e., annotate the HtmlToPdf_WithRotation_Succeeds and
HtmlToPdf_WithWatermark_Succeeds methods) so test runners can filter or skip
them when integration services are unavailable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3f97fb1-d3ea-4b8c-ba6c-3d4380311f46
📒 Files selected for processing (20)
README.mdexamples/WatermarkAndRotate/Program.csexamples/WatermarkAndRotate/WatermarkAndRotate.csprojsrc/Gotenberg.Sharp.Api.Client/Domain/Builders/BaseBuilder.cssrc/Gotenberg.Sharp.Api.Client/Domain/Builders/Faceted/RotationOptionsBuilder.cssrc/Gotenberg.Sharp.Api.Client/Domain/Builders/Faceted/SplitOptionsBuilder.cssrc/Gotenberg.Sharp.Api.Client/Domain/Builders/Faceted/StampOptionsBuilder.cssrc/Gotenberg.Sharp.Api.Client/Domain/Builders/Faceted/WatermarkOptionsBuilder.cssrc/Gotenberg.Sharp.Api.Client/Domain/Requests/BuildRequestBase.cssrc/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/FacetBase.cssrc/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/RotationOptions.cssrc/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/SplitOptions.cssrc/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/StampOptions.cssrc/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/WatermarkOptions.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/OverlaySource.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/PageRanges.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/RotationAngle.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/SplitMode.cssrc/Gotenberg.Sharp.Api.Client/Infrastructure/Constants.cstest/GotenbergSharpClient.Tests/CrossCuttingOptionsTests.cs
- Fix double-dispose of HttpClientHandler in example (use disposeHandler: true)
- Switch serialization tests from .Result to async/await
- Add [Category("Integration")] to integration tests for CI filtering
Resolve conflicts in README.md (keep both cross-cutting and standalone PDF sections) and PageRanges.cs (keep semantic validation from develop).
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
BuildRequestBase(available on all request types):RotationOptions,SplitOptions,WatermarkOptions,StampOptionsRotationAngle(90/180/270),PageRanges(regex-validated "1-3,5"),OverlaySourceenum (text/image/pdf),SplitModeenum (intervals/pages)BaseBuilderwithSetRotationOptions,SetSplitOptions,SetWatermarkOptions,SetStampOptionsfluent methodsSetTextWatermark,SetTextStamp,SplitByIntervals,SplitByPagesTest plan
Summary by CodeRabbit
New Features
Documentation
Tests