Add missing Chromium form fields with DDD value objects#70
Merged
Conversation
Add waitForSelector, emulatedMediaFeatures, failOnHttpStatusCodes, failOnResourceHttpStatusCodes, ignoreResourceHttpStatusDomains, and failOnResourceLoadingFailed to HtmlConversionBehaviors facet. Introduce validated DDD value objects (CssSelector, EmulatedMediaFeature, HttpStatusCode, DomainName) to prevent primitive obsession and enforce domain constraints at construction time. Serialization to Gotenberg's wire format is handled internally by FacetBase.
Add console example demonstrating waitForSelector, emulated media features, failOnHttpStatusCodes, and ignoreResourceHttpStatusDomains. Add corresponding README section with concise usage example.
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR adds support for advanced Chromium HTML-to-PDF conversion features through new value objects, builder extensions, and serialization support. It introduces strongly-typed configuration for CSS selector waits, media feature emulation, and HTTP status-code-based failure handling, with complete test coverage and documentation examples. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/EmulatedMediaFeature.cs (1)
66-74: Enforce documented allowed values in convenience factories.Line 66 and Line 73 accept arbitrary non-empty strings, even though the API docs here constrain expected values. Consider validating these inputs in the factories to keep the value object truly domain-safe.
💡 Proposed refactor
public static EmulatedMediaFeature PrefersColorScheme(string scheme) => - Create("prefers-color-scheme", scheme); + scheme switch + { + "light" or "dark" => Create("prefers-color-scheme", scheme), + _ => throw new ArgumentException("Scheme must be 'light' or 'dark'.", nameof(scheme)) + }; @@ public static EmulatedMediaFeature PrefersReducedMotion(string value) => - Create("prefers-reduced-motion", value); + value switch + { + "no-preference" or "reduce" => Create("prefers-reduced-motion", value), + _ => throw new ArgumentException("Value must be 'no-preference' or 'reduce'.", nameof(value)) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/EmulatedMediaFeature.cs` around lines 66 - 74, The convenience factories PrefersColorScheme and PrefersReducedMotion currently accept any non-empty string; update them to validate inputs against the documented allowed values and throw meaningful exceptions: for PrefersColorScheme only accept "light" or "dark" (case-insensitive) and for PrefersReducedMotion only accept "no-preference" or "reduce" (case-insensitive), ensuring null/empty inputs also raise ArgumentException/ArgumentNullException; perform the checks before calling Create and include the invalid value in the exception message to keep the EmulatedMediaFeature value object domain-safe.test/GotenbergSharpClient.Tests/ChromiumMissingFieldsTests.cs (1)
155-160: Consider usingawaitinstead of.Resultfor async operations.Using
.Resultblocks the thread and can mask async issues. While acceptable in tests, usingawaitwith anasync Tasktest method would be more consistent with async patterns.♻️ Proposed async refactor
[Test] - public void WaitForSelector_SerializesToCorrectHttpContent() + public async Task WaitForSelector_SerializesToCorrectHttpContent() { var behaviors = new HtmlConversionBehaviors { WaitForSelector = CssSelector.Create("#content") }; var httpContents = behaviors.ToHttpContent().ToList(); var content = httpContents.FirstOrDefault(c => c.Headers.ContentDisposition?.Name == "waitForSelector"); content.Should().NotBeNull(); - content!.ReadAsStringAsync().Result.Should().Be("#content"); + (await content!.ReadAsStringAsync()).Should().Be("#content"); }This pattern applies to other serialization tests as well (lines 179, 204, 229, 254, 275).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/GotenbergSharpClient.Tests/ChromiumMissingFieldsTests.cs` around lines 155 - 160, Change the synchronous blocking call to an async await: make the containing test method return Task and be declared async, replace content!.ReadAsStringAsync().Result with await content!.ReadAsStringAsync(), and update other similar assertions (the other ReadAsStringAsync().Result occurrences) the same way so tests use async Task and await rather than .Result; locate the occurrences by searching for ReadAsStringAsync().Result in ChromiumMissingFieldsTests and adjust the test signatures and assertions accordingly.src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/CssSelector.cs (1)
52-52: Implicit conversion can throwNullReferenceExceptionifselectoris null.The implicit operator does not guard against a null
CssSelectorinstance. While rare in practice, passing a null value will throw.Consider adding a null check or documenting that null is not supported:
🛡️ Proposed defensive fix
- public static implicit operator string(CssSelector selector) => selector.Value; + public static implicit operator string(CssSelector selector) => selector?.Value!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/CssSelector.cs` at line 52, The implicit conversion operator CssSelector -> string can throw NullReferenceException when selector is null; update the operator (implicit operator string) to guard against null by returning null (or empty) when selector is null (e.g., check selector is null ? null : selector.Value) so calling code won’t throw; reference the CssSelector type, its implicit operator string and the Value property when making the change.src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/DomainName.cs (1)
36-42: Consider adding basic domain format validation.The factory accepts any non-whitespace string. While this provides flexibility, invalid domain names like
"not a domain!"would pass validation. If Gotenberg rejects malformed domains, the error would only surface at request time.A simple regex or check for invalid characters could provide earlier feedback. However, this is optional since Gotenberg will ultimately validate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/DomainName.cs` around lines 36 - 42, DomainName.Create currently only checks for non-whitespace input; add basic format validation to fail fast for obviously invalid hostnames (e.g., spaces or illegal characters). Inside DomainName.Create (and/or a private helper on DomainName), after trimming verify the value matches a simple hostname/domain pattern (e.g., allow letters, digits, hyphens and dots, no leading/trailing hyphen on labels, no whitespace, and a reasonable TLD/label length) or use Uri.CheckHostName/IdnMapping as a validator; if validation fails throw an ArgumentException with a clear message. Keep the existing trimming and preserve the returned new DomainName(domain) when valid.test/GotenbergSharpClient.Tests/ChromiumMissingFieldsIntegrationTests.cs (1)
8-14: Consider adding a test category attribute for integration tests.These tests require a running Gotenberg instance and won't pass in CI without one. Adding
[Category("Integration")]allows selective test execution.♻️ Proposed fix
[TestFixture] +[Category("Integration")] public class ChromiumMissingFieldsIntegrationTests { private const string GotenbergUrl = "http://localhost:3000";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/GotenbergSharpClient.Tests/ChromiumMissingFieldsIntegrationTests.cs` around lines 8 - 14, Add an NUnit Category attribute to mark these tests as integration tests: decorate the ChromiumMissingFieldsIntegrationTests class (or its test methods) with [Category("Integration")] so CI can exclude them by category; update the class declaration for ChromiumMissingFieldsIntegrationTests (the TestFixture class) to include the Category attribute.src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/HttpStatusCode.cs (1)
28-28: CustomHttpStatusCodeclass may cause namespace ambiguity withSystem.Net.HttpStatusCode.While currently no files import both namespaces, developers using this value object alongside
System.NetAPIs will require explicit namespace qualification. The new value object serves a different purpose (validating Gotenberg-specific status code ranges), but the identical name creates potential confusion.Consider renaming to
GotenbergHttpStatusCodeorHttpStatusCodeValueto clarify intent and eliminate ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/HttpStatusCode.cs` at line 28, Rename the public sealed class HttpStatusCode to a distinct name (e.g., GotenbergHttpStatusCode or HttpStatusCodeValue) to avoid confusion with System.Net.HttpStatusCode; update the class declaration (HttpStatusCode), its file name, any constructors/factory methods and all usages/references (including IEquatable and IComparable implementations and equality/compare calls) to the new identifier so callers and imports no longer clash with System.Net types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Gotenberg.Sharp.Api.Client/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cs`:
- Around line 248-251: The params overloads of SetFailOnHttpStatusCodes (the
method returning HtmlConversionBehaviorBuilder and the similar overload at the
other location) do not guard against a null array and will throw
NullReferenceException when enumerated; update those methods (e.g., the public
HtmlConversionBehaviorBuilder SetFailOnHttpStatusCodes(params int[]
statusCodes)) to validate input and throw ArgumentNullException if the incoming
statusCodes is null, or treat null as an empty array before calling the
enumerable overload (the one that accepts IEnumerable<HttpStatusCode> or
IEnumerable<int>), so callers passing null receive a deterministic
ArgumentNullException or safe behavior instead of an NRE.
- Around line 234-239: The SetFailOnHttpStatusCodes method currently checks the
enumerable for null but doesn't guard against null elements; update
HtmlConversionBehaviorBuilder.SetFailOnHttpStatusCodes to validate item-level
entries by iterating the incoming statusCodes and throwing an
ArgumentException/ArgumentNullException if any element is null (before calling
ToList()), then assign the validated list to
_htmlConversionBehaviors.FailOnHttpStatusCodes; apply the same item-level null
validation to the corresponding method shown at lines 258-263.
- Around line 307-313: AddIgnoreResourceHttpStatusDomains currently mutates
state per-iteration and is not null-safe; change it to validate the input array
and each domain first (handle null params and null/empty strings),
convert/normalize all domains (using AddIgnoreResourceHttpStatusDomain's logic
or its converter) into a temporary list, and only append to the builder's
internal collection once validation succeeds for all entries so the operation is
atomic; if any domain is invalid throw or return without partial changes. Ensure
AddIgnoreResourceHttpStatusDomains remains the public entrypoint and reuses
AddIgnoreResourceHttpStatusDomain/its conversion logic for consistency.
In `@src/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/FacetBase.cs`:
- Around line 83-84: The code in FacetBase.cs is serializing emulated media
features into a JSON object by using features.ToDictionary(...) which both
produces the wrong shape and can throw on duplicate names; change the
serialization to serialize the List<EmulatedMediaFeature> directly (e.g.,
JsonConvert.SerializeObject(features)) so the output is a JSON array of
{name,value} objects as defined by EmulatedMediaFeature and you avoid
ToDictionary’s ArgumentException.
---
Nitpick comments:
In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/CssSelector.cs`:
- Line 52: The implicit conversion operator CssSelector -> string can throw
NullReferenceException when selector is null; update the operator (implicit
operator string) to guard against null by returning null (or empty) when
selector is null (e.g., check selector is null ? null : selector.Value) so
calling code won’t throw; reference the CssSelector type, its implicit operator
string and the Value property when making the change.
In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/DomainName.cs`:
- Around line 36-42: DomainName.Create currently only checks for non-whitespace
input; add basic format validation to fail fast for obviously invalid hostnames
(e.g., spaces or illegal characters). Inside DomainName.Create (and/or a private
helper on DomainName), after trimming verify the value matches a simple
hostname/domain pattern (e.g., allow letters, digits, hyphens and dots, no
leading/trailing hyphen on labels, no whitespace, and a reasonable TLD/label
length) or use Uri.CheckHostName/IdnMapping as a validator; if validation fails
throw an ArgumentException with a clear message. Keep the existing trimming and
preserve the returned new DomainName(domain) when valid.
In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/EmulatedMediaFeature.cs`:
- Around line 66-74: The convenience factories PrefersColorScheme and
PrefersReducedMotion currently accept any non-empty string; update them to
validate inputs against the documented allowed values and throw meaningful
exceptions: for PrefersColorScheme only accept "light" or "dark"
(case-insensitive) and for PrefersReducedMotion only accept "no-preference" or
"reduce" (case-insensitive), ensuring null/empty inputs also raise
ArgumentException/ArgumentNullException; perform the checks before calling
Create and include the invalid value in the exception message to keep the
EmulatedMediaFeature value object domain-safe.
In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/HttpStatusCode.cs`:
- Line 28: Rename the public sealed class HttpStatusCode to a distinct name
(e.g., GotenbergHttpStatusCode or HttpStatusCodeValue) to avoid confusion with
System.Net.HttpStatusCode; update the class declaration (HttpStatusCode), its
file name, any constructors/factory methods and all usages/references (including
IEquatable and IComparable implementations and equality/compare calls) to the
new identifier so callers and imports no longer clash with System.Net types.
In `@test/GotenbergSharpClient.Tests/ChromiumMissingFieldsIntegrationTests.cs`:
- Around line 8-14: Add an NUnit Category attribute to mark these tests as
integration tests: decorate the ChromiumMissingFieldsIntegrationTests class (or
its test methods) with [Category("Integration")] so CI can exclude them by
category; update the class declaration for ChromiumMissingFieldsIntegrationTests
(the TestFixture class) to include the Category attribute.
In `@test/GotenbergSharpClient.Tests/ChromiumMissingFieldsTests.cs`:
- Around line 155-160: Change the synchronous blocking call to an async await:
make the containing test method return Task and be declared async, replace
content!.ReadAsStringAsync().Result with await content!.ReadAsStringAsync(), and
update other similar assertions (the other ReadAsStringAsync().Result
occurrences) the same way so tests use async Task and await rather than .Result;
locate the occurrences by searching for ReadAsStringAsync().Result in
ChromiumMissingFieldsTests and adjust the test signatures and assertions
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc112c3e-ee9d-4c0e-9afc-bf83f8306b2d
📒 Files selected for processing (17)
README.mdexamples/ChromiumFeatures/ChromiumFeatures.csprojexamples/ChromiumFeatures/Program.cssrc/Gotenberg.Sharp.Api.Client/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cssrc/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/FacetBase.cssrc/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/HtmlConversionBehaviors.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/CssSelector.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/DomainName.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/EmulatedMediaFeature.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/HttpStatusCode.cssrc/Gotenberg.Sharp.Api.Client/Infrastructure/Constants.cstest/GotenbergSharpClient.Tests/ChromiumMissingFieldsIntegrationTests.cstest/GotenbergSharpClient.Tests/ChromiumMissingFieldsTests.cstest/GotenbergSharpClient.Tests/ValueObjects/CssSelectorTests.cstest/GotenbergSharpClient.Tests/ValueObjects/DomainNameTests.cstest/GotenbergSharpClient.Tests/ValueObjects/EmulatedMediaFeatureTests.cstest/GotenbergSharpClient.Tests/ValueObjects/HttpStatusCodeTests.cs
Comment on lines
+83
to
+84
| List<EmulatedMediaFeature> features => JsonConvert.SerializeObject( | ||
| features.ToDictionary(f => f.Name, f => f.Value)), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Gotenberg v8 Chromium routes, what is the exact expected form value shape for emulatedMediaFeatures(JSON array of{name,value} objects vs JSON object map)?
💡 Result:
For Gotenberg v8 Chromium routes, the exact expected form value shape for emulatedMediaFeatures is a JSON array of {name, value} objects. It is not a JSON object map.
Citations:
- 1: https://gotenberg.dev/docs/convert-with-chromium/convert-html-to-pdf
- 2: https://sourceforge.net/projects/gotenberg.mirror/files/v8.27.0/
🏁 Script executed:
# Find and examine the EmulatedMediaFeature class
find . -name "EmulatedMediaFeature.cs" -type f | head -5Repository: ChangemakerStudios/GotenbergSharpApiClient
Length of output: 162
🏁 Script executed:
cat -n ./src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/EmulatedMediaFeature.csRepository: ChangemakerStudios/GotenbergSharpApiClient
Length of output: 3573
🏁 Script executed:
# Examine the FacetBase.cs file around lines 83-84
sed -n '75,95p' ./src/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/FacetBase.csRepository: ChangemakerStudios/GotenbergSharpApiClient
Length of output: 1184
Serialize emulatedMediaFeatures as a list, not a dictionary.
Lines 83-84 currently convert features to a dictionary via features.ToDictionary(f => f.Name, f => f.Value), which produces a JSON object instead of the required JSON array of {name, value} objects. This violates the Gotenberg v8 API contract documented in EmulatedMediaFeature.cs (line 23). Additionally, ToDictionary() throws ArgumentException on duplicate feature names, creating avoidable runtime failures.
Serialize the list directly to preserve the array structure; the class's [JsonProperty] attributes ensure correct JSON output:
💡 Proposed fix
- List<EmulatedMediaFeature> features => JsonConvert.SerializeObject(
- features.ToDictionary(f => f.Name, f => f.Value)),
+ List<EmulatedMediaFeature> features => JsonConvert.SerializeObject(features),📝 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.
Suggested change
| List<EmulatedMediaFeature> features => JsonConvert.SerializeObject( | |
| features.ToDictionary(f => f.Name, f => f.Value)), | |
| List<EmulatedMediaFeature> features => JsonConvert.SerializeObject(features), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/FacetBase.cs` around
lines 83 - 84, The code in FacetBase.cs is serializing emulated media features
into a JSON object by using features.ToDictionary(...) which both produces the
wrong shape and can throw on duplicate names; change the serialization to
serialize the List<EmulatedMediaFeature> directly (e.g.,
JsonConvert.SerializeObject(features)) so the output is a JSON array of
{name,value} objects as defined by EmulatedMediaFeature and you avoid
ToDictionary’s ArgumentException.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test/GotenbergSharpClient.Tests/ValueObjects/GotenbergStatusCodeTests.cs (1)
32-39: Consider adding a test for null implicit conversion.The
GotenbergStatusCodehas an implicit conversion that returns0for null input. Adding a test would document this behavior and catch any future changes.💡 Suggested test
[Test] public void ImplicitConversion_WithNull_ReturnsZero() { GotenbergStatusCode? nullCode = null; int result = nullCode; result.Should().Be(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/GotenbergSharpClient.Tests/ValueObjects/GotenbergStatusCodeTests.cs` around lines 32 - 39, Add a new unit test that verifies the implicit conversion of a nullable GotenbergStatusCode returns 0 when null: create a test method named ImplicitConversion_WithNull_ReturnsZero that declares GotenbergStatusCode? nullCode = null, performs the implicit conversion to int, and asserts the result is 0 using the same assertion style as the other tests (e.g., result.Should().Be(0)); place this alongside the existing ImplicitConversion_ToIntReturnsValue test to document and guard the null behavior.src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/DomainName.cs (1)
53-53: Implicit conversion returns null for null input without compile-time warning.The null-forgiving operator (
!) suppresses the nullable warning but the expressiondomain?.Valuestill evaluates tonullwhendomainis null. This means callers usingstring s = nullDomainName;get a runtime null without compiler warning. Consider either:
- Throwing
ArgumentNullExceptionfor null input (fail-fast), or- Documenting that the conversion returns null for null input
This is minor since
DomainNameinstances are typically validated before use.💡 Option: Fail-fast on null
- public static implicit operator string(DomainName domain) => domain?.Value!; + public static implicit operator string(DomainName domain) => domain?.Value + ?? throw new ArgumentNullException(nameof(domain));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/DomainName.cs` at line 53, The implicit conversion operator on DomainName currently returns domain?.Value! which yields null silently for a null DomainName; update the operator (the implicit operator string in the DomainName class) to fail-fast by checking for null and throwing an ArgumentNullException (use nameof(domain)) so callers cannot receive a null string unexpectedly; also update the operator's XML doc comment to state it throws on null input.src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/GotenbergStatusCode.cs (1)
66-66: Implicit conversion returns 0 for null, which is outside the valid range.When
codeis null, this returns0, butGotenbergStatusCode.Create(0)throwsArgumentOutOfRangeException. This asymmetry could cause confusion—a null status code silently converts to an invalid value rather than failing. Consider throwing for null input to maintain consistency with the validation invariants.💡 Option: Fail-fast on null
- public static implicit operator int(GotenbergStatusCode code) => code?.Value ?? 0; + public static implicit operator int(GotenbergStatusCode code) => code?.Value + ?? throw new ArgumentNullException(nameof(code));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/GotenbergStatusCode.cs` at line 66, The implicit conversion operator for GotenbergStatusCode returns 0 when code is null, which contradicts the validation in GotenbergStatusCode.Create(0); update the implicit operator int(GotenbergStatusCode code) to throw an ArgumentNullException (or other appropriate exception) when code is null instead of returning 0 so null inputs fail fast and remain consistent with the type's invariants; locate the operator definition in GotenbergStatusCode and replace the null-coalescing behavior with an explicit null check that throws.test/GotenbergSharpClient.Tests/ChromiumMissingFieldsIntegrationTests.cs (1)
68-82: Consider adding a negative test for failure triggering.The test verifies success when status codes are configured but doesn't verify that the failure actually triggers when appropriate. A negative test loading a page that returns a configured status code would validate the feature works end-to-end.
💡 Example negative test
[Category("Integration")] [Test] public async Task HtmlToPdf_WithFailOnHttpStatusCodes_WhenPageReturnsMatchingCode_ThrowsOrReturns409() { // This would require a test endpoint that returns a specific status code // or mocking the behavior to verify the failure path }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/GotenbergSharpClient.Tests/ChromiumMissingFieldsIntegrationTests.cs` around lines 68 - 82, Add a negative integration test that verifies the fail-on-http-status behavior actually triggers: create a new test (e.g., HtmlToPdf_WithFailOnHttpStatusCodes_WhenPageReturnsMatchingCode_Throws) that uses HtmlRequestBuilder with SetFailOnHttpStatusCodes(499, 599) and points the document to a test endpoint which returns one of those status codes (or mock the HTTP response if integration endpoint is unavailable); call _client.HtmlToPdfAsync and assert that it either throws the expected exception or returns an error/HTTP 409-like result, mirroring how failures are surfaced by HtmlToPdfAsync so the failure path of HtmlToPdf_WithFailOnHttpStatusCodes_Succeeds is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/DomainName.cs`:
- Line 53: The implicit conversion operator on DomainName currently returns
domain?.Value! which yields null silently for a null DomainName; update the
operator (the implicit operator string in the DomainName class) to fail-fast by
checking for null and throwing an ArgumentNullException (use nameof(domain)) so
callers cannot receive a null string unexpectedly; also update the operator's
XML doc comment to state it throws on null input.
In `@src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/GotenbergStatusCode.cs`:
- Line 66: The implicit conversion operator for GotenbergStatusCode returns 0
when code is null, which contradicts the validation in
GotenbergStatusCode.Create(0); update the implicit operator
int(GotenbergStatusCode code) to throw an ArgumentNullException (or other
appropriate exception) when code is null instead of returning 0 so null inputs
fail fast and remain consistent with the type's invariants; locate the operator
definition in GotenbergStatusCode and replace the null-coalescing behavior with
an explicit null check that throws.
In `@test/GotenbergSharpClient.Tests/ChromiumMissingFieldsIntegrationTests.cs`:
- Around line 68-82: Add a negative integration test that verifies the
fail-on-http-status behavior actually triggers: create a new test (e.g.,
HtmlToPdf_WithFailOnHttpStatusCodes_WhenPageReturnsMatchingCode_Throws) that
uses HtmlRequestBuilder with SetFailOnHttpStatusCodes(499, 599) and points the
document to a test endpoint which returns one of those status codes (or mock the
HTTP response if integration endpoint is unavailable); call
_client.HtmlToPdfAsync and assert that it either throws the expected exception
or returns an error/HTTP 409-like result, mirroring how failures are surfaced by
HtmlToPdfAsync so the failure path of
HtmlToPdf_WithFailOnHttpStatusCodes_Succeeds is covered.
In `@test/GotenbergSharpClient.Tests/ValueObjects/GotenbergStatusCodeTests.cs`:
- Around line 32-39: Add a new unit test that verifies the implicit conversion
of a nullable GotenbergStatusCode returns 0 when null: create a test method
named ImplicitConversion_WithNull_ReturnsZero that declares GotenbergStatusCode?
nullCode = null, performs the implicit conversion to int, and asserts the result
is 0 using the same assertion style as the other tests (e.g.,
result.Should().Be(0)); place this alongside the existing
ImplicitConversion_ToIntReturnsValue test to document and guard the null
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16fa7f24-0601-4bc1-8528-7af0e81a4f6a
📒 Files selected for processing (11)
examples/ChromiumFeatures/Program.cssrc/Gotenberg.Sharp.Api.Client/Domain/Builders/Faceted/HtmlConversionBehaviorBuilder.cssrc/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/FacetBase.cssrc/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/HtmlConversionBehaviors.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/CssSelector.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/DomainName.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/EmulatedMediaFeature.cssrc/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/GotenbergStatusCode.cstest/GotenbergSharpClient.Tests/ChromiumMissingFieldsIntegrationTests.cstest/GotenbergSharpClient.Tests/ChromiumMissingFieldsTests.cstest/GotenbergSharpClient.Tests/ValueObjects/GotenbergStatusCodeTests.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/FacetBase.cs
- examples/ChromiumFeatures/Program.cs
- src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/EmulatedMediaFeature.cs
- src/Gotenberg.Sharp.Api.Client/Domain/ValueObjects/CssSelector.cs
- test/GotenbergSharpClient.Tests/ChromiumMissingFieldsTests.cs
- src/Gotenberg.Sharp.Api.Client/Domain/Requests/Facets/HtmlConversionBehaviors.cs
- Make implicit operators on DomainName, GotenbergStatusCode, and CssSelector throw ArgumentNullException instead of silently returning null/0 — consistent with DDD validation philosophy - Add null implicit conversion test for GotenbergStatusCode - Add negative integration test verifying failOnResourceLoadingFailed triggers GotenbergApiException when resources fail to load
Use invalid.test (NXDOMAIN) instead of 192.0.2.1 (black-hole IP) for the failOnResourceLoadingFailed negative test. The .test TLD resolves to NXDOMAIN immediately, avoiding the 3-minute TCP timeout that was failing CI.
Resolve conflicts in README.md (keep chromium features section alongside develop's watermark/split/standalone sections) and FacetBase.cs (keep both chromium and cross-cutting type serialization cases).
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
waitForSelector,emulatedMediaFeatures,failOnHttpStatusCodes,failOnResourceHttpStatusCodes,ignoreResourceHttpStatusDomains, andfailOnResourceLoadingFailedtoHtmlConversionBehaviorsCssSelector,EmulatedMediaFeature,HttpStatusCode,DomainName— enforce domain constraints at creation timeFacetBaseTest plan
Summary by CodeRabbit
New Features
Documentation