Validate Zotero credentials early and support configurable library type#212
Conversation
…upport configurable library type Signed-off-by: Nguyen Huy Hoang <181364121+huyhoang171106@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds early validation for Zotero configuration and allows selecting the Zotero library type (user vs group) when constructing the pyzotero client, addressing issue #85’s missing-credentials failure mode.
Changes:
- Introduced
normalize_zotero_settings()to validateuser_id,api_key, andlibrary_typeearly. - Wired normalized Zotero settings into
Executorinitialization andfetch_zotero_corpus(). - Wrapped Zotero API calls with a friendlier failure message.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| library_type = str(zotero_config.get("library_type", "user")).strip().lower() | ||
|
|
||
| if not user_id or not api_key: | ||
| raise ValueError("config.zotero.user_id and config.zotero.api_key must be set.") | ||
|
|
||
| if library_type not in {"user", "group"}: | ||
| raise ValueError('config.zotero.library_type must be either "user" or "group".') |
There was a problem hiding this comment.
library_type normalization treats an explicit null/None value as the string "none" (because str(None).lower()), which then fails validation. If users wire this to an env var with a null default (common with Hydra/OmegaConf), the config will error unexpectedly. Consider defaulting falsy values to "user" (e.g., apply or "user" before lowercasing) so missing/empty values behave like the intended default.
| collections = zot.everything(zot.collections()) | ||
| corpus = zot.everything(zot.items(itemType='conferencePaper || journalArticle || preprint')) | ||
| except Exception as e: | ||
| raise RuntimeError("Failed to access Zotero library. Please check user_id, api_key, and library_type.") from e |
There was a problem hiding this comment.
The except Exception wrapper always raises a RuntimeError suggesting credentials/library type are wrong, but the underlying failure could also be network issues, Zotero downtime, rate limiting, or an unexpected response shape. This can be misleading when debugging. Consider either narrowing the caught exceptions to Zotero/HTTP-related ones, or including the original exception type/message in the raised error so operators can distinguish credential problems from transient failures.
| raise RuntimeError("Failed to access Zotero library. Please check user_id, api_key, and library_type.") from e | |
| raise RuntimeError( | |
| "Failed to access Zotero library. This may be due to invalid user_id/api_key/library_type " | |
| f"or a transient Zotero/network issue. Original error: {type(e).__name__}: {e}" | |
| ) from e |
| def normalize_zotero_settings(zotero_config: DictConfig) -> tuple[str, str, str]: | ||
| user_id = str(zotero_config.get("user_id") or "").strip() | ||
| api_key = str(zotero_config.get("api_key") or "").strip() | ||
| library_type = str(zotero_config.get("library_type", "user")).strip().lower() | ||
|
|
||
| if not user_id or not api_key: | ||
| raise ValueError("config.zotero.user_id and config.zotero.api_key must be set.") | ||
|
|
||
| if library_type not in {"user", "group"}: | ||
| raise ValueError('config.zotero.library_type must be either "user" or "group".') | ||
|
|
||
| return user_id, api_key, library_type |
There was a problem hiding this comment.
normalize_zotero_settings introduces new validation behavior (required user_id/api_key, allowed library_type values) but there are no unit tests covering these cases. Since normalize_include_path_patterns has dedicated tests, adding similar tests for Zotero settings would prevent regressions (e.g., empty env vars, invalid library_type, default behavior).
| if library_type not in {"user", "group"}: | ||
| raise ValueError('config.zotero.library_type must be either "user" or "group".') |
There was a problem hiding this comment.
This adds a new config.zotero.library_type setting, but the repository’s shipped Hydra configs (config/base.yaml / config/custom.yaml) currently don’t mention it, so users won’t discover how to configure group libraries. Please update the default/custom config docs (and/or README) to document the new field and its expected values ("user"|"group").
Summary
Validate Zotero credentials early and support configurable library type.
Files changed
src/zotero_arxiv_daily/executor.py(modified)Testing
Closes #85