feat(imports): add DataProvider / OLImportRecord base classes#437
Closed
mekarpeles wants to merge 4 commits into
Closed
feat(imports): add DataProvider / OLImportRecord base classes#437mekarpeles wants to merge 4 commits into
mekarpeles wants to merge 4 commits into
Conversation
The save_many endpoint posts JSON but was missing the Content-Type header, relying on the server to guess the encoding.
…tput Edition, Work, and Author json() methods now exclude revision, latest_revision, created, and last_modified — fields set by the OL server that are invalid in write payloads. READONLY_FIELDS is defined once in common.py and shared by all three entity classes. The three JSON schemas are updated in parallel: these fields remain defined as valid properties but are removed from required[], since save payloads never need to include them.
… API dicts ol.load(doc) dispatches on doc['type']['key'] and returns the appropriate typed object (Edition, Work, Author, Delete, Redirect). This is the intended entry point for the "raw dict → save_many" path: ol.save_many([ol.load(doc) for doc in raw_docs], comment) Author references inside Edition docs are resolved to stub Author objects (olid only, no network fetch) — sufficient for save_many, which only needs the key reference.
Adds olclient/imports.py with the reusable primitives for bulk import adapters: OLImportRecord (Pydantic, mirrors import.schema.json), DataProviderRecord, DataProvider, JSONLProvider, and PaginatedAPIProvider. Concrete source adapters (e.g. ITANProvider) live in openlibrary-bots/sources/<slug>/ and import from this module. Related: internetarchive/openlibrary#12091
There was a problem hiding this comment.
Pull request overview
Adds a new olclient.imports module with reusable base classes/models for bulk import adapters, and aligns existing OL entity serialization/validation so server-managed readonly fields aren’t included in write payloads (plus a new OpenLibrary.load() helper for round-tripping API docs into save-ready entities).
Changes:
- Introduces
olclient/imports.pywithDataProvider,DataProviderRecord,OLImportRecord, and provider helpers likeJSONLProvider. - Adds
READONLY_FIELDSand updates Work/Edition/Author.json()implementations (plus newOpenLibrary.load()) to strip server-managed fields from save payloads. - Updates entity JSON Schemas to stop requiring readonly fields; expands unit tests to cover load/roundtrip behavior and save_many headers.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_openlibrary.py | Adds tests for readonly-field stripping, OpenLibrary.load(), and save_many Content-Type header. |
| olclient/schemata/work.schema.json | Removes readonly fields from the required list to match save-ready payloads. |
| olclient/schemata/edition.schema.json | Removes readonly fields from the required list to match save-ready payloads. |
| olclient/schemata/author.schema.json | Removes readonly fields from the required list to match save-ready payloads. |
| olclient/openlibrary.py | Adds Content-Type for save_many, strips readonly fields in .json(), and adds load() dispatcher. |
| olclient/imports.py | New bulk-import primitives and provider base classes (including JSONL streaming). |
| olclient/entity_helpers/work.py | Strips readonly fields from Work .json() output. |
| olclient/common.py | Defines shared READONLY_FIELDS constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+163
to
+175
| def iter_records(self) -> Iterator[DataProviderRecord]: | ||
| with urllib.request.urlopen(self.SOURCE_URL) as f: | ||
| for lineno, raw_line in enumerate(f, start=1): | ||
| line = raw_line.strip() | ||
| if not line: | ||
| continue | ||
| try: | ||
| data = json.loads(line) | ||
| yield self.RECORD_CLASS.model_validate(data) | ||
| except Exception as exc: | ||
| log.warning( | ||
| "%s: skipping line %d — %s", self.__class__.__name__, lineno, exc | ||
| ) |
Comment on lines
+764
to
+766
| work_olid = ( | ||
| doc.pop('works')[0]['key'].split('/')[-1] if 'works' in doc else None | ||
| ) |
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
olclient/imports.pywith reusable primitives for bulk import adaptersOLImportRecord— Pydantic model mirroringopenlibrary/schemata/import.schema.json(all fields,additionalProperties: false)DataProviderRecord— abstract base; subclass with source-native fields, implementto_ol_import() → OLImportRecord | NoneDataProvider— abstract traversal base; subclass and implementiter_records()JSONLProvider— mixin that streams a local path or remote JSONL URL line-by-line, with per-line error handlingPaginatedAPIProvider— stub base for paginated REST API traversalUsage
Concrete source adapters live in
openlibrary-bots/sources/<slug>/and import from this module.See
internetarchive/openlibrary-bots(feat/sources-itan) for the first concrete adapter (ITAN Global Publishing).Related