Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a JSON import path for serverless ADR report items, backed by shared Pydantic schemas for validating item payloads.
Changes:
- Adds shared JSON schema models for report items and top-level import payloads.
- Adds a serverless importer that loads JSON, creates a default layout template, and saves text/table/image/file/scene/tree items.
- Adds normalization helpers for JSON-style tags and table/tree conversion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/ansys/dynamicreporting/core/utils/json_importer_models.py |
Defines Pydantic models for supported JSON report item payloads. |
src/ansys/dynamicreporting/core/serverless/json_importer.py |
Implements serverless import logic for validated JSON report items. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #548 +/- ##
==========================================
+ Coverage 82.08% 82.64% +0.56%
==========================================
Files 32 39 +7
Lines 9443 9904 +461
Branches 1698 1774 +76
==========================================
+ Hits 7751 8185 +434
- Misses 1195 1210 +15
- Partials 497 509 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
28a50b0 to
7ee078b
Compare
358ecd0 to
9b0a2e4
Compare
2270a46 to
f0e2147
Compare
6fd926f to
fa9bd40
Compare
2ca9e0e to
5997a69
Compare
viseshrp
left a comment
There was a problem hiding this comment.
@dcansys this PR goes in the opposite direction of what we agreed in our meeting. We agreed to adopt Pydantic for the subset of models this import needs, as the first step towards replacing the hand-built service and serverless models. Instead, 06b41b2 removed Pydantic entirely and added a third set of hand-built models and validation code, about 1,500 lines. Did you review this code yourself before pushing it? Which AI model did you use to write it?
@margalva Please correct me if I am wrong about the direction of this PR. |
@viseshrp @dcansys from my notes on the backlog refinement, I have: remove pydantic. We will add a data validation in the core of ADR at a later time, not specific to this functionality. We will most probably use pydantic to do that, but it needs to be in the core of ADR. |
@margalva Hmm, do you have a date for the notes? I remember what you have mentioned is what we decided before we put this on hold until Dragan was back from vacation in July/August and then had a meeting again where we decided to either adopt Pydantic or extend the current set of models to include JSON import without rebuilding something from scratch, based on the time we had to deliver this feature. I also remember we put the original PR on hold because the original code proposed to write a complete set of new models from scratch, similar to the current state of this PR. Sorry, but I'm confused. |
@margalva @dcansys
So, we decided to switch to pydantic later, but use the existing models for now. That is still not addressed in this PR. We are back to square one. The only difference here from the original implementation is that we have removed pydantic completely and replaced it with our own version of pydantic-like models and validation instead of reusing what we already have in our code (a 3rd model/solution path -- same problem as the very first attempt and also typical of mistakes made by AI). |
Hi @margalva @viseshrp, below is my recollection of how the requirements and implementation direction for JSON report-item import evolved. Please correct me if I’ve missed anything.
The agent-generated MVP plan said to be "additive only" and to "not modify the existing models."
|
|
I agree the parser duplicates some item validation, and I'll remove the checks the backend validators already cover, such as tree value types. I don't think the JSON item import can be built on the existing models alone, though. They validate ADR item content, not the import document. The JSON contract needs conversion before those models can accept it: tables are transposed, missing tree keys and values are filled in, relative media paths are resolved, and tags are converted to strings. There's also no existing model for the document fields such as The importer already creates every item through the existing APIs, so the existing validators still run. My proposal is to keep only the JSON-specific checks and conversion, remove the duplicated item checks, and delegate item validation to the backends. To me the take-away is: fewer importer checks give less duplication but weaker guarantees about the JSON contract. When asked an agent to analyze the main branch and what's needed for integration based on new JSON schema: "What the existing validators won't enforce: Rejecting unsupported schema_version major versions. This needs a few lines. |
|
@dcansys Go ahead and make your changes. Please use Opus 5.5 for review and fixing as per my comments. Let me know when done. I will do my review after. |
|
Please read: CODE_REVIEW_WALKTHROUGH.md before review. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e schema with the parser Squashes the 14 commits from 4f0d06e to b021fdc, including the merge of main (c4ed862), which stays the second parent so the PR diff is unchanged. - Pass trees, tables, text/HTML values and media paths to the existing ItemREST and serverless validators instead of re-checking them in the importer. - Key REST payload setters by stored item type; check image paths with ItemREST.validate_file. - Use each backend's add_tag for imported tags, validate document tags up front, and reject non-string item_type with a validation error. - Move the JSON schema builder into the package and trim the tests to the needed coverage. - Allow null for optional document and item fields in the generated schema, matching the parser; fix the documented error header; drop stale drift-guard claims. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>





Summary
Adds beta, items-only JSON import through
import_from_jsonon bothServiceand serverlessADR. Supports text, HTML, tables, trees, images, animations, scenes, and files, with shared validation, document and item tags, relative media-path handling, and configurable per-item failure collection.Adds a versioned JSON Schema generated from the importer’s field specifications, a canonical sample, end-user documentation, and an example. Report templates remain outside this import path and continue to use the existing
load_templates*APIs.Testing
uv run pytest tests/json_import -q -p no:cacheprovidermake smoketestmake checkFixes https://ado.internal.synopsys.com/tfs/ANSYS_Development/Portfolio/_workitems/edit/1464631