feat(api): introduce DocumentRequest and ProcessedDocumentResponse sealed base classes - #634
Conversation
040a6fc to
a3792be
Compare
|
@ThomasVitale this is what the implementation of #632 would look like. WDYT? |
There was a problem hiding this comment.
Pull request overview
Introduces a new sealed abstract DocumentRequest base type in docling-serve-api to consolidate shared sources and target request fields across convert, batch-convert, and chunk request models, enabling polymorphic handling of these request types by consumers.
Changes:
- Added
ai.docling.serve.api.request.DocumentRequest(sealed) with sharedsourcesand optionaltarget. - Updated
ConvertDocumentRequest,BatchConvertDocumentRequest, andChunkDocumentRequestto extendDocumentRequest(withtoString(callSuper = true)). - Added
DocumentRequestTestscovering the sealed hierarchy and inherited field behavior; updated module exports and “What’s New” docs accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/src/doc/docs/whats-new.md | Adds release note entry for the new DocumentRequest base type and polymorphic usage. |
| docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java | New unit tests validating the sealed hierarchy, inheritance, and dispatch behavior. |
| docling-serve/docling-serve-api/src/main/java/module-info.java | Exports the new ai.docling.serve.api.request package. |
| docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/request/package-info.java | Marks the new package as @NullMarked. |
| docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/request/DocumentRequest.java | New sealed base request type with shared sources / target. |
| docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/convert/request/ConvertDocumentRequest.java | Refactors to extend DocumentRequest; keeps convert-specific fields. |
| docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/convert/request/BatchConvertDocumentRequest.java | Refactors to extend DocumentRequest; enforces non-null target via overridden getter. |
| docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/chunk/request/ChunkDocumentRequest.java | Refactors to extend DocumentRequest; keeps chunk-specific fields. |
Suppressed comments (2)
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:54
- Batch requests are documented as requiring a
PresignedUrlTargetorS3Target, but this test constructs aBatchConvertDocumentRequestwith aZipTarget. Please switch to a valid batch target implementation.
DocumentRequest request = BatchConvertDocumentRequest.builder()
.source(HTTP_SOURCE)
.target(ZipTarget.builder().build())
.build();
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:122
- Batch requests are documented as requiring a
PresignedUrlTargetorS3Target, but this test usesZipTargetfor the batch request in the list. Use a valid batch target here (and consider formatting theList.of(...)entries one-per-line for readability).
List<DocumentRequest> requests = List.of(
ConvertDocumentRequest.builder().source(HTTP_SOURCE).build(), BatchConvertDocumentRequest.builder().source(HTTP_SOURCE).target(ZipTarget.builder().build())
.build(), HierarchicalChunkDocumentRequest.builder().source(HTTP_SOURCE).build(), HybridChunkDocumentRequest.builder().source(HTTP_SOURCE).build()
);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
b3c52be to
f7127c0
Compare
:java_duke: JaCoCo coverage report
|
|
||||||||||||||
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (7)
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:54
BatchConvertDocumentRequestdocs and client tests indicate the batch endpoint requires aPresignedUrlTargetorS3Target; usingZipTargethere makes the test demonstrate an invalid request shape.
}
@Test
void sourcesDefaultToEmptyList() {
DocumentRequest request = ConvertDocumentRequest.builder().build();
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:18
- This test file imports
ZipTarget, but batch conversion targets should bePresignedUrlTargetorS3Target(and the updated assertions below usePresignedUrlTarget). Update the import to match the supported batch target types.
import ai.docling.serve.api.convert.request.target.ZipTarget;
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:122
- This list-based test also uses
ZipTargetfor a batch request, which conflicts with the documented batch target contract. Switching toPresignedUrlTargetkeeps the test aligned with supported request shapes and improves readability.
}
else if (request instanceof HierarchicalChunkDocumentRequest) {
return "hierarchical-chunk";
}
else if (request instanceof HybridChunkDocumentRequest) {
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:174
- The codebase consistently formats
else ifon the same line as the closing brace (e.g.,Jackson2ConvertDocumentResponseDeserializer.java:38). This method’s style is inconsistent and may be reformatted by Spotless.
docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/request/DocumentRequest.java:34 - The Javadoc example uses switch pattern matching (e.g.,
case ConvertDocumentRequest r -> ...), which requires preview features on Java 17 and may not compile for typical consumers of this library. Consider using aninstanceof-based example (Java 16+) or explicitly noting the Java version requirement.
* <p>This is a {@code sealed} class — the only permitted subtypes are
* {@link ConvertDocumentRequest}, {@link BatchConvertDocumentRequest}, and
* {@link ChunkDocumentRequest} — enabling exhaustive pattern matching:
*
* <pre>{@code
docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/convert/request/BatchConvertDocumentRequest.java:67
getTarget()throws with the message "target is marked non-null but is null", but the non-null contract here is specific to batch requests rather than an annotation-derived constraint. Using a batch-specific message will be clearer for API consumers.
@Override
public Target getTarget() {
return Objects.requireNonNull(super.getTarget(), "target is marked non-null but is null");
}
docs/src/doc/docs/whats-new.md:28
- This changelog entry suggests dispatch "via pattern matching". Since the project targets Java 17, it would be clearer to mention
instanceof-based dispatch (and optionally note switch pattern matching as Java 21+/preview) to avoid implying consumers must enable preview features.
* **New `DocumentRequest` sealed base class** — `ConvertDocumentRequest`, `BatchConvertDocumentRequest`, and `ChunkDocumentRequest` now extend a common `DocumentRequest` abstract class in the `ai.docling.serve.api.request` package. This enables polymorphism when working with different request types — for example, accepting a `DocumentRequest` and dispatching to the correct endpoint based on the concrete type via pattern matching.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/convert/request/BatchConvertDocumentRequest.java:66
- The null-check message in getTarget() says "marked non-null" even though the non-null contract is enforced by this override (the superclass getter is @nullable). This makes the exception harder to understand; use a message that states the batch-request contract explicitly.
return Objects.requireNonNull(super.getTarget(), "target is marked non-null but is null");
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:65
- This test verifies instanceof dispatch, but it doesn’t assert the sealed "permits" list on DocumentRequest. Since the PR introduces a sealed base type, add an explicit check of getPermittedSubclasses() so accidental future permit changes are caught by tests.
void allConcreteSubtypesAreDistinguishableViaInstanceOf() {
List<DocumentRequest> requests = List.of(
ConvertDocumentRequest.builder().source(HTTP_SOURCE).build(), BatchConvertDocumentRequest.builder().source(HTTP_SOURCE).target(ZipTarget.builder().build())
.build(), HierarchicalChunkDocumentRequest.builder().source(HTTP_SOURCE).build(), HybridChunkDocumentRequest.builder().source(HTTP_SOURCE).build()
);
docs/src/doc/docs/whats-new.md:28
- This changelog entry introduces the new sealed base class, but it doesn’t mention that
ConvertDocumentRequestandBatchConvertDocumentRequestare nowfinalto participate in the sealed hierarchy. That can be a breaking change for consumers who subclassed these request models; consider calling it out here.
* **New `DocumentRequest` sealed base class** — `ConvertDocumentRequest`, `BatchConvertDocumentRequest`, and `ChunkDocumentRequest` now extend a common `DocumentRequest` abstract class in the `ai.docling.serve.api.request` package. This enables polymorphism when working with different request types — for example, accepting a `DocumentRequest` and dispatching to the correct endpoint based on the concrete type via pattern matching.
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
69a6506 to
426358e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:22
- PR description says
DocumentRequestTestscontains 12 test cases including a direct sealed-permits verification. This file currently contains 7@Testmethods and does not verifyDocumentRequest’s direct permitted subclasses, so the implementation doesn’t match the stated test plan/coverage.
/**
* Unit tests for the {@link DocumentRequest} sealed hierarchy.
*/
class DocumentRequestTests {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/DoclingServeConvertApi.java:152
ConvertDocumentRequestdoes not declare a nestedBuildertype (it declaresConvertDocumentRequestBuildervia Lombok@SuperBuilder), so the explicit type witness.<ConvertDocumentRequest.Builder<?, ?>>is very likely to fail compilation. Removing the explicit type parameter (and relying onvarinference) avoids referencing a non-existent nested type.
var builder = Optional.ofNullable(request).<ConvertDocumentRequest.Builder<?, ?>>map(ConvertDocumentRequest::toBuilder)
.orElseGet(ConvertDocumentRequest::builder);
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
426358e to
de1c877
Compare
…aled base classes Introduce sealed type hierarchies for both requests and responses in the docling-serve-api module, enabling exhaustive pattern matching and polymorphic handling of document processing operations. DocumentRequest (ai.docling.serve.api.request) is an abstract sealed class with common fields (sources, target) that permits ConvertDocumentRequest, BatchConvertDocumentRequest, and ChunkDocumentRequest. ProcessedDocumentResponse (ai.docling.serve.api.response) is an abstract sealed marker class that permits ConvertDocumentResponse (itself sealed) and ChunkDocumentResponse (final). This enables consumers to use a common type bound when working with either conversion or chunking results. Closes docling-project#632 Assisted-By: Claude Code <noreply@anthropic.com> Signed-off-by: Eric Deandrea <eric.deandrea@ibm.com>
de1c877 to
7cf8632
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/convert/request/BatchConvertDocumentRequest.java:67
BatchConvertDocumentRequest.getTarget()currently throws aNullPointerExceptionviaObjects.requireNonNull(...), but this module is configured to throwIllegalArgumentExceptionfor Lombok@NonNullcontract violations (seesrc/lombok.config). For consistency with the rest of the API (and existing tests that expectIllegalArgumentExceptionfor missing required fields), throwIllegalArgumentExceptionwhentargetis absent.
@Override
public Target getTarget() {
return Objects.requireNonNull(super.getTarget(), "target is marked non-null but is null");
}
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:98
- This test currently asserts that
BatchConvertDocumentRequest.getTarget()throwsNullPointerExceptionwhentargetis missing. The module’s Lombok configuration usesIllegalArgumentExceptionfor@NonNullviolations (lombok.nonNull.exceptionType=IllegalArgumentException), so the assertion should match the intended contract.
assertThatThrownBy(request::getTarget)
.isInstanceOf(NullPointerException.class);
}
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (4)
docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/convert/request/BatchConvertDocumentRequest.java:67
BatchConvertDocumentRequestpreviously enforced a non-nulltargetvia Lombok@NonNull(configured to throwIllegalArgumentExceptionin this module), but the new override usesObjects.requireNonNull(...)which throws aNullPointerExceptionand shifts the failure from build-time to getter-time. For consistency with the module’s Lombok@NonNullcontract (see docling-serve/docling-serve-api/src/lombok.config:6) and to avoid surprising exception type changes, throwIllegalArgumentExceptionhere instead ofNullPointerException.
@Override
public Target getTarget() {
return Objects.requireNonNull(super.getTarget(), "target is marked non-null but is null");
}
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:97
- This test currently asserts
NullPointerException, butBatchConvertDocumentRequest#getTarget()should align with the module’s Lombok@NonNullconvention (IllegalArgumentException via lombok.nonNull.exceptionType) whentargetis missing.
assertThatThrownBy(request::getTarget)
.isInstanceOf(NullPointerException.class);
}
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/response/ProcessedDocumentResponseTests.java:79
- The project codebase consistently formats
else ifon the same line as the closing brace (e.g., Jackson2ConvertDocumentResponseDeserializer.java:40). This new test uses a different style (}thenelse ifon the next line), which is likely to fail Spotless formatting checks.
}
else if (response instanceof PreSignedUrlConvertDocumentResponse) {
return "pre-signed-url";
}
else if (response instanceof PreSignedUrlConvertResponse) {
return "pre-signed-url-response";
docling-serve/docling-serve-api/src/test/java/ai/docling/serve/api/request/DocumentRequestTests.java:126
- The project codebase consistently formats
else ifon the same line as the closing brace (e.g., Jackson2ConvertDocumentResponseDeserializer.java:40). This new test uses a different style (}thenelse ifon the next line), which is likely to fail Spotless formatting checks.
}
else if (request instanceof BatchConvertDocumentRequest) {
return "batch";
}
else if (request instanceof HierarchicalChunkDocumentRequest) {
Summary
DocumentRequestbase class (ai.docling.serve.api.request) that consolidates the sharedsourcesandtargetfields fromConvertDocumentRequest,BatchConvertDocumentRequest, andChunkDocumentRequestProcessedDocumentResponsebase class (ai.docling.serve.api.response) that unifiesConvertDocumentResponseandChunkDocumentResponseunder a common typeDocumentRequestorProcessedDocumentResponseand dispatch based on concrete type (e.g., LangChain4j'sDoclingDocumentParsercan useProcessedDocumentResponseas a type bound for generic builders that work with both conversion and chunking results)Request type hierarchy
Response type hierarchy
Test plan
./gradlew --no-daemon :docling-serve-api:testpassesDocumentRequestTests— 7 tests covering sealed hierarchy, field inheritance,instanceofdispatch,toBuilder(), andBatchConvertDocumentRequest.getTarget()non-null contractProcessedDocumentResponseTests— 4 tests covering sealed hierarchy,instanceofassignability for both convert and chunk responses, and sealed permits verificationResolves #632