fix!: Refactor ConvertDocumentResponse for polymorphic deserialization based on docling-serve API response#388
Conversation
ArpanKrChakraborty
force-pushed
the
feature/convert-api
branch
2 times, most recently
from
e971bc7 to
a5ea31c
Compare
|
Hi @ArpanKrChakraborty sorry for the delay in getting to this - I've been out of the office for 2 weeks and I'm still catching up. I will get to this! |
:java_duke: JaCoCo coverage report
|
|
||||||||||||||
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
ArpanKrChakraborty
force-pushed
the
feature/convert-api
branch
2 times, most recently
from
52fbd90 to
d74afef
Compare
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
|
Hi @ArpanKrChakraborty I do have to ask this question and please be honest. How much of the initial description you provided on #206 and how much of the implementation in this PR is generated by AI? |
|
Hi @edeandrea, in #206 the design changes I proposed are completely from my side. I went through docling-serve, docling-jobkit repos and the docling-serve documentation , to understand the flow and realized when which type of response is returned and proposed a basic superclass - subclass solution to it. Regarding implementation, I did use AI to generate java docs to some extent and explore knowledge on jackson annotations (like a form of advanced google search) and such and that's it. What you see is a squashed commit, the implemention you see now is a result of many iteration of changes - I did like around 10-15 commits in total and this is the final result. |
|
Thank you! I will be back in the office tomorrow after a 2 week hiatus so will review then. Thank you for your patience. |
There was a problem hiding this comment.
Pull request overview
This PR refactors ConvertDocumentResponse in docling-serve-api into a sealed, polymorphic response hierarchy to support multiple Docling Serve convert response shapes (in-body JSON, presigned-url stats, and ZIP archive streams), and updates the reference client, docs, and tests accordingly.
Changes:
- Replaced the former monolithic
ConvertDocumentResponsemodel with a sealed base type plusInBodyConvertDocumentResponse,PreSignedUrlConvertDocumentResponse, andZipArchiveConvertDocumentResponse. - Updated
docling-serve-clientto handle ZIP streaming responses and task-result responses that may be JSON or ZIP. - Updated documentation snippets and expanded tests to cover the new response types and ZIP behaviors (including referenced image artifacts).
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/src/doc/docs/testcontainers.md | Updates docs sample to use InBodyConvertDocumentResponse. |
| docs/src/doc/docs/getting-started.md | Updates getting-started sample to use InBodyConvertDocumentResponse. |
| docs/src/doc/docs/docling-serve/serve-client.md | Updates client docs; adds note for in-body response type (but error-handling snippet needs further fixes). |
| docs/src/doc/docs/docling-serve/serve-api.md | Updates API docs to describe the three response variants. |
| docling-testing/docling-version-tests/.../TagsTester.java | Updates version tests to branch on response type. |
| docling-serve/docling-serve-client/.../AbstractDoclingServeClientTests.java | Expands client tests to assert response types and ZIP contents. |
| docling-serve/docling-serve-client/.../util/package-info.java | Adds @NullMarked to the util package. |
| docling-serve/docling-serve-client/.../util/Utils.java | Adds helper parsing for Content-Disposition and Content-Type. |
| docling-serve/docling-serve-client/.../operations/TaskOperations.java | Adds Content-Type-based JSON-vs-ZIP handling for task results. |
| docling-serve/docling-serve-client/.../operations/StreamResponse.java | Introduces a stream response wrapper for binary bodies + headers. |
| docling-serve/docling-serve-client/.../operations/HttpOperations.java | Extends HTTP abstraction to support stream responses and readValue. |
| docling-serve/docling-serve-client/.../operations/ConvertOperations.java | Adds ZIP streaming path for zip/multi-source conversions. |
| docling-serve/docling-serve-client/.../DoclingServeClient.java | Implements stream GET/POST execution and StreamResponse handling. |
| docling-serve/docling-serve-api/.../convert/response/*.java | Adds new response subtypes + ResponseType enum; refactors base type to sealed + Jackson deduction. |
| docling-serve/docling-serve-api/.../ConvertDocumentResponseTests.java | Updates tests for the new response types. |
| docling-serve/docling-serve-api/.../DoclingServeTaskApi.java | Updates Javadoc to reflect task result semantics. |
| docling-serve/docling-serve-api/.../DoclingServeConvertApi.java | Updates Javadoc to reflect polymorphic convert responses. |
You can also share your feedback on Copilot code review. Take the survey.
| import com.fasterxml.jackson.annotation.JsonIgnoreProperties; | ||
| import com.fasterxml.jackson.annotation.JsonInclude; | ||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
| import com.fasterxml.jackson.annotation.JsonSetter; | ||
| import com.fasterxml.jackson.annotation.Nulls; | ||
| import com.fasterxml.jackson.annotation.JsonSubTypes; | ||
| import com.fasterxml.jackson.annotation.JsonTypeInfo; |
| switch (Utils.getContentType(response.getHeaders()).orElse("Unknown Content-Type")) { | ||
| case HttpOperations.CONTENT_TYPE_JSON -> { | ||
| try { | ||
| return httpOperations | ||
| .readValue(new String(response.getBody().readAllBytes(), StandardCharsets.UTF_8) | ||
| , ConvertDocumentResponse.class); | ||
| } catch (IOException e) { | ||
| throw new DoclingServeClientException(e); | ||
| } | ||
| } | ||
| case HttpOperations.CONTENT_TYPE_ZIP -> { | ||
| String fileName = Utils.getFileName(response.getHeaders()).orElse("converted_docs.zip"); | ||
| return ZipArchiveConvertDocumentResponse | ||
| .builder().fileName(fileName) | ||
| .inputStream(response.getBody()) | ||
| .build(); | ||
| } | ||
| default -> throw new DoclingServeClientException(null, "Content-Type missing in task api response"); |
...ing-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeClient.java
Show resolved
Hide resolved
| if (statusCode == 422) { | ||
| if(StreamResponse.class.equals(expectedReturnType)) { | ||
| try { | ||
| body = new String(((InputStream)body).readAllBytes(), StandardCharsets.UTF_8); | ||
| } catch (IOException e) { | ||
| throw new DoclingServeClientException(e); | ||
| } | ||
| } | ||
| throw new ValidationException( | ||
| readValue(body, ValidationError.class), | ||
| readValue(body.toString(), ValidationError.class), | ||
| "An error occurred while making %s request to %s".formatted(request.method(), request.uri()) | ||
| ); | ||
| } | ||
| else if (statusCode >= 400) { | ||
| // Handle errors | ||
| // The Java HTTPClient doesn't throw exceptions on error codes | ||
| throw new DoclingServeClientException("An error occurred: %s".formatted(body), statusCode, body); | ||
| if(StreamResponse.class.equals(expectedReturnType)) { | ||
| try { | ||
| body = new String(((InputStream)body).readAllBytes(), StandardCharsets.UTF_8); | ||
| } catch (IOException e) { | ||
| throw new DoclingServeClientException(e); | ||
| } | ||
| } | ||
| throw new DoclingServeClientException("An error occurred: %s".formatted(body.toString()), statusCode, body.toString()); |
| Transport errors (DNS, TLS, connection reset, timeouts) are thrown as standard Java exceptions | ||
| from `HttpClient`. Conversion may also return structured errors in the response body — inspect | ||
| `ConvertDocumentResponse#getErrors()` even when content is present: | ||
|
|
||
| ```java | ||
| // InBodyConvertDocumentResponse | ||
| var result = api.convertSource(request); | ||
| if (result.getErrors() != null && !result.getErrors().isEmpty()) { | ||
| result.getErrors().forEach(err -> |
| case HttpOperations.CONTENT_TYPE_JSON -> { | ||
| try { | ||
| return httpOperations | ||
| .readValue(new String(response.getBody().readAllBytes(), StandardCharsets.UTF_8) | ||
| , ConvertDocumentResponse.class); | ||
| } catch (IOException e) { | ||
| throw new DoclingServeClientException(e); | ||
| } | ||
| } |
| public static Optional<String> getContentType(StreamResponse.ResponseHeaders headers) { | ||
| return headers.getFirstValue(HttpOperations.CONTENT_TYPE_HEADER) | ||
| .filter(ai.docling.serve.api.util.Utils::isNotNullOrBlank); | ||
| } |
| return this.httpOperations.executePost(createRequestContext("/v1/convert/source", request)); | ||
| final String uri = "/v1/convert/source"; | ||
|
|
||
| boolean hasMultipleSources = request.getSources().size() > 1; |
| private void checkDoclingResponse(ConvertDocumentResponse response) { | ||
| Log.debugf("Response: %s", response); | ||
|
|
||
| assertThat(response) | ||
| .as("Response should not be null") | ||
| .isNotNull(); | ||
|
|
||
| assertThat(response.getStatus()) | ||
| .as("Response status should not be null or empty") | ||
| .isNotEmpty(); | ||
|
|
||
| assertThat(response.getErrors()) | ||
| .as("Response should not have errors") | ||
| .isNullOrEmpty(); | ||
|
|
||
| assertThat(response.getDocument()) | ||
| .as("Response should have a valid document") | ||
| .isNotNull() | ||
| .extracting( | ||
| DocumentResponse::getFilename, | ||
| DocumentResponse::getMarkdownContent, | ||
| DocumentResponse::getTextContent, | ||
| DocumentResponse::getJsonContent | ||
| ) | ||
| .satisfies(o -> | ||
| assertThat(o) | ||
| .as("Document should have a filename") | ||
| .asString() | ||
| .isNotEmpty(), | ||
| atIndex(0) | ||
| ) | ||
| .satisfies(o -> | ||
| assertThat(o) | ||
| .as("Document should have markdown content") | ||
| .asString() | ||
| .isNotEmpty(), | ||
| atIndex(1) | ||
| ) | ||
| .satisfies(o -> | ||
| assertThat(o) | ||
| .as("Document should have text content") | ||
| .asString() | ||
| .isNotEmpty(), | ||
| atIndex(2) | ||
| ) | ||
| .satisfies(o -> | ||
| assertThat(o) | ||
| .as("Document should have JSON content") | ||
| .asInstanceOf(InstanceOfAssertFactories.type(DoclingDocument.class)) | ||
| .isNotNull(), | ||
| atIndex(3) | ||
| ); | ||
| switch(response.getResponseType()) { | ||
| case InBodyConvertDocumentResponse -> { | ||
| var inBodyResponse = (InBodyConvertDocumentResponse)response; | ||
| Log.debugf("Response: %s", inBodyResponse); | ||
|
|
||
| assertThat(inBodyResponse) | ||
| .as("Response should not be null") | ||
| .isNotNull(); | ||
|
|
||
| assertThat(inBodyResponse.getStatus()) | ||
| .as("Response status should not be null or empty") | ||
| .isNotEmpty(); | ||
|
|
||
| assertThat(inBodyResponse.getErrors()) | ||
| .as("Response should not have errors") | ||
| .isNullOrEmpty(); | ||
|
|
||
| assertThat(inBodyResponse.getDocument()) | ||
| .as("Response should have a valid document") | ||
| .isNotNull() | ||
| .extracting( | ||
| DocumentResponse::getFilename, | ||
| DocumentResponse::getMarkdownContent, | ||
| DocumentResponse::getTextContent, | ||
| DocumentResponse::getJsonContent | ||
| ) | ||
| .satisfies(o -> | ||
| assertThat(o) | ||
| .as("Document should have a filename") | ||
| .asString() | ||
| .isNotEmpty(), | ||
| atIndex(0) | ||
| ) | ||
| .satisfies(o -> | ||
| assertThat(o) | ||
| .as("Document should have markdown content") | ||
| .asString() | ||
| .isNotEmpty(), | ||
| atIndex(1) | ||
| ) | ||
| .satisfies(o -> | ||
| assertThat(o) | ||
| .as("Document should have text content") | ||
| .asString() | ||
| .isNotEmpty(), | ||
| atIndex(2) | ||
| ) | ||
| .satisfies(o -> | ||
| assertThat(o) | ||
| .as("Document should have JSON content") | ||
| .asInstanceOf(InstanceOfAssertFactories.type(DoclingDocument.class)) | ||
| .isNotNull(), | ||
| atIndex(3) | ||
| ); | ||
| } | ||
| case PreSignedUrlConvertDocumentResponse -> { | ||
| var preSignedUrlResponse = (PreSignedUrlConvertDocumentResponse)response; | ||
| Log.debugf("Response: %s", preSignedUrlResponse); | ||
| } | ||
| case ZipArchiveConvertDocumentResponse -> {} | ||
| } |
| /** | ||
| * Number of attempted conversions | ||
| * | ||
| * @param numSucceeded the number of attempted conversions | ||
| * @return the number of attempted conversions | ||
| */ | ||
| @JsonProperty("num_converted") | ||
| private Integer numConverted; | ||
|
|
||
| /** | ||
| * Number of successful conversions | ||
| * | ||
| * @param numSucceeded the number of successful conversions | ||
| * @return the number of successful conversions | ||
| */ | ||
| @JsonProperty("num_succeeded") | ||
| private Integer numSucceeded; | ||
|
|
||
| /** | ||
| * Number of failed conversions | ||
| * | ||
| * @param numSucceeded the number of failed conversions | ||
| * @return the number of failed conversions | ||
| */ | ||
| @JsonProperty("num_failed") | ||
| private Integer numFailed; | ||
|
|
||
| @Override | ||
| @lombok.ToString.Include | ||
| public ResponseType getResponseType() { | ||
| return ResponseType.PreSignedUrlConvertDocumentResponse; | ||
| } | ||
|
|
||
| /** | ||
| * Builder for creating {@link PreSignedUrlConvertDocumentResponse} instances. | ||
| * Generated by Lombok's {@code @Builder} annotation. | ||
| * | ||
| * <p>Builder methods: | ||
| * <ul> | ||
| * <li>{@code processingTime(Double)} - Set the processing time in seconds</li> | ||
| * <li>{@code numConverted(Integer)} - Set the number of successful conversions</li> | ||
| * <li>{@code numFailed(Integer)} - Set the number of failed conversions</li> | ||
| * <li>{@code numConverted(Integer)} - Set the number of attempted conversions</li> | ||
| * </ul> |
| public enum ResponseType { | ||
| @JsonProperty("in_body") InBodyConvertDocumentResponse, | ||
| @JsonProperty("zip_archive") ZipArchiveConvertDocumentResponse, | ||
| @JsonProperty("presigned_url") PreSignedUrlConvertDocumentResponse |
There was a problem hiding this comment.
Couple of things
- Since
ResponseTypeis already in theconvert.responsepackage, its constants don't have to sayConvertDocumentResponse - Generally enum constants should be all uppercase using snake_case
so something like this:
public enum ResponseType {
@JsonProperty("in_body")
IN_BODY,
@JsonProperty("zip_archive")
ZIP_ARCHIVE,
@JsonProperty("presigned_url")
PRE_SIGNED_URL
}| public ConvertDocumentResponse convertSource(ConvertDocumentRequest request) { | ||
| ValidationUtils.ensureNotNull(request, "request"); | ||
| return this.httpOperations.executePost(createRequestContext("/v1/convert/source", request)); | ||
| final String uri = "/v1/convert/source"; |
There was a problem hiding this comment.
Prefer using the var keyword rather than declaring return type.
|
|
||
| private <I> RequestContext<I, ConvertDocumentResponse> createRequestContext(String uri, I request) { | ||
| return RequestContext.<I, ConvertDocumentResponse>builder() | ||
| private <I, O> RequestContext<I, O> createRequestContext(String uri, I request, Class<O> responseType) { |
There was a problem hiding this comment.
Should this be private <I, O extends ConvertDocumentResponse> RequestContext<I, O> createRequestContext(String uri, I request) {
| } | ||
|
|
||
| public InputStream getBody() { return body; } | ||
|
|
There was a problem hiding this comment.
There's an extra blank line here
| public InputStream getBody() { return body; } | ||
|
|
||
|
|
||
| public ResponseHeaders getHeaders() { return headers; } |
There was a problem hiding this comment.
Please don't keep methods as one-line
i.e. prefer this:
public ResponseHeaders getHeaders() {
return this.headers;
}| private InputStream body; | ||
| private ResponseHeaders headers; | ||
|
|
||
| public Builder() {} |
There was a problem hiding this comment.
The builder constructors should be private
|
|
||
| public Builder() {} | ||
|
|
||
| public Builder(StreamResponse streamResponse) { |
There was a problem hiding this comment.
The builder constructors should be private
| this.headers = builder.headers; | ||
| } | ||
|
|
||
| public InputStream getBody() { return body; } |
There was a problem hiding this comment.
Please don't have methods as one line
| var preSignedUrlResponse = (PreSignedUrlConvertDocumentResponse)response; | ||
| Log.debugf("Response: %s", preSignedUrlResponse); | ||
| } | ||
| case ZipArchiveConvertDocumentResponse -> {} |
There was a problem hiding this comment.
We probably don't need this for now. Maybe even assert that the response is a InBodyConvertDocumentResponse since thats what this test covers. Fail the test otherwise.
|
|
||
| ```java | ||
| ConvertDocumentResponse response = api.convertSource(request); | ||
| InBodyConvertDocumentResponse response = (InBodyConvertDocumentResponse)api.convertSource(request); |
There was a problem hiding this comment.
Please have a space between the cast and the variable.
edeandrea
left a comment
edeandrea
left a comment
There was a problem hiding this comment.
I also didn't notice anything in the whats-new.md file describing this as a breaking change?
edeandrea
changed the title
|
Hi @ArpanKrChakraborty thank you for your patience! I've had copilot go through this. I've reviewed copilot's findings and I agree with them. I've also added some of my own findings. |
|
Hi @edeandrea. You want me to consider all of copilot's review points ? Also regarding the mentioning of breaking changes in whats-new.md, I was not sure how to put it, but let me draft up something for starters. Thanks for the review ! |
Yes please. I've gone through everything copilot mentioned and they are things I would have mentioned myself (it even found some things I didn't during my manual review). |
|
You don't have to use the solution it proposed specifically, but the issues it found are valid issues. |
|
Thanks @edeandrea! I will refactor the code as suggested. |
…docling-project#206) Refactored ConvertDocumentResponse into a sealed abstract base class with three concrete implementations (InBodyConvertDocumentResponse, PreSignedUrlConvertDocumentResponse, ZipArchiveConvertDocumentResponse) to properly handle different response types from Docling Serve API. - Use Jackson @JsonTypeInfo with DEDUCTION for automatic type detection of JSON-based responses - Leverage Java 17 sealed classes for type safety - Update reference implementation in docling-serve-client to handle these new types - Add comprehensive test coverage for all response types - Update documentation and Javadoc - Apply Spotless Fixes docling-project#206 Signed-off-by: Arpan Chakraborty <arpan.chakraborty1908@gmail.com>
edeandrea
force-pushed
the
feature/convert-api
branch
from
9fc28a9 to
0fef244
Compare
|
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 |
fix: Refactor ConvertDocumentResponse for polymorphic deserialization (#206)
Refactored ConvertDocumentResponse into a sealed abstract base class with
three concrete implementations (InBodyConvertDocumentResponse,
PreSignedUrlConvertDocumentResponse, ZipArchiveConvertDocumentResponse)
to properly handle different response types from Docling Serve API.
Fixes #206