feat(units_v4): add Units V4 coverage

[lkubie]

Add Coverage of Units V4 to SDK

[claude]

Code Review

Issues Found

• PR title Lkubie/unitsv4 does not follow Conventional Commits. It must be prefixed, e.g. feat(units_v4): add v4 units and unit families coverage. Release-please derives the changelog and version bump from the title, so a non-conventional title breaks the release flow.
• [src/albert/collections/units_v4.py:209] and [src/albert/collections/unit_families_v4.py:195] search() is missing @validate_call. Every other public method in these two files has it, and every other search() in the repo (activities, substance_v4, users) is decorated. Without it, the status/origin/type/order_by enum arguments are not validated or coerced, diverging from the established pattern.
• [src/albert/collections/units_v4.py:903] get_by_ids reads only response.json().get("items"), while unit_families_v4.get_by_ids defensively reads data.get("items") or data.get("Items"). If the units search endpoint returns the list under Items, this silently yields an empty result. Align the two.

Summary

3 issues found. The PR title blocks the release pipeline; the missing @validate_call is a clear pattern/validation gap; the get_by_ids key handling is a latent inconsistency worth aligning before the v4 endpoint goes live.

[prasad-albert]

Since multiple v4 APIs (units, unit-families, and upcoming v4 services) share this exact same contract (application/merge-patch+json + If-Match with ETag), we should avoid having each collection implement its own internal _get_with_version() and diffing logic.

Specifically, fetching the entity inside update() introduces two bugs:

1. Lost Updates: It compares the local object against the newest DB version and sends the newest ETag, silently overwriting concurrent edits instead of letting DynamoDB trigger a 412 conflict.
2. OpenSearch 404s: GET reads from OpenSearch (which has indexing lag), so updating a unit right after creation throws an immediate 404.

Proposed shared abstraction:

1. In BaseResource, add _etag: str | None = PrivateAttr(default=None) and an optional _snapshot() of initial values.
2. When get_by_id or create receives a response, set instance._etag = response.headers.get("ETag").
3. Add a reusable _execute_merge_patch(...) in BaseCollection (or a utility helper) that diffs against _initial_values (or model_fields_set), sends If-Match: resource._etag, and populates the new ETag on the returned model.

This eliminates the hidden GET, cuts round-trips in half, makes OCC work properly, and provides a reusable foundation for all v4 collections.

Feel free to implement any other abstraction

[prasad-albert]

UnitV4 requires name, type, and symbol with no default values (and UnitFamilyV4 requires name and type). This prevents callers from constructing standalone update objects:

# Fails with Pydantic ValidationError (missing name, type, symbol):
client.units_v4.update(unit=UnitV4(id="UNI123", description="New description"))

If a caller passes dummy values to satisfy Pydantic (e.g. name="placeholder"), those fields enter model_fields_set and overwrite server values.

Suggested fix:
Give non-ID fields sensible defaults (default=None) on the resource models:

name: str | None = None
type: UnitV4Type | None = None
symbol: str | None = None

Alternatively, validate required fields during create() (or provide dedicated request models).

[prasad-albert]

A couple of minor API validation gaps to align with the rest of the SDK:

1. Missing @validate_call on search():
   Both UnitV4Collection.search() and UnitFamilyV4Collection.search() omit @validate_call. All public collection methods in the SDK use @validate_call to ensure enum parameters (status, origin, type, order_by) are validated and coerced properly at runtime.
2. merge() parameter validation:
   MergeUnitDto in invent-core enforces @ArrayMinSize(1) on childIds. Adding a check that child_ids is non-empty in UnitV4Collection.merge() provides fast client-side feedback before hitting the network.

[prasad-albert]

Both test suites currently disable execution with:

pytestmark = pytest.mark.skip(
    reason="The v4 units API (master-data) is not yet available in the test environment."
)

The v4 master-data API is actually live and accessible on staging (endpoints like lookup, get_compatible, create, and delete respond normally).

For convertible units, if the test environment lacks pre-existing global reference units, using expressions (e.g., unit_expression="kg*m/s^2" or ref_unit_exp="1000*g") resolves dynamically via CalcEngine without needing pre-existing data. We should unskip these suites so CI can continuously exercise them.

[claude]

Code Review

Issues Found

• [src/albert/collections/units_v4.py:210] search() is missing the @validate_call decorator. Every other public method in this collection (and the convention in substance_v4.search/workflows.search) uses it. Without it, the status/origin/type/order_by/max_items arguments are not validated or coerced (an out-of-range enum string passes straight through instead of failing loudly).
• [src/albert/collections/unit_families_v4.py:195] Same issue: search() is missing @validate_call while all sibling methods have it.
• [PR title] Title Lkubie/unitsv4 does not follow Conventional Commits and will break the release-please / commit-lint flow. It should be e.g. feat(units): add units_v4 and unit_families_v4 coverage.

Summary
3 issues found. Non-crashing but real: the two missing @validate_call decorators drop input validation on the public search APIs, and the non-conventional PR title blocks the release/changelog tooling. Core logic (merge-patch builders, create serialization, UUID/legacy ID handling, pagination) looks correct.

[claude]

Code Review

Issues Found

1. PR title. Lkubie/unitsv4 does not follow Conventional Commits. It should be like feat(units_v4): add Units V4 coverage. This PR itself adds a pr-title workflow to enforce conventional titles, so the current title will fail that check and produce a bad changelog entry on squash-merge.

2. src/albert/collections/units_v4.py (search, ~line 210). The search() method is missing the @validate_call decorator. Every other collection search (activities, lots, inventory, substance_v4, etc.) carries it. Without it, the enum/list args (status, origin, type, order_by) are not validated or coerced, so a caller passing a raw string bypasses validation instead of getting a clear error.

3. src/albert/collections/unit_families_v4.py (search, ~line 195). Same issue: search() is missing @validate_call.

Summary

3 issues found. No functional runtime bugs in the units_v4 logic (serialization, pagination mode, merge-patch diffing, and client registration all check out); the findings are convention/consistency issues, the most impactful being the non-conventional PR title which the new pr-title workflow will reject.

[claude]

Code Review

Issues Found:

• src/albert/collections/units_v4.py:905 — get_by_ids reads the batch response with response.json().get("items") only, while the sibling unit_families_v4.get_by_ids (unit_families_v4.py:501) reads data.get("items") or data.get("Items") for the same /search endpoint shape. If the endpoint returns the list under "Items" (capitalized), units_v4.get_by_ids silently returns an empty list instead of the matching units. Integration tests are skipped (master-data API not yet available), so this casing is unverified. Make both consistent (read both keys).

Summary: 1 minor issue found. Low-severity latent inconsistency in response-key handling; no other correctness, serialization, pagination, or docstring problems. @validate_call, model_dump(by_alias=True, mode="json"), OFFSET pagination wiring, str-based enums, and modern type hints are all correct.

[claude]

Looks good - no bugs or correctness issues found.