Add file adapter for syncing from local CSV exports#131
Conversation
|
Warning Review limit reached
More reviews will be available in 33 minutes and 46 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis pull request introduces a new read-only file adapter for Infrahub that loads CSV exports into DiffSync models. The implementation includes a CSV reader with configurable delimiters and encoding, a FileAdapter class that resolves file paths relative to a configured base directory and reads records via registered format readers, and record conversion logic that derives local identifiers and applies schema field mappings including static values, direct scalar mappings, delimited list splitting, and reference field resolution. A FileModel base class marks the adapter as source-only. Comprehensive tests verify CSV parsing, end-to-end model loading with identifier and reference resolution, and error handling for unsupported formats and missing files. Documentation and example configuration demonstrate the adapter's capabilities and usage. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/adapters/test_file.py (1)
87-100: ⚡ Quick winAdd an explicit empty-page CSV test for the reader/adapter path.
Please add a test for an empty page case (e.g., header-only CSV and/or fully empty CSV) to validate the no-record behavior explicitly.
As per coding guidelines: "Unit tests for utils and adapter edge cases (timeouts, 401/403, empty pages); parametrized tests for config parsing; keep tests atomic and single-purpose".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/adapters/test_file.py` around lines 87 - 100, Add a new atomic unit test in tests/adapters/test_file.py that verifies read_csv returns an empty list for empty-page CSVs: create a tmp_path file for two cases (fully empty file and header-only file like "name,description\n"), call read_csv(path, settings={}) for each, and assert the result is [] to explicitly cover the no-record edge case for the reader/adapter; reference the existing test helpers and the read_csv function when locating where to add the new test(s).examples/file_to_infrahub/config.yml (1)
7-12: ⚡ Quick winUse a local relative
directoryand trim default CSV options in the example.This example is less portable than necessary. Using
examples/file_to_infrahub/dataties it to repo-root execution, anddelimiter/encodingrepeat defaults.Proposed cleanup
source: name: file settings: - # Base directory used to resolve relative file paths below. - # Defaults to this config's directory when omitted. - directory: "examples/file_to_infrahub/data" - # CSV options (applied to all .csv files) - delimiter: "," - encoding: "utf-8" + # Base directory used to resolve relative file paths below. + directory: "data"As per coding guidelines: "examples/**/*.{yaml,yml}: Example configurations must be minimal, accurate, and redacted of sensitive information".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/file_to_infrahub/config.yml` around lines 7 - 12, The example config uses a repo-root-dependent directory and repeats default CSV options; change the directory key to a local relative value (e.g., "directory: ./data" or remove the directory key entirely to use the config's directory) and remove the redundant CSV option keys "delimiter" and "encoding" so the example is minimal and relies on defaults.infrahub_sync/adapters/file.py (1)
4-4: ⚡ Quick winAlign logging with repo convention (or migrate to
structlogproject-wide).
infrahub_sync/adapters/file.pyuses stdliblogging(import logging/logging.getLogger(__name__)), and the other adapters underinfrahub_sync/adapters/also use stdliblogging(nostructlogusage found). Ifstructlogis a required standard, this file should be updated as part of a broader convention change.The
config.source.name.title() == self.type.title()gating matches sibling adapters, so that concern is resolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infrahub_sync/adapters/file.py` at line 4, This file currently imports the stdlib logging but doesn't follow the repo's logger pattern; add a module-level logger (e.g., logger = logging.getLogger(__name__)) and replace any direct logging.* calls with logger.* to match the other adapters, or if the repo standard is to use structlog, replace the import with structlog and create a logger via structlog.get_logger() and update all logger usages accordingly (ensure you update the module-level logger symbol so other functions/classes in this file use the same logger).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@examples/file_to_infrahub/config.yml`:
- Around line 7-12: The example config uses a repo-root-dependent directory and
repeats default CSV options; change the directory key to a local relative value
(e.g., "directory: ./data" or remove the directory key entirely to use the
config's directory) and remove the redundant CSV option keys "delimiter" and
"encoding" so the example is minimal and relies on defaults.
In `@infrahub_sync/adapters/file.py`:
- Line 4: This file currently imports the stdlib logging but doesn't follow the
repo's logger pattern; add a module-level logger (e.g., logger =
logging.getLogger(__name__)) and replace any direct logging.* calls with
logger.* to match the other adapters, or if the repo standard is to use
structlog, replace the import with structlog and create a logger via
structlog.get_logger() and update all logger usages accordingly (ensure you
update the module-level logger symbol so other functions/classes in this file
use the same logger).
In `@tests/adapters/test_file.py`:
- Around line 87-100: Add a new atomic unit test in tests/adapters/test_file.py
that verifies read_csv returns an empty list for empty-page CSVs: create a
tmp_path file for two cases (fully empty file and header-only file like
"name,description\n"), call read_csv(path, settings={}) for each, and assert the
result is [] to explicitly cover the no-record edge case for the reader/adapter;
reference the existing test helpers and the read_csv function when locating
where to add the new test(s).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b518736a-d408-4931-9c59-5c8f8e065a33
⛔ Files ignored due to path filters (2)
examples/file_to_infrahub/data/devices.csvis excluded by!**/*.csvexamples/file_to_infrahub/data/organizations.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
docs/docs/adapters/file.mdxdocs/sidebars.tsexamples/file_to_infrahub/config.ymlinfrahub_sync/adapters/file.pytests/adapters/test_file.py
- Simplify example config: drop repo-root-bound directory, redundant
delimiter/encoding defaults; prefix mappings with data/
Flat-file sources naturally produce duplicate identifiers when a model is derived from a foreign-key column (e.g. one LocationSite per unique 'location' value across many device rows). Previously the second occurrence raised ObjectAlreadyExists and aborted the load. Now the first occurrence wins and a summary count is logged.
There was a problem hiding this comment.
4 high-severity items below were reproduced by running the adapter against the real code. Findings are grouped:
- correctness first (ranked by severity/confidence)
- cleanup/efficiency
The example config, constructor signature, reader-registry dispatch, and source.name.title() gate all checked out.
| obj_id = obj.get("id") | ||
| if obj_id is None: | ||
| for key, value in obj.items(): | ||
| if key.endswith("_id") and value: |
There was a problem hiding this comment.
[Correctness — HIGH] Ragged CSV row → uncaught AttributeError.
When a data row has more fields than the header, csv.DictReader stores the surplus under a None key (restkey). _derive_local_id then iterates obj.items() and calls None.endswith("_id").
Reproduced against the adapter:
Header 'name,description' + row 'router,core,EXTRA1,EXTRA2'
→ {'name':'router','description':'core', None:['EXTRA1','EXTRA2']}
→ AttributeError: 'NoneType' object has no attribute 'endswith'
The try/except in model_loader only wraps _read_file, not the obj_to_diffsync loop, so this propagates uncaught.
Fix: guard the key — if key and key.endswith("_id") and value: — and/or strip the restkey=None entry in read_csv.
| list_separator = self.settings.get("list_separator", ";") | ||
|
|
||
| for field in mapping.fields: | ||
| field_is_list = model.is_list(name=field.name) |
There was a problem hiding this comment.
[Correctness — HIGH] List-attribute splitting silently fails for real generated models.
is_list() keys off isinstance(field.default, list) (__init__.py:281). The generator emits an optional List attribute as list[Any] | None = None (no list default), so is_list() returns False, this branch is skipped, and the raw string is stored — pydantic then raises ValidationError: Input should be a valid list.
Reproduced:
aliases: list[Any] | None = None # what get_kind emits
is_list('aliases') → False
model_loader(... 'a;b;c') → ValidationErrorThe advertised "a tags column with core;edge becomes ['core','edge']" only works because the test hand-writes tags: list[str] = [], which is not how the generator represents List attributes. (Cardinality-many relationships get = [] and do work.)
Fix: make is_list inspect the type annotation rather than the default, and add a test using the generated list[Any] | None = None form.
| empty_as_none = settings.get("empty_as_none", True) | ||
|
|
||
| records: list[dict[str, Any]] = [] | ||
| with path.open(newline="", encoding=encoding) as handle: |
There was a problem hiding this comment.
[Correctness — MED/HIGH] BOM not stripped; first column silently mis-read.
Opening with encoding="utf-8" (not utf-8-sig) leaves a UTF-8 BOM attached to the first header. Excel-exported CSVs are the primary use case for this adapter.
Reproduced:
fieldnames = ['\ufeffname', 'description']
get_value(obj, 'name') → None # for every row
The primary column loads empty, identifier derivation falls back to the row index, and a required attribute fails validation — a confusing failure on a common input.
Fix: path.open(newline="", encoding=encoding) → use "utf-8-sig" as the default encoding (or strip a leading BOM from the first fieldname).
| if not field_is_list: | ||
| if value is not None: | ||
| # Flat files reference another object directly by its identifier. | ||
| data[field.name] = value |
There was a problem hiding this comment.
[Correctness — MED] References stored as the raw cell value; breaks for composite-key targets.
Unlike every sibling adapter (which resolves the reference against self.store and emits node.get_unique_id()), this writes the raw column value into the relationship field. That only works when the target model has a single identifier.
For a target keyed by multiple identifiers (e.g. name + region), the destination computes the peer unique_id as 'site-a__region' (identifiers joined by __), but the file adapter stores 'site-a' — they never match, so the relationship perpetually diffs as changed or silently drops.
Fix: reuse adapters/utils.build_mapping (which exists exactly for joining a target's _identifiers), or at minimum document the single-identifier limitation. Same applies to the list-reference branch just below (line 234).
| item = model(**data) | ||
| try: | ||
| self.add(item) | ||
| except ObjectAlreadyExists: |
There was a problem hiding this comment.
[Correctness — MED] Duplicate-skip swallows genuine collisions silently.
Catching ObjectAlreadyExists and only logging a count means rows that collide on the derived identifier but carry different attributes are silently discarded. get_unique_id() is built from the schema _identifiers, so the swallowed collision is a real primary-key collision, not just the FK-derived-duplicate case the comment describes.
Example: device sw1 appears twice with descriptions A then B → the second is dropped, the user sees only an INFO-level skipped N duplicate count and never learns which records vanished.
Fix: log each dropped unique_id at WARNING, and/or offer a strict mode that raises on genuine collisions vs. the intentional FK-dedup path.
|
|
||
| records: list[dict[str, Any]] = [] | ||
| with path.open(newline="", encoding=encoding) as handle: | ||
| reader = csv.DictReader(handle, delimiter=delimiter) |
There was a problem hiding this comment.
[Correctness — LOW] Duplicate header columns silently overwrite.
csv.DictReader keeps only the last value when two headers share a name (e.g. name,name,description → the first name is dropped). Flat exports with repeated column names corrupt silently. Consider detecting duplicate fieldnames and warning.
| element.mapping, | ||
| ) | ||
|
|
||
| def _derive_local_id(self, obj: dict[str, Any], mapping: SchemaMappingModel, index: int) -> str: |
There was a problem hiding this comment.
[Reuse] _derive_local_id duplicates utils.derive_identifier_key — and its output is never read.
The id/*_id scan (lines 185-190) is a copy of derive_identifier_key (utils.py:59), a rule also re-implemented in prometheus.py:39 — three copies that drift independently. More importantly, unlike sibling adapters the file adapter never consumes local_id for reference resolution (references use raw values; identity uses get_unique_id), so the elaborate 3-tier derivation produces an output nothing reads.
Fix: delegate to derive_identifier_key for the id/*_id portion, and reconsider whether the identifier/index fallbacks are needed at all.
|
|
||
| return reader(path, self.settings) | ||
|
|
||
| def model_loader(self, model_name: str, model: type[FileModel]) -> None: |
There was a problem hiding this comment.
[Reuse] model_loader / obj_to_diffsync are near-verbatim copies of genericrestapi.
The schema_mapping scan, the source.name.title() filter/transform gate, and the 4-branch field-dispatch ladder are duplicated across genericrestapi/nautobot/netbox/aci/prometheus/file. Each adapter really only differs in how it fetches raw objects.
Fix (follow-up, not blocking): lift the shared load skeleton into DiffSyncMixin with only the fetch step overridden, so the contract can't drift per adapter.
|
|
||
| list_separator = self.settings.get("list_separator", ";") | ||
|
|
||
| for field in mapping.fields: |
There was a problem hiding this comment.
[Efficiency] Per-row recomputation in the hot path.
model.is_list(name=field.name) (getattr-fallback + dict lookup) and self.settings.get("list_separator", ";") (line 212) are recomputed for every field of every row, though both are constant per model. For a 100k-row file with M fields that's 100k×M is_list calls.
Fix: compute list_separator once in __init__, and build a per-model {field.name: is_list} map above the row loop.
| class FileModel(DiffSyncModelMixin, DiffSyncModel): | ||
| """Model for the file adapter. The file adapter is read-only (source) for now.""" | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
[Simplification / Altitude] Dead overrides + speculative registry.
FileModel.create/update are no-op super() passthroughs that add maintenance burden with no behavior over inheriting. The "read-only source" contract is asserted by a comment, not enforced — if File were configured as a destination, super().create() would report SUCCESS while writing nothing. Consider enforcing source-only at the mixin level (raise NotImplementedError).
Separately, the FILE_READERS/EXTENSION_FORMATS two-dict registry + format-override plumbing (line 56) is speculative for a single CSV reader (YAGNI) — fine to keep if the multi-format roadmap is near, but it's currently dead surface.
2 participants
Summary by CodeRabbit
New Features
Documentation
Examples
Tests