Repository navigation
PDFCLOUD-6254: Convert structured documents to PDF - #47
datalogics-kam wants to merge 18 commits into
Conversation
ce87afd to
8a5b039
Compare
aad4964 to
4ef4925
Compare
datalogics-kam
left a comment
datalogics-kam
left a comment
There was a problem hiding this comment.
Blocking dependency: PDFCLOUD-6287 blocks this work because the client
currently reproduces the upstream structured-text contract's open-ended
strings instead of preventing invalid requests.
In this PR, PdfStructuredTextPageSetup.size is str; the five font selectors
in PdfStructuredTextStyle are str/Sequence[str]; and the internal
Pydantic models enforce only min_length=1. language is likewise only a
non-empty string even though the OpenAPI description merely says it is
"typically" BCP 47. Consequently, values such as arialbold and Executive
pass client-side validation even though they are not valid structured-text font
and page-size identifiers.
This matters because the public font page documents watermark/add-text-style
names and API tokens, while the structured-text converter uses a different
APDFL catalog. See the upstream contract work and complete discrepancy list:
https://datalogics-jira.atlassian.net/browse/PDFCLOUD-6287
Please keep this PR blocked until PDFCLOUD-6287 publishes the authoritative
OpenAPI enums and validation. Then consume those contracts here as public
Literal aliases with accepted-values documentation, validate them in the
payload models, and add exhaustive accepted-value plus representative rejection
tests for sync and async callers.
GitHub does not permit the PR author to submit a Request changes review, so this
is recorded as a review comment; PDFCLOUD-6287 is formally linked as blocking
PDFCLOUD-6254 in Jira.
8a5b039 to
375c2e2
Compare
✅ Deploy Preview for pdfrest-python ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
- Export public structured document types for page, style, table, and format-specific options. - Add format-specific payload validation and nested wire serialization. - Deduplicate uploaded Markdown images into ordered resource IDs. Assisted-by: Codex
- Add synchronous and asynchronous helpers for Markdown, plain text, JSON, XML, and CSV inputs. - Cover exact request serialization, validation boundaries, transport behavior, and request customization. Assisted-by: Codex
- Exercise all five structured formats through synchronous and asynchronous clients. - Verify Markdown image resources, response metadata, and server-side invalid-option handling against deterministic fixtures. Assisted-by: Codex
- Add a runnable upload-first example covering Markdown, plain text, JSON, XML, and CSV helpers. - Add deterministic source documents and register the example in the inventory. Assisted-by: Codex
- Add all five format-specific conversion helpers to the Into PDF API guide. - Link generated method signatures to their public structured option types. Assisted-by: Codex
- Release the structured document conversion helpers as a minor feature update. - Keep the project metadata and lockfile package version synchronized. Assisted-by: Codex
- Explain data presentation, page orientation, image alt-text policy, line handling, and CSV alignment values. - Keep value documentation on public aliases for generated API reference reuse. Assisted-by: Codex
Exercise every structured-document literal through both client transports against the live service, including shared page orientation. Assisted-by: Codex
Verify every helper rejects every other structured source before transport, and covers sync and async request customization timeouts. Assisted-by: Codex
datalogics-tsmith
left a comment
datalogics-tsmith
left a comment
There was a problem hiding this comment.
Requesting changes for one confirmed API-compatibility defect and two repository-policy gaps. The CSV delimiter issue prevents callers from using a valid PDFCloud API input; the accompanying tests should cover the corrected behavior and the remaining declared constraints. Please also resolve or explicitly document the production private-import exception.
|
|
||
|
|
||
| class _StrictStructuredTextModel(BaseModel): | ||
| model_config = ConfigDict(extra="forbid", str_strip_whitespace=True) |
There was a problem hiding this comment.
[P2] Preserve whitespace CSV delimiters. PDFCloud-API validates csv.delimiter with z.string().length(1) and does not trim it, but this shared configuration strips " " and "\t" before _StructuredTextCsvOptions.delimiter checks their length. I confirmed both values pass the API schema, and a live /pdf conversion with a space delimiter succeeded. The typed helper currently rejects that valid request before transport. Please preserve delimiter text verbatim, limit trimming to fields the API actually trims, and add sync/async unit plus live cases for whitespace delimiters. The same global stripping also makes whitespace-only image_alt_text stricter than the API, so please make that divergence intentional or align it too.
There was a problem hiding this comment.
Addressed the SDK validation and serialization issue in 0706b7f. CSV delimiters and image alternative text are preserved verbatim; trimming is limited to title, language, and font names. Payload tests and distinct sync/async request tests cover space/tab delimiters, padded and whitespace-only alternative text, retained trimming, and invalid delimiter lengths rejected before transport.
The requested sync/async space/tab live tests are retained with an explicit skip. A stronger probe using genuinely space/tab-separated CSV input and a second-column override uncovered a separate CLU defect: CsvAdapter.ResolveDelimiter() treats whitespace delimiters as unspecified and falls back to comma. The second-column override then produces HTTP 400 because only one column was parsed. A successful PDF response alone therefore does not establish correct delimiter handling. The skip reason names that defect and directs re-enabling these cases after the CLU fix is deployed.
Validation at this commit: 394 focused unit tests passed (10 existing format-matching skips), and 52 live tests passed against http://sleipnir:3000 (four CLU-blocked cases skipped).
There was a problem hiding this comment.
Thanks—the SDK delimiter and image-alt-text behavior is fixed, and the stronger live probes correctly expose the separate CLU defect. This thread is not fully resolved because AGENTS.md:234-244 requires behavior that cannot be exercised live to be called out in the PR description with the reason and a follow-up plan. The current description does not mention the four skipped space/tab cases, the CLU blocker, or a tracking plan; it also has stale validation counts and base-branch information. Please update the PR description and link the concrete CLU follow-up before resolving this thread.
There was a problem hiding this comment.
The PR description documents the current main base, validation counts, and all four skipped sync/async space/tab delimiter cases. Explicit whitespace delimiters are currently ignored by the service. The retained tests use whitespace-separated CSV input and an index-1 column override to verify that two columns are parsed.
After corrected behavior is available, verify both parsed columns and remove the skips from test_live_convert_csv_to_pdf_whitespace_delimiter and test_live_async_convert_csv_to_pdf_whitespace_delimiter. All four cases must pass with the second-column override intact.
bafcf62 contains the public-disclosure guidance in AGENTS.md and the corresponding skip-reason cleanup. The description and skip reason use observable API behavior, and the description references only the primary work item. This is separate from the page-size fix and live boundary coverage.
| [ | ||
| pytest.param({"page_setup": {"width": 0.1, "height": 0.1}}, id="page-min"), | ||
| pytest.param({"page_setup": {"margin": {"top": 0}}}, id="margin-min"), | ||
| pytest.param({"style": {"text_size": 6}}, id="text-size-min"), |
There was a problem hiding this comment.
[P2] Complete the required constraint-boundary matrix. TESTING_GUIDELINES.md:134-138 requires explicit tests for every ge, le, gt, lt, or Annotated constraint at the documented bound, just inside it, and outside it; the AGENTS testing rules repeat that requirement. This table still lacks text_size just-inside cases and an upper-outside value, and the suite lacks empty-value cases for column_width_weights, fallback_fonts, CSV columns, title, and language. Please add those cases so every newly declared constraint has regression coverage.
There was a problem hiding this comment.
Addressed in two reviewable commits: 3f8fd3e covers numeric and RGB boundaries, and 5ebce31 covers text, collections, and resource cardinality.
The matrix now includes text_size values 6, 7, 71, and 72, with 5 and 73 rejected; empty column_width_weights, fallback_fonts, CSV columns, title, and language; minimum-length successes and whitespace rejection where trimming applies; every declared numeric and RGB channel boundary; and source/image collection and image-index bounds. Public options are exercised through payload serialization and distinct sync/async client paths, with a fail-on-use transport confirming invalid inputs never reach HTTP execution. Internal image index and image ID bounds have direct payload tests.
Validation: the final focused module passed 1,138 tests with 10 existing format-matching skips. The non-live Nox matrix passed 2,835 tests with the same 10 skips on each of Python 3.10–3.14; the class-function coverage gates passed on all five versions. Ruff and Basedpyright passed for the remediation, and all applicable commit/pre-push hooks passed.
There was a problem hiding this comment.
The expanded unit matrix is thorough and resolves the specific unit omissions called out above. This thread remains open because the live half of the repository requirement is still absent: AGENTS.md:349-358 requires live tests to enumerate valid and invalid numeric boundaries, vary one parameter per request, and use extra_body to reach server-side rejection paths. The live structured-conversion module currently exercises representative numeric values but not the declared numeric bounds. Please add distinct sync and async live boundary cases for the newly constrained fields.
There was a problem hiding this comment.
Addressed in 3e09db4. This commit contains only the live numeric/RGB boundary matrix, CSV column and Markdown image-index coverage, and the two supporting fixtures. Page-size regressions are in a separate commit.
Schema-derived cases vary one value per request through distinct sync and async tests. Invalid values use extra_body to reach server validation; schema-valid page dimensions that violate usable-area requirements have explicit rejection checks.
The same final file tree previously passed 426 focused live tests, with four existing delimiter skips, and the Python 3.10–3.14 non-live matrix. After the history split, all 430 live cases collected successfully and pre-commit checks passed. The full matrix and live requests were not repeated because the file contents are unchanged.
| from .models._internal import ( | ||
| BasePdfRestGraphicPayload, | ||
| BmpPdfRestPayload, | ||
| ConvertCsvToPdfPayload, |
There was a problem hiding this comment.
[P3] Resolve the private-module import policy conflict. AGENTS.md:223-226 says production code must not import modules whose names begin with an underscore. This import block predates the PR, but these new payload imports extend that violation. Please expose the payloads through an approved non-private production module, or document a narrow exception for the client-to-payload relationship before adding more imports here.
There was a problem hiding this comment.
Addressed in 5fd61fc. AGENTS.md now explicitly permits private-module and private-symbol imports within the package, including the client-to-payload relationship. External dependencies must be consumed through their supported public APIs; another company-owned package still counts as external. Package internals remain outside public exports unless deliberately promoted. This preserves the intended public/private boundary without exposing the payload models as supported SDK APIs. The commit's applicable hooks passed.
Allow imports of private modules and symbols within the package while requiring supported public APIs for external dependencies, including other company-owned packages. Keep internal contracts out of public exports and permit focused internal tests when needed. Assisted-by: Codex
Restrict whitespace trimming to titles, languages, and font names. Preserve CSV delimiters and image alternative text verbatim, and reject invalid delimiter lengths before executing either client transport. Add payload and sync/async request regressions. Retain live space/tab cases with an explicit skip until the CLU delimiter fix is deployed. Assisted-by: Codex
Exercise accepted and rejected dimensions, margins, text sizes, heading scales, table borders, padding, column weights, and column indices. Check every RGB channel at its limits and reject invalid tuple lengths. Verify exact payload serialization through distinct sync and async request paths, with invalid inputs rejected before transport execution. Assisted-by: Codex
Exercise minimum text lengths, whitespace rejection, and nonempty collection bounds through payloads and both client transports. Verify exact output and image alternative text serialization. Reject empty or multiple sources for every structured document format before transport execution. Cover image index and image ID list bounds on the internal Markdown payload contract. Assisted-by: Codex
datalogics-tsmith
left a comment
datalogics-tsmith
left a comment
There was a problem hiding this comment.
Requesting changes because the remediation is not complete: page-size whitespace normalization regressed, the constraint matrix still lacks the required live numeric-boundary coverage, and the CLU-blocked whitespace-delimiter live cases are not disclosed in the PR description with a concrete follow-up plan. I continued the two existing discussions for the partially addressed findings and opened one new inline comment for the newly introduced regression.
|
|
||
|
|
||
| class _StructuredTextPageSetup(_StrictStructuredTextModel): | ||
| size: PdfStructuredTextPageSize | None = None |
There was a problem hiding this comment.
[P2] Restore the API's page-size whitespace normalization. The remediation removed model-wide whitespace stripping, but page_setup.size was not given a field-specific normalizer. PDFCloud-API defines this field as z.string().trim().min(1): its schema converts " Letter " to "Letter", and I confirmed the live /pdf endpoint accepts that request. This SDK model now rejects it with a literal validation error. Please apply the same pre-strip used for the other API-normalized text fields and add payload, sync, and async regression cases for a padded named size.
There was a problem hiding this comment.
Addressed in fa6ba23. This commit contains only the page-size whitespace fix and its payload, sync/async request, and live regressions. The field-specific pre-strip preserves the accepted named-size spellings; the public type remains a Literal.
The focused unit module was rerun after splitting the commits: 1,141 passed, with 10 existing format-matching skips. The live page-size cases previously passed, and the final file tree is identical to that validated version. Pre-commit checks passed.
Normalize padded named sizes before literal validation. Keep payload, sync/async request, and live page-size regressions with the fix. Assisted-by: Codex
Derive numeric and RGB cases from payload schemas and exercise accepted and rejected values through both transports. Cover CSV column options and image indices with deterministic table and two-image fixtures. Assisted-by: Codex
Keep repository content and review communication suitable for public sharing. Describe blocked delimiter coverage through observable service behavior and limit work-item references to the primary PR item. Assisted-by: Codex
56b7742 to
bafcf62
Compare
This branch was successfully deployed
2 participants
PDFCLOUD-6254
Why this change
The SDK could not convert structured Markdown, plain text, JSON, XML, or CSV documents through the documented
POST /pdfcapability. Callers had to construct untyped requests and could combine options that the service accepts only for particular source formats.What changed (high level)
The synchronous and asynchronous clients now provide five format-specific helpers backed by Pydantic payload models and public structured-document types. Shared page, typography, table, color, and output settings serialize to
structured_text_options, while format-specific signatures keep incompatible options separate. Markdown image mappings accept uploaded resources and internally deduplicate their IDs. A runnable example, fixtures, and generated API-guide links make the feature discoverable.Behavior changes
Callers upload a source document and pass its
PdfRestFiletoconvert_markdown_to_pdf,convert_plain_text_to_pdf,convert_json_to_pdf,convert_xml_to_pdf, orconvert_csv_to_pdf. Local validation rejects incorrect source formats, multiple source files, unsupported Markdown image MIME types, and invalid literal or numeric options before transport. CSV delimiters and image alternative text remain verbatim; page-size names, title, language, and font names follow the API's whitespace trimming. This feature release updates version 1.1.0 to 1.2.0 without changing existing public APIs.Validation
uv run pytest -n auto --maxschedchunk 2 tests/test_convert_structured_documents_to_pdf.py— 1,141 passed; 10 existing matching-format skips.uvx nox -s tests -- -m 'not live'— 2,838 passed and 10 matching-format skips on each of Python 3.10–3.14. The 90% class-function coverage gates passed for all four client classes on every interpreter.uv run pytest -n auto --maxschedchunk 2 tests/live/test_live_convert_structured_documents_to_pdf.py— 426 passed; four space/tab delimiter cases skipped for the service limitation described below. The live numeric matrix derives bounds from the payload schemas, varies one numeric value per request, and exercises both transports. Invalid values reach the server throughextra_body. Positive page dimensions too small for the required usable area are checked as rendering rejections.uv run pre-commit run --all-filespassed on the completed changes in a clean checkout. The full live suite and example matrix were not rerun locally because this remediation changes only structured-conversion validation and its tests.Risks and follow-ups
This PR targets
main. Four live tests remain skipped because the service currently ignores explicit space and tab CSV delimiters. These tests use genuinely whitespace-separated CSV input and an index-1 column override, so a successful PDF response alone cannot mask incorrect parsing. The service follow-up must preserve explicit single-character whitespace delimiters and verify both parsed columns and their exact values. After corrected delimiter handling is available, remove the skips from bothtest_live_convert_csv_to_pdf_whitespace_delimiterandtest_live_async_convert_csv_to_pdf_whitespace_delimiterand require all four cases to pass with the second-column override intact. The SDK accepts and preserves valid whitespace delimiters; end-to-end parsing verification remains pending that service correction.Workflows are unchanged. CI runs non-live tests, class-function coverage, and examples on Python 3.10–3.14; live tests run on Python 3.11 for pull requests and scheduled runs. Diff coverage and docs-preview artifacts are PR-only.