Skip to content

feat(substance_v4): add SubstanceCollectionV4 wrapping v4 substance API#460

Open
prasad-albert wants to merge 5 commits intonextfrom
worktree-substance-v4
Open

feat(substance_v4): add SubstanceCollectionV4 wrapping v4 substance API#460
prasad-albert wants to merge 5 commits intonextfrom
worktree-substance-v4

Conversation

@prasad-albert
Copy link
Copy Markdown
Collaborator

@prasad-albert prasad-albert commented Apr 8, 2026

Summary

  • Adds SubstanceCollectionV4 at client.substances_v4 wrapping the new /api/v4/substances API
  • Implements get_by_ids, get_by_id, search (with paginator), create, and update_metadata
  • Adds SubstanceV4Info, SubstanceV4SearchItem, SubstanceV4Create, SubstanceV4CreateResult, SubstanceV4Metadata, and SubstanceV4Response resource models
  • Fully additive and non-breaking — v3 SubstanceCollection is untouched; V4 suffix will be stripped in SDK 2.0 when v3 is deprecated

@claude
Copy link
Copy Markdown

claude bot commented Apr 8, 2026

Code Review

Issues Found

  • [src/albert/collections/substance_v4.py:86] The class-level Methods section documents update_metadata(id, updated) but the actual parameter is named metadata. Per project standards, the Methods section must accurately reflect the real signature.

Mentions

  • [src/albert/resources/substance_v4.py:238] MetadataPatchOperationV4 is defined but never used anywhere in the PR. Per project standards, minimum code that solves the problem — no speculative abstractions.

Summary

1 docstring correctness issue, 1 pattern deviation. Neither blocks functionality, but the wrong param name in the class docstring will appear in generated docs.

@prasad-albert prasad-albert changed the base branch from main to next April 8, 2026 11:40
@prasad-albert
Copy link
Copy Markdown
Collaborator Author

@claude review the PR again

@claude
Copy link
Copy Markdown

claude bot commented Apr 9, 2026

Claude finished @prasad-albert's task in 2m 36s —— View job


Code Review

  • Read changed files
  • Review collection implementation
  • Review resource models
  • Review tests
  • Review docs/config changes
  • Post findings

Previous issues (param name, unused MetadataPatchOperationV4) are both resolved. ✅

Issues Found

[src/albert/collections/substance_v4.py:202-210] get_by_id delegates to get_by_ids and blindly indexes [0]. If the caller passes catch_errors=True with an unknown identifier, the API may return an empty substances list — resulting in an IndexError rather than a meaningful error. Since catch_errors is an explicit parameter on get_by_id, this is a reachable code path.

return self.get_by_ids(
    cas_ids=[cas_id] if cas_id else None,
    ...
)[0]  # IndexError if substances list is empty

Suggest a guard:

results = self.get_by_ids(...)
if not results:
    raise ValueError(f"No substance found for the provided identifier.")
return results[0]

Fix this →

Nits

  • [src/albert/collections/substance_v4.py:229] The search docstring says "the advanced filters take precedence" over search_key — this is backend behaviour, not enforced by the SDK. This is informative, but it's worth mentioning it's API-side behaviour to avoid implying the SDK enforces it.

  • [src/albert/resources/substance_v4.py:8] SubstanceV4SearchItem lacks extra="allow", while SubstanceV4Info (line 93) has ConfigDict(extra="allow"). If the v4 search endpoint adds fields in future, SubstanceV4SearchItem will silently drop them. Fine if the search response shape is stable and intentionally more constrained.

Everything Else

  • SubstanceV4SearchPaginator: correctly sets self._offset before super().__init__() so the overridden _create_iterator sees it; overrides _create_iterator to read "substances" key (correct, since the base looks for "Items"); setdefault("limit", DEFAULT_LIMIT) in the base is a no-op because limit=20 is already in params before super().__init__() is called. ✅
  • get_by_ids validation, update_metadata operations loop, create payload wrapping — all correct. ✅
  • Resource models: aliases consistent with wire format, populate_by_name=True on BaseAlbertModel means SubstanceV4Identifier(attribute_name="casID") in tests works correctly. ✅
  • Tests: integration-style only, good coverage (get, search variants, max_items, create round-trip via existing items, update_metadata). ✅
  • Docs pages and mkdocs.yml entries present. ✅

Summary

One real issue (get_by_id can raise IndexError with catch_errors=True), two nits. Everything from the prior review is cleanly addressed.

@prasad-albert
Copy link
Copy Markdown
Collaborator Author

  • [src/albert/resources/substance_v4.py:8] SubstanceV4SearchItem lacks extra="allow", while SubstanceV4Info (line 93) has ConfigDict(extra="allow"). If the v4 search endpoint adds fields in future, SubstanceV4SearchItem will silently drop them. Fine if the search response shape is stable and intentionally more constrained.

@claude Fixed the primary issue
SubstanceV4SearchItem inherits from the BaseAlbertModel following the standard pattern in the SDK.

@claude
Copy link
Copy Markdown

claude bot commented Apr 9, 2026

Claude finished @prasad-albert's task in 42s —— View job


Code Review Update

  • Read trigger comment
  • Verify get_by_id guard fix
  • Review SubstanceV4SearchItem pattern explanation
  • Post findings

Primary issue (get_by_id IndexError) — Fixed. ✅

results = self.get_by_ids(...)
if not results:
    raise ValueError("No substance found for the provided identifier.")
return results[0]

Nit (SubstanceV4SearchItem missing extra="allow") — Acknowledged. BaseAlbertModel itself doesn't include extra="allow" (confirmed in src/albert/core/base.py), so SubstanceV4SearchItem is correctly following the SDK standard. SubstanceV4Info opts in explicitly with ConfigDict(extra="allow"), which is appropriate for the richer model. The nit stands as informational only — no action required.

All prior issues are resolved. PR looks good. ✅

prasad-albert and others added 4 commits April 9, 2026 17:40
- Adds SubstanceCollectionV4 at client.substances_v4 with get_by_ids,
  get_by_id, search (paginated), create, and update_metadata
- Adds resource models: SubstanceV4Info, SubstanceV4SearchItem,
  SubstanceV4Create, SubstanceV4CreateResult, SubstanceV4Metadata,
  SubstanceV4Response
- Non-breaking: v3 SubstanceCollection untouched; V4 suffix stripped in SDK 2.0

Co-Authored-By: Claude Code <claude-code@anthropic.com>
Co-Authored-By: Claude Code <claude-code@anthropic.com>
Co-Authored-By: Claude Code <claude-code@anthropic.com>
…ding backend fixes

Co-Authored-By: Claude Code <claude-code@anthropic.com>
@prasad-albert prasad-albert force-pushed the worktree-substance-v4 branch from f47cee7 to b7d08d6 Compare April 9, 2026 12:11
Co-Authored-By: Claude Code <claude-code@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant