Add strongly-typed cookie support for Gotenberg API #61
Conversation
Implemented comprehensive cookie functionality replacing the poor JObject-based implementation: - Created Cookie record with proper Gotenberg fields (name, value, domain, path, secure, httpOnly, sameSite) - Updated HtmlConversionBehaviors to use List<Cookie> instead of JObject - Added JSON serialization support in FacetBase for cookie collections - Implemented fluent builder methods: AddCookie(Cookie), AddCookie(params), AddCookies(IEnumerable<Cookie>) - Added InternalsVisibleTo attribute for test project - Created comprehensive test suite (13 tests) validating JSON serialization and HTTP content generation Resolves #59 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds cookie support for HTML-to-PDF: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Builder as HtmlConversionBehaviorBuilder
participant Ext as BuilderExtensions
participant Behaviors as HtmlConversionBehaviors
participant Facet as FacetBase
participant HTTP as Multipart/Form-Data
Dev->>Builder: AddCookie(Cookie) / SetMetadata(...)
Dev->>Ext: AddCookie(name,...) / AddCookies(list)
Ext->>Builder: delegate AddCookie(Cookie)
Builder->>Behaviors: append to Cookies list
Dev->>Behaviors: Build request -> ToHttpContent()
Behaviors->>Facet: ToHttpContent() per property
Facet->>Facet: GetValueAsInvariantCultureString -> if List<Cookie> -> JsonConvert.SerializeObject
Facet->>HTTP: add part "cookies" with JSON payload
HTTP-->>Dev: multipart/form-data ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (3)lib/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cs (4)
lib/Domain/Builders/Faceted/DocumentBuilder.cs (2)
lib/Domain/Builders/Faceted/HtmlConversionBehaviorBuilderExtensions.cs (1)
🔇 Additional comments (6)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/Domain/Requests/Facets/Cookie.cs (1)
62-66: Consider using an enum for SameSite.The
SameSiteproperty accepts any string value, but the documentation specifies only "Strict", "Lax", or "None" are valid. Using an enum would provide compile-time type safety and prevent invalid values.If you decide to add type safety, you could define an enum like:
/// <summary> /// SameSite cookie attribute values /// </summary> public enum SameSiteCookieAttribute { Strict, Lax, None }Then update the property (note: you'd need a custom JsonConverter or use
[JsonConverter(typeof(StringEnumConverter))]to serialize as strings):/// <summary> /// SameSite attribute. Accepted values: "Strict", "Lax", or "None" (optional) /// </summary> -[JsonProperty("sameSite", NullValueHandling = NullValueHandling.Ignore)] -public string? SameSite { get; init; } +[JsonProperty("sameSite", NullValueHandling = NullValueHandling.Ignore)] +[JsonConverter(typeof(StringEnumConverter))] +public SameSiteCookieAttribute? SameSite { get; init; }However, keeping it as a string provides flexibility if the Gotenberg API evolves or accepts additional values not yet documented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cs(1 hunks)lib/Domain/Requests/Facets/Cookie.cs(1 hunks)lib/Domain/Requests/Facets/FacetBase.cs(1 hunks)lib/Domain/Requests/Facets/HtmlConversionBehaviors.cs(1 hunks)lib/Gotenberg.Sharp.Api.Client.csproj(1 hunks)lib/Infrastructure/Constants.cs(1 hunks)test/GotenbergSharpClient.Tests/CookieTests.cs(1 hunks)test/GotenbergSharpClient.Tests/HtmlConversionBehaviorBuilderTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
lib/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cs (3)
lib/Domain/Requests/ChromeRequest.cs (2)
IEnumerable(24-27)ChromeRequest(18-28)lib/Extensions/RequestInterfaceExtensions.cs (2)
IEnumerable(22-26)IEnumerable(34-57)lib/Domain/Builders/BaseChromiumBuilder.cs (2)
TBuilder(86-91)TBuilder(79-84)
test/GotenbergSharpClient.Tests/CookieTests.cs (3)
lib/Infrastructure/Constants.cs (2)
Gotenberg(51-295)Headers(25-28)test/GotenbergSharpClient.Tests/HtmlConversionBehaviorBuilderTests.cs (6)
Test(12-32)Test(34-63)Test(65-86)Test(88-128)Test(130-147)Test(149-192)lib/Domain/Requests/Facets/HtmlConversionBehaviors.cs (1)
HtmlConversionBehaviors(23-103)
lib/Domain/Requests/Facets/HtmlConversionBehaviors.cs (3)
lib/Infrastructure/Constants.cs (3)
Constants(18-296)Chromium(175-294)HtmlConvert(257-285)lib/Domain/Requests/ChromeRequest.cs (1)
ChromeRequest(18-28)lib/Domain/Builders/BaseChromiumBuilder.cs (1)
TBuilder(86-91)
test/GotenbergSharpClient.Tests/HtmlConversionBehaviorBuilderTests.cs (5)
lib/Infrastructure/Constants.cs (2)
Gotenberg(51-295)Headers(25-28)test/GotenbergSharpClient.Tests/CookieTests.cs (7)
Test(10-33)Test(35-62)Test(64-108)Test(110-144)Test(146-183)Test(185-202)Test(204-223)lib/Domain/Builders/BaseChromiumBuilder.cs (2)
TBuilder(86-91)TBuilder(79-84)lib/Domain/Requests/ChromeRequest.cs (2)
Gotenberg(16-29)ChromeRequest(18-28)lib/Domain/Builders/HtmlRequestBuilder.cs (1)
HtmlRequestBuilder(18-44)
🔇 Additional comments (7)
lib/Infrastructure/Constants.cs (1)
269-269: LGTM!The
Cookiesconstant is correctly defined and appropriately placed within theHtmlConvertsection, maintaining consistency with other header constants.lib/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cs (2)
98-98: LGTM!The corrected error message now accurately reflects the parameter name being validated.
105-169: LGTM!The cookie management methods are well-implemented with:
- Proper null validation with
ArgumentNullException- Lazy initialization of the
Cookiescollection using null-coalescing assignment- Fluent builder pattern for method chaining
- Comprehensive XML documentation
- Consistent overload design with a convenient parameterized variant
The implementation aligns well with the existing builder patterns in the codebase.
lib/Domain/Requests/Facets/HtmlConversionBehaviors.cs (1)
57-61: LGTM!The
Cookiesproperty is correctly implemented with:
- Appropriate nullable type allowing optional cookie sets
- Proper
MultiFormHeaderattribute referencing the correct constant- Clear XML documentation
- Logical placement among related HTTP-related properties
The implementation is consistent with other properties in this class.
test/GotenbergSharpClient.Tests/HtmlConversionBehaviorBuilderTests.cs (1)
1-192: LGTM!Excellent test coverage for the cookie builder functionality:
- Tests all three
AddCookie/AddCookiesmethod variants- Validates both internal state and HTTP content serialization
- Covers edge cases (minimal parameters, accumulation, multiple cookies)
- Uses FluentAssertions for clear, readable assertions
- Verifies JSON structure including presence/absence of optional fields
The test suite provides strong confidence in the cookie management implementation.
lib/Domain/Requests/Facets/Cookie.cs (1)
20-67: LGTM!The
Cookierecord is well-designed with:
- Sealed record type providing value semantics and immutability
- Required properties enforcing API contracts at compile time
- Proper JSON serialization attributes with
NullValueHandling.Ignorefor clean output- Comprehensive XML documentation
- Init-only properties supporting immutable record pattern
The implementation provides a clean, type-safe API for cookie management.
test/GotenbergSharpClient.Tests/CookieTests.cs (1)
1-224: LGTM!Comprehensive test coverage for cookie serialization and HTTP content integration:
- Tests JSON serialization with required fields only vs. all fields
- Validates
NullValueHandling.Ignorebehavior (optional fields excluded when null)- Covers list serialization to JSON arrays
- Tests
HtmlConversionBehaviors.ToHttpContent()integration thoroughly- Includes edge cases: null cookies (no content), empty list (empty array)
- Uses FluentAssertions for clear assertions
The test suite provides excellent coverage of the cookie serialization pipeline from model to HTTP content.
Docstrings generation was requested by @Jaben. * #61 (comment) The following files were modified: * `lib/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cs` * `lib/Domain/Requests/Facets/FacetBase.cs`
|
Note Generated docstrings for this pull request at #62 |
global using Newtonsoft.Json; Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/Domain/Requests/Facets/FacetBase.cs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build and Push to Nuget
lib/Domain/Requests/Facets/FacetBase.cs
[error] 72-73: CS1513: } expected
🪛 GitHub Check: build (8.x)
lib/Domain/Requests/Facets/FacetBase.cs
[failure] 76-76:
{ expected
[failure] 74-74:
} expected
[failure] 74-74:
Identifier expected
[failure] 73-73:
Identifier expected
| using Newtonsoft.Json; | ||
|
|
||
| namespace Gotenberg.Sharp.API.Client.Domain.Requests.Facets | ||
| float f => f.ToString(cultureInfo), |
There was a problem hiding this comment.
Critical syntax error: using directives inside switch expression.
Lines 73-76 contain using directives and a namespace declaration inserted in the middle of the GetValueAsInvariantCultureString method's switch expression. This causes compilation failure (CS1513: } expected).
These lines must be removed from here. The using Newtonsoft.Json; directive should be added at the top of the file alongside the existing using System.Globalization; statement.
Apply this diff to fix the syntax error:
return value switch
{
LibrePdfFormats format => format.ToFormDataValue(),
ConversionPdfFormats format => format.ToFormDataValue(),
-using System.Globalization;
-using Newtonsoft.Json;
-
-namespace Gotenberg.Sharp.API.Client.Domain.Requests.Facets
float f => f.ToString(cultureInfo),
double d => d.ToString(cultureInfo),Then add the using Newtonsoft.Json; directive at the top of the file:
using System.Globalization;
+using Newtonsoft.Json;
namespace Gotenberg.Sharp.API.Client.Domain.Requests.FacetsNote: The provided diff doesn't show the actual code changes that use JsonConvert.SerializeObject (mentioned in past review comments). Please ensure those changes are present and correctly placed in the file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| using System.Globalization; | |
| using Newtonsoft.Json; | |
| namespace Gotenberg.Sharp.API.Client.Domain.Requests.Facets | |
| return value switch | |
| { | |
| LibrePdfFormats format => format.ToFormDataValue(), | |
| ConversionPdfFormats format => format.ToFormDataValue(), | |
| float f => f.ToString(cultureInfo), | |
| double d => d.ToString(cultureInfo), | |
| }; |
| using System.Globalization; | |
| using Newtonsoft.Json; | |
| namespace Gotenberg.Sharp.API.Client.Domain.Requests.Facets | |
| using System.Globalization; | |
| using Newtonsoft.Json; | |
| namespace Gotenberg.Sharp.API.Client.Domain.Requests.Facets | |
| { | |
| // … | |
| } |
🧰 Tools
🪛 GitHub Check: build (8.x)
[failure] 76-76:
{ expected
[failure] 74-74:
} expected
[failure] 74-74:
Identifier expected
[failure] 73-73:
Identifier expected
🤖 Prompt for AI Agents
In lib/Domain/Requests/Facets/FacetBase.cs around lines 73-76, there are stray
using directives and a namespace declaration injected inside the
GetValueAsInvariantCultureString method's switch expression causing a compile
error; remove those lines from inside the method and add "using
Newtonsoft.Json;" to the top of the file next to the existing "using
System.Globalization;". After moving the using, verify and place any
JsonConvert.SerializeObject usage inside the method's appropriate switch arm
(not inside the switch syntax itself), ensuring the switch returns the
serialized string or invariant-culture string as intended and that all braces
and parentheses are balanced.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cs(1 hunks)lib/Domain/Requests/Facets/Cookie.cs(1 hunks)test/GotenbergSharpClient.Tests/HtmlConversionBehaviorBuilderTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/GotenbergSharpClient.Tests/HtmlConversionBehaviorBuilderTests.cs (6)
test/GotenbergSharpClient.Tests/CookieTests.cs (7)
Test(10-33)Test(35-62)Test(64-108)Test(110-144)Test(146-183)Test(185-202)Test(204-223)lib/Domain/Requests/Facets/HtmlConversionBehaviors.cs (1)
HtmlConversionBehaviors(23-97)lib/Domain/Requests/ChromeRequest.cs (2)
Gotenberg(16-29)ChromeRequest(18-28)lib/Domain/Builders/BaseChromiumBuilder.cs (1)
TBuilder(86-91)lib/Domain/Builders/HtmlRequestBuilder.cs (1)
HtmlRequestBuilder(18-44)lib/Domain/Requests/HtmlRequest.cs (1)
HtmlRequest(27-61)
lib/Domain/Requests/Facets/Cookie.cs (2)
lib/Infrastructure/Constants.cs (1)
Gotenberg(51-295)lib/Extensions/StringExtensions.cs (1)
IsNotSet(25-28)
lib/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cs (1)
lib/Domain/Requests/Facets/Cookie.cs (1)
Validate(68-73)
| { | ||
| if (cookies == null) throw new ArgumentNullException(nameof(cookies)); | ||
|
|
||
| this._htmlConversionBehaviors.Cookies ??= new List<Cookie>(); | ||
| this._htmlConversionBehaviors.Cookies.AddRange(cookies); | ||
|
|
There was a problem hiding this comment.
Validate each cookie before adding
AddCookies bypasses the per-cookie validation you enforce in AddCookie(Cookie). A null entry or a cookie with missing required fields slips straight into the payload, leading to malformed JSON and inevitable API rejections. Iterate and delegate to AddCookie so every item goes through the same guardrails.
Apply this diff:
public HtmlConversionBehaviorBuilder AddCookies(IEnumerable<Cookie> cookies)
{
if (cookies == null) throw new ArgumentNullException(nameof(cookies));
- this._htmlConversionBehaviors.Cookies ??= new List<Cookie>();
- this._htmlConversionBehaviors.Cookies.AddRange(cookies);
+ foreach (var cookie in cookies)
+ {
+ AddCookie(cookie);
+ }
return this;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| if (cookies == null) throw new ArgumentNullException(nameof(cookies)); | |
| this._htmlConversionBehaviors.Cookies ??= new List<Cookie>(); | |
| this._htmlConversionBehaviors.Cookies.AddRange(cookies); | |
| public HtmlConversionBehaviorBuilder AddCookies(IEnumerable<Cookie> cookies) | |
| { | |
| if (cookies == null) throw new ArgumentNullException(nameof(cookies)); | |
| foreach (var cookie in cookies) | |
| { | |
| AddCookie(cookie); | |
| } | |
| return this; | |
| } |
🤖 Prompt for AI Agents
In lib/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cs around lines 165
to 170, AddCookies currently shortcuts per-cookie validation by just adding the
collection; change it to iterate the incoming cookies (after the existing null
check and initializing the Cookies list) and call the existing AddCookie(Cookie)
for each entry so every cookie is validated and null/malformed items are
rejected consistently; ensure you preserve the null-argument check and the
initialization of _htmlConversionBehaviors.Cookies before the loop.
1 participant
Implemented comprehensive cookie functionality replacing the poor JObject-based implementation:
Resolves #59
Summary by CodeRabbit
New Features
Tests
Documentation