fix(export): harden YAML export input handling - #958
Conversation
Graph payloads circulate under two vocabularies, entities/relationships and nodes/edges, and consumers each reconciled them locally with competing idioms. The same payload could be exported, silently dropped, or rejected depending on which consumer read it. Add normalize_graph_payload() to utils.helpers as the single place that decision is made. Both spellings present with one empty resolves to the populated one, which is the shape JSONExporter emits; both non-empty and different is refused, since there is no basis to prefer either and picking one would silently discard the other; a non-empty mapping with no recognized key raises rather than returning empty collections, with require_recognized=False for callers that should degrade. Adopt it in the three exporters that genuinely alias. LPGExporter read nodes with entities as the default, so it dropped every entity when nodes was present but empty, losing everything on a JSON round-trip. ArangoAQLExporter had the same idiom plus a manual fallback. Neo4jCSVExporter routes its mapping branch through the shared resolver so the reference implementation cannot drift; its attribute branch stays local, since objects are not mappings. Also feed LPGExporter._generate_indexes the resolved entities. It read entities directly, so a nodes/edges payload produced no indexes even once node generation was fixed. CSVExporter and JSONExporter are deliberately excluded: they write entities, relationships, nodes and edges as separate outputs by design rather than reconciling two spellings of one collection, so normalizing there would rename output files.
export_yaml declared Union[Dict[str, Any], List[Dict[str, Any]]], but both YAML exporters read their payload by key, so a list reached .get() and surfaced as a bare AttributeError from inside the exporter, naming neither the offending argument nor the shape expected. Reject rather than wrap. These formats distinguish entities from relationships from triplets, so inferring which collection a bare list represents would silently mislabel the records, and wrapping it under an unrecognised key would write a structurally valid file with every collection empty - trading a loud failure for silent data loss. Validate in the exporters, matching the existing precedent in Neo4jCSVExporter._normalize_graph, so direct users of the classes get the same contract as callers of the convenience wrapper. Narrow the wrapper type hint to Dict[str, Any] to match.
- semantica/export/yaml_exporter.py — import Sequence from typing instead of collections.abc. `Sequence[str]` in _require_mapping's annotation is evaluated at function-definition time; collections.abc.Sequence only became subscriptable in Python 3.9, so on the 3.8 this project declares support for, importing this module raised TypeError. typing.Sequence has supported subscripting since 3.5.3. Mapping stays imported from collections.abc since it's only used for isinstance. - tests/export/test_yaml_exporter_input_validation.py — clean up each test's tempfile.mkdtemp() dir via addCleanup instead of leaking it, and read exported YAML through a context manager instead of an unclosed yaml.safe_load(open(...)).
Both YAML exporters built their output from a fixed set of `.get(key, [])`
lookups, so a mapping keyed by anything else serialized to a structurally
valid file with every collection empty. Nothing signalled the loss: no
exception, no warning, and the progress log reported a completed export.
The only way to notice was to open the file. The realistic trigger is
re-exporting an `export_json` payload, whose `{"data", "count", "metadata"}`
envelope drops every record.
- SemanticNetworkYAMLExporter.export_semantic_network now resolves its
collections through normalize_graph_payload(), which raises rather than
returning empty collections for an unrecognized mapping. Adopting the
shared resolver rather than repeating the check locally also brings the
'nodes'/'edges' aliases, so ContextGraph.to_dict() — the most direct path
from this library's own graph type to YAML, used in
examples/capability_gap_context_graphs_example.py — exports its records
instead of an empty file.
- export_for_pipeline built its nested semantic network from the same
defaulted lookups and had the same defect; it goes through the resolver
too.
- YAMLSchemaExporter.export_ontology_schema gets the equivalent check over
its own key set. Schemas are a separate vocabulary with no aliasing, so
_require_recognized_keys lives in this module rather than in the shared
graph resolver.
- 'metadata' is deliberately not sufficient to make a payload recognized.
An export_json envelope carries one, so accepting it would readmit the
case this fix is most likely to be needed for.
- An empty mapping is still exported: an empty graph is legitimate and has
no records to lose.
- SemanticNetworkYAMLExporter.export() serializes before creating the
output directory, so a rejected export leaves nothing behind.
The two rejections keep distinct exception types, following what the
codebase already does: a payload of the wrong *type* cannot be exported at
all and raises ProcessingError, matching Neo4jCSVExporter._normalize_graph;
a mapping whose *contents* are unusable raises ValidationError, matching
normalize_graph_payload. _require_mapping therefore runs first at every
entry point, so a non-mapping never reaches the resolver.
Docstring Raises sections, export_usage.md and docs/reference/export.md
record the accepted input shapes and both failures.
Closes semantica-agi#953.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoHarden YAML export validation and normalize graph-payload vocabularies
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| file_path = Path(file_path) | ||
| ensure_directory(file_path.parent) | ||
|
|
||
| yaml_content = self.export_semantic_network(data, **options) | ||
|
|
||
| ensure_directory(file_path.parent) |
There was a problem hiding this comment.
2. Progress success on failure 🐞 Bug ◔ Observability
SemanticNetworkYAMLExporter.export() calls export_semantic_network(), which stops progress tracking with status="completed", before creating the output directory and writing the file. If ensure_directory/open/write fails, progress tracking can still report a successful export even though no output file was produced.
Agent Prompt
## Issue description
`SemanticNetworkYAMLExporter.export_semantic_network()` marks the export as completed in the progress tracker before `export()` performs the filesystem side effects (directory creation and file write). This can lead to false “completed” progress status when the actual export fails during `ensure_directory(...)` or file write.
## Issue Context
This PR intentionally moved `ensure_directory()` to run after serialization so validation failures leave no output directory behind. That ordering is fine, but the progress lifecycle should still reflect the *full* export-to-disk outcome.
## Fix Focus Areas
- semantica/export/yaml_exporter.py[233-258]
- semantica/export/yaml_exporter.py[260-285]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@pravit-amp can you handle the qodo findings before we review it. |
Addresses the Qodo findings on semantica-agi#958. Presence-only recognition (finding 1): checking that a recognized key is present answered "did the caller use our vocabulary" when the question that matters is "did anything the caller supplied survive". A payload like {"entities": [], "data": [...records...]} cleared the check, resolved to empty, and dropped every record under 'data' -- the silent-empty export by a narrower route. - utils/helpers.py — split the check in two. _require_recognized_keys keeps the presence rule; _require_nothing_dropped runs after resolution and refuses a payload that resolved to nothing while an unread key still holds records. Only a non-empty list counts as evidence: ContextGraph.to_dict() always carries a populated 'statistics' dict, and an empty graph must stay exportable, so 'metadata', 'statistics' and 'count' are named as context rather than records. - export/yaml_exporter.py — the schema path had the same hole and now runs both checks through the shared helpers rather than its own copy, so the two vocabularies cannot drift apart in what counts as a silent-empty export. Progress reported success on a failed write (finding 3): export_semantic_ network stops its tracking as completed once serialization returns, but export() then creates the directory and writes the file. A failure there left the tracker showing a completed export with no output. - export/yaml_exporter.py — the serialization span now says it serialized, not that it exported, and export() opens its own span around the filesystem work that stops as failed on error. Nothing reports a completed export until the bytes are on disk. Finding 2 (export_yaml no longer accepts List[Dict]) is the intended resolution of semantica-agi#952 rather than a regression: wrapping a bare list under a guessed key is what would mislabel the records. The signature, docstring and PR description already record the narrowed contract. Tests cover both directions of each fix, including that an empty ContextGraph still exports and that a failing write is not reported as completed.
Findings 1 and 3 are fixed in ae190e4. On finding 2 — narrowing export_yaml to reject List[Dict] is the intended resolution of #952 rather than a regression against it: the old signature declared list support the code never had (it raised AttributeError), and wrapping a bare list under a guessed key would mislabel records that this format keeps distinct. The declared signature, docstring, and the Breaking Changes section of the PR description are updated to match. |
Sameer6305
left a comment
Sameer6305
left a comment
There was a problem hiding this comment.
Thanks @pravit-amp for putting this together. I appreciate the effort here, especially centralizing the entities/nodes and relationships/edges reconciliation instead of continuing to fix the same ambiguity independently in each exporter. The resolver design around populated-vs-empty aliases and refusing conflicting non-empty aliases is a solid direction, and the added end-to-end coverage is useful.
I reviewed the implementation against semantica-agi/main, including the connected exporters and public export paths. I found two issues that I think should be addressed before merge:
1. normalize_graph_payload() still accepts invalid collection values
semantica/utils/helpers.py — _resolve_collection()
The resolver currently treats any truthy value as a collection and does:
if value:
return list(value)This means malformed payloads such as:
{"entities": "abc"}are normalized to:
{"entities": ["a", "b", "c"], ...}while:
{"entities": 42}raises a raw:
TypeError: 'int' object is not iterable
The problem is that the new helper is now the central validation boundary, but invalid collection values can still either be silently transformed or leak low-level exceptions from list() into the exporters.
This is particularly risky because downstream exporters expect graph records, not arbitrary iterables. The resulting error then points at exporter internals rather than the malformed entities/nodes field.
Suggested fix: validate collection values before converting them. At minimum reject strings, bytes, mappings, and non-iterable scalar values with an actionable ValidationError. Ideally also validate that the resulting records have the expected mapping/record shape.
Please add regression tests for at least:
{"entities": "abc"}{"entities": 42}{"nodes": {"id": "n1"}}- equivalent relationship/edge cases
The resolver should either return a valid graph collection or fail at the normalization boundary.
2. Neo4j CSV still bypasses the new "unrecognized payload" rejection
semantica/export/neo4j_csv_exporter.py — _normalize_graph()
The new helper is called as:
normalize_graph_payload(graph, require_recognized=False)That means a non-empty mapping containing no recognized graph keys still normalizes to empty entities/relationships instead of raising.
For example, a payload such as:
{"data": [{"id": "e1"}]}can still reach the Neo4j exporter as an empty graph and produce empty CSV output.
This is directly at odds with the central contract introduced by this PR: the helper's default behavior is specifically designed to prevent an unrecognized mapping from becoming a valid-looking empty export.
I understand why the object/attribute path in Neo4j is kept separate, but for an actual mapping there does not appear to be a reason to opt out of the recognized-key validation.
Suggested fix: use the normal normalize_graph_payload(graph) behavior for mapping inputs, while retaining the existing attribute-based path for graph objects. Add a regression test asserting that an unrecognized non-empty mapping raises ValidationError rather than producing empty CSV files.
Why I think these need to be fixed before merge
Both issues are at the boundary of the new shared normalization contract. The first allows malformed collection values to escape the new validation layer, and the second leaves one of the adopters capable of the exact silent-empty behavior this refactor is intended to eliminate.
Once these are addressed and the corresponding regression tests are added, the overall direction of the PR looks good from my side.
Current verdict: Request changes.
The changes are relatively contained, so I don't think the PR needs a redesign — tightening the normalization boundary and making the mapping path consistent in Neo4j should be sufficient.
Two gaps at the boundary the shared normalizer is supposed to own.
_resolve_collection() resolved on truthiness alone, so a recognized key
could still hold something that is not a collection of records:
{"entities": "abc"} normalized to three single-character "records", and
{"entities": 42} surfaced as a raw TypeError from list() inside whichever
exporter happened to read it, naming the exporter rather than the payload
key at fault. Collection values are now validated before conversion --
strings, bytes, mappings, and non-iterable scalars are rejected by key
name, and each element must be a mapping or an attribute-carrying object,
the two record shapes the exporters actually read. None stays legal as an
absent collection, the spelling a JSON round-trip produces for []; it
cannot hide dropped records, since _require_nothing_dropped() still runs.
Every spelling present is validated, not just the one that wins, so a
malformed alias is not excused by a well-formed canonical key.
Neo4jCSVExporter._normalize_graph() opted out of the recognized-key check
for mappings, which left it able to turn {"data": [...]} into header-only
CSVs indistinguishable from a genuinely empty graph -- the exact failure
the rest of the change exists to prevent. Mapping payloads now go through
normalize_graph_payload() on its default terms. The attribute path for
graph objects is untouched. With no caller left opting out, the
require_recognized flag is removed rather than kept as a way back into
the silent-empty export.
Regression tests cover the malformed values end to end through every
export path that reads the normalizer, and assert the rejected Neo4j
export writes no CSV files.
Thanks for the careful read — both findings were real and are now fixed in a 1. A new On your "ideally also validate record shape": records are accepted as either Two judgment calls worth surfacing, happy to change either:
2. Neo4j CSV now uses the default contract for mappings.
Tests — 15 new cases covering the four shapes you listed across all five Docs updated in Full suite matches the pre-change baseline on my machine (the failures there |
Fix 1 -- _require_usable_schema silent data loss (P1):
_require_usable_schema() passed all values from _SCHEMA_KEYS into
_require_nothing_dropped() as evidence that records survived. Scalar
metadata fields such as version='1.0' and uri='http://...' are truthy
strings, so any one of them caused _require_nothing_dropped() to return
early and silently discard records stored under an unread key alongside
them (e.g. {'version': '1.0', 'nodes': [{'id': 'c1'}]}). Fixed by
building the resolved list from only non-empty list/tuple values of
recognised schema keys.
Fix 2 -- _is_record accepts modules and type objects (P2):
_is_record() accepted any object with __dict__, which includes Python
modules and class objects. Elements that passed _coerce_records then
reached exporters and raised AttributeError (e.g. module 'math' has no
attribute 'get') rather than a ValidationError at the validation
boundary. Fixed by excluding types.ModuleType and type from the
__dict__ branch while preserving support for all user-defined
attribute-bearing record objects.
Tests: 101 tests pass across
tests/utils/test_normalize_graph_payload.py
tests/export/test_yaml_exporter_key_recognition.py
tests/export/test_yaml_exporter_input_validation.py
tests/export/test_neo4j_csv_exporter.py
Sameer6305
left a comment
Sameer6305
left a comment
There was a problem hiding this comment.
Thanks @pravit-amp for putting this together. The centralization of graph-payload normalization is a solid improvement, and I appreciate the effort to keep the behavior consistent across the exporters instead of continuing to handle entities/nodes and relationships/edges independently.
I also appreciate the follow-up on the review findings. The requested fixes were addressed cleanly in 49fe0174, including validation of invalid collection values and making the Neo4j mapping path follow the shared recognized-key validation contract.
During the follow-up review, we also identified two additional edge cases in the new validation boundary:
_require_usable_schema()could treat scalar schema metadata such asversion,uri,title, ordescriptionas evidence that records had been exported, allowing records under an unread key to be silently dropped._is_record()accepted modules and class/type objects through the generic__dict__path, allowing invalid records to reach exporter internals instead of failing at the normalization boundary.
These were addressed in def27781 (fix(export): close YAML schema and record validation gaps), along with regression coverage. The changes are intentionally small and scoped to the validation boundary.
I verified the relevant coverage after the fixes:
- 101 targeted tests passing across the normalization, YAML validation, YAML key-recognition, and Neo4j exporter suites.
git diff --checkclean.- Regression coverage added for the newly identified cases.
- The existing support for mappings, dataclasses, and attribute-bearing record objects is preserved.
Overall, the implementation now has a consistent validation boundary, rejects malformed collection values with actionable errors, prevents unrecognized mappings from becoming silent empty exports, and keeps the different graph-payload aliases consistent across the relevant exporters.
Thanks again for the careful implementation and for addressing the review feedback promptly.
Also One separate follow-up point from the broader review: we also noticed that some validation error paths can reflect caller-controlled key names without a length bound, which can result in unnecessarily large error/log messages for extremely long input keys. This is pre-existing and not introduced by this PR, so it does not block approval.
I'll raise this separately shortly so it can be tracked independently. @pravit-amp, if you're okay taking that follow-up, feel free to pick it up separately.
@KaifAhmad1, approved from our side. The PR is ready for your final review and merge.
…graph_payload LPGExporter and ArangoAQLExporter called normalize_graph_payload() with no type guard, so non-mapping input raised ValidationError from inside the resolver while the YAML and Neo4j exporters raised ProcessingError for the identical mistake -- inconsistent with the exception-type contract this PR establishes. Both now use the shared _require_mapping() guard (moved from yaml_exporter.py into utils/helpers.py so all three can use it). Neo4jCSVExporter._normalize_graph checked isinstance(graph, dict), so a non-dict Mapping (MappingProxyType, ChainMap) fell through to the object-attribute branch and was rejected, even though the identical payload exported fine via the other three exporters. Now checks isinstance(graph, Mapping). normalize_graph_payload() accepts dataclass/attribute-bearing object records, but LPGExporter/ArangoAQLExporter call .get(...) directly on resolved entities -- an object-shaped record passed validation only to crash with a raw AttributeError once used, the exact failure this boundary exists to prevent. Records are now converted to plain dicts at the boundary (_coerce_records -> new _record_to_dict), so every consumer gets a uniform shape regardless of which reading the caller used. Two non-empty spellings of the same collection holding identical records in a different order were rejected as conflicting, since the check used plain list equality. Comparison is now an order-independent multiset of each record's canonical JSON form.
…rdening Documents the full arc of semantica-agi#958 -- the normalize_graph_payload() centralization, YAML input validation, both review rounds from @Sameer6305, and the exception-type/record-shape follow-up fixes -- plus closes semantica-agi#956, semantica-agi#952, semantica-agi#953.
…-validation # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Thanks @pravit-amp for the centralization work here, and @Sameer6305 for two thorough review rounds — this got the shared boundary to a solid place. Did a final pass before merge and found four more gaps that the earlier rounds didn't reach, all fixed in ae1d5958:
LPGExporter/ArangoAQLExportercallednormalize_graph_payload()with no type guard, so non-mapping input raisedValidationErrorfrom inside the resolver while YAML/Neo4jCSVExporterraisedProcessingErrorfor the identical mistake — inconsistent with the exception-type contract this PR itself documents. The_require_mapping()guard is now shared fromutils/helpers.pyand used by all three.Neo4jCSVExporter._normalize_graphcheckedisinstance(graph, dict), so a non-dictMapping(MappingProxyType,ChainMap) was rejected even though the identical payload exported fine via the other three exporters. Now checksisinstance(graph, Mapping).normalize_graph_payload()accepts dataclass/attribute-bearing object records, butLPGExporter/ArangoAQLExporterread them with.get(...)— an object-shaped record passed validation only to crash with a rawAttributeErroronce used. Records are now converted to plain dicts at the boundary (_coerce_records→ new_record_to_dict), so every consumer gets a uniform shape.- Two non-empty spellings of the same collection holding identical records in a different order were rejected as conflicting (plain list equality). Comparison is now an order-independent multiset of each record's canonical JSON form.
Added regression coverage for all four in tests/utils/test_normalize_graph_payload.py, updated 4 existing tests that asserted the old (unconverted) object-passthrough behavior, and merged main in to resolve the CHANGELOG.md conflict (d056a8df) — that was the only conflicting file, purely an adjacent-insertion clash with #957's entry.
Full suite post-merge: pytest tests/export tests/utils tests/context tests/test_export_module.py tests/test_export_methods_wrapper.py tests/test_notebooks_simulation.py — 771 passed, 4 skipped. black/isort/flake8 --max-line-length=88 clean on every touched line. python -m build succeeds. Also added a CHANGELOG entry (838639b3) covering the full arc of this PR: @pravit-amp's original centralization work, both of @Sameer6305's review rounds, and this follow-up.
Nice collaborative fix, @pravit-amp and @Sameer6305 — approving to merge.
3 participants
Description
Consolidates the YAML-export input-handling work into one PR. Supersedes #954, which carried the first commit alone.
The three changes are one story and are hard to review apart: #953's fix is written against the resolver added for #956, and #952 and #953 reject adjacent shapes of the same bad input through the same code path. All three issues are assigned to me.
Today, three different malformed inputs to
export_yamlfail three different ways — one loud but uninformative, one silent, one correct only by accident:After this PR the first two raise with an actionable message, and the third exports its records.
Type of Change
Related Issues
Closes #956
Closes #952
Closes #953
Changes Made
normalize_graph_payload()inutils.helpers([FEATURE] refactor(export): centralize graph-payload key normalization — four competing idioms across nine exporters produce divergent results #956) — graph payloads circulate under two vocabularies,entities/relationshipsandnodes/edges, and consumers each reconciled them locally with competing idioms, so the same payload could be exported, silently dropped, or rejected depending on which consumer read it. This becomes the single place that decision is made, adopted byLPGExporter,ArangoAQLExporterandNeo4jCSVExporter.LPGExporterin particular dropped every entity whennodeswas present but empty — the shapeJSONExporteremits.AttributeErrornaming neither the argument nor the expected shape. Rejected rather than wrapped: these formats distinguish entities from relationships from triplets, so guessing which one a list holds would mislabel the records.export_semantic_network,export_for_pipelineandexport_ontology_schemanow validate first.ContextGraph.to_dict()exports correctly — thenodes/edgesaliases come with the shared resolver, so the most direct path from this library's own graph type to YAML (used inexamples/capability_gap_context_graphs_example.py) no longer produces an empty file.export()serializes before creating the output directory, so a rejected export leaves nothing behind.Design notes for reviewers
ProcessingError, matchingNeo4jCSVExporter._normalize_graph. A mapping whose contents are unusable raisesValidationError, matchingnormalize_graph_payload._require_mappingruns first at every entry point, so a non-mapping never reaches the resolver. Happy to flatten to one type if you'd prefer.metadataalone does not make a payload recognized. Anexport_jsonenvelope carries one, so accepting it would readmit the exact silent-empty export this is most likely needed for.CSVExporterandJSONExporterare deliberately excluded from the resolver: they writeentities,relationships,nodesandedgesas separate outputs by design rather than reconciling two spellings of one collection, so normalizing there would rename output files.Testing
python -m build)New coverage:
tests/utils/test_normalize_graph_payload.py— resolver behaviour, plus end-to-end assertions run through the real exporters rather than mocks, since the behaviour under test is that the exporters now agree.tests/export/test_yaml_exporter_input_validation.py— non-mapping rejection, both directions, including that no file is written.tests/export/test_yaml_exporter_key_recognition.py— every input shape from [BUG] export_yaml silently writes an empty export when the input dict lacks its expected keys — no warning, logs report success #953's table, aContextGraph.to_dict()round-trip built from a real graph so it breaks ifto_dict()changes vocabulary, and an assertion that no success is logged for a rejected export.Test Commands
black,isortandflake8are clean on every line this PR touches. (blackstill reports pre-existing trailing whitespace in unrelated docstrings ofutils/helpers.pyandexport/methods.py, left alone to keep the diff scoped.)Documentation
semantica/export/export_usage.mdgains an "Accepted Input" section with the recognized key sets per method;docs/reference/export.mdnotes the contract inline. Every public method touched has itsRaises:section updated. No cookbook changes — no new examples were added.Breaking Changes
Breaking Changes: Yes
Three call patterns that previously returned cleanly now raise:
export_yaml({"data": [...]}, path)ValidationErrorexport_yaml({"nodes": [...]}, path, method="schema")classes/propertiesValidationErrorexport_yaml([...], path)AttributeErrorfrom inside the exporterProcessingErrorMigration: pass the records under a recognized key —
{"entities": [...]}/{"nodes": [...]}forsemantic_network,{"classes": [...]}forschema.This was raised on #953 before implementation, and raising was chosen over warning: the current logs affirmatively report success, so a caller has no way to detect the loss. Anyone whose call now raises was writing an empty file and losing data.
{}still exports, so genuinely-empty graphs are unaffected. Anodes/edgespayload that previously wrote an empty file now writes its records — a behaviour change, but in the direction the caller intended.Checklist
Additional Notes
Assignment: #953 was assigned by @KaifAhmad1 on the issue; #952 and #956 are also assigned to me.
Reviewable commit-by-commit — the four commits are ordered refactor → #952 → #952 review fixes → #953, each self-contained.
One follow-up deliberately left out of scope:
CSVExporterandJSONExporterstill emit both vocabularies as separate outputs. That is by design today, but worth a look if the project wants one canonical on-disk shape.