Skip to content

add support for get_by_ids and count#95

Open
harrisonzhao wants to merge 2 commits into
ioxiocom:mainfrom
harrisonzhao:main
Open

add support for get_by_ids and count#95
harrisonzhao wants to merge 2 commits into
ioxiocom:mainfrom
harrisonzhao:main

Conversation

@harrisonzhao

Copy link
Copy Markdown

support sync and async

@ioxiocom ioxiocom deleted a comment from arsenyinfo Jun 15, 2026
@arsenyinfo

Copy link
Copy Markdown

increment silently skips local update for nested dotted field paths

  • Priority: P2
  • Location: firedantic/_async/model.py:127-128, firedantic/_sync/model.py:127-128
  • Scenario: When increment("owner.first_name", 1) or any dotted field path is passed, the DB write succeeds but the local model is not updated due to if "." in field: return guard
  • Potential solution: Implement nested field traversal for local update, or raise an exception when a dotted path is passed, or document the limitation in the docstring

increment alias resolution ignores Pydantic v2 serialization_alias and validation_alias

  • Priority: P2
  • Location: firedantic/_async/model.py:131-137, firedantic/_sync/model.py:131-137
  • Scenario: A field declared with Field(serialization_alias="count_v2") is saved to Firestore under "count_v2" via model_dump(by_alias=True), but increment("count_v2", 1) fails to resolve the local field because the alias-matching loop only checks field_info.alias, not serialization_alias or validation_alias
  • Potential solution: Extend the alias check to compare field_info.serialization_alias and field_info.validation_alias in addition to field_info.alias

increment silently stores wrong-typed value on local model when amount type doesn't match field type

  • Priority: P2
  • Location: firedantic/_async/model.py:139-146, firedantic/_sync/model.py:139-146
  • Scenario: A field declared as stock: int with current value 3 and amount=0.5 stores 3.5 (float) directly in __dict__ without validation or coercion because validate_assignment=True is not configured; this diverges from Firestore's stored value and from Pydantic's constructor coercion on subsequent reload()
  • Potential solution: Coerce value to the declared field annotation before setattr, or skip local update when types are incompatible, or enable validate_assignment=True on the base model

increment local update is unsafe inside a transaction — model becomes stale on rollback

  • Priority: P2
  • Location: firedantic/_async/model.py:118-124, firedantic/_sync/model.py:118-124
  • Scenario: When increment is called within a @transactional decorated function, transaction.update(doc_ref, data) is a buffered write that only takes effect on commit, but _increment_field_locally at line 124 mutates the in-memory model unconditionally; if the transaction aborts, the local model holds a never-applied increment
  • Potential solution: Skip _increment_field_locally when transaction is not None, letting the caller reload the model after the transaction commits

SubModel.get_by_id and SubModel.get_by_ids use wrong type variable bound to base model instead of submodel

  • Priority: P3
  • Location: firedantic/_async/model.py:560-565 (get_by_id), firedantic/_async/model.py:575-580 (get_by_ids); firedantic/_sync/model.py:558-563 (get_by_id), firedantic/_sync/model.py:570-575 (get_by_ids)
  • Scenario: Both methods use cls: Type[TAsyncBareModel] / Type[TBareModel] and return TAsyncBareModel / List[TBareModel], but these type variables are bound to AsyncBareModel / BareModel instead of AsyncBareSubModel / BareSubModel; the new get_by_ids copies the same incorrect pattern from pre-existing get_by_id
  • Potential solution: Change TAsyncBareModel to TAsyncBareSubModel in both AsyncSubModel.get_by_id and AsyncSubModel.get_by_ids, and TBareModel to TBareSubModel in both SubModel.get_by_id and SubModel.get_by_ids
  • Uncertainty: Type-checking issue only; no runtime effect

🔍 Reviewed by nitpicker

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.

2 participants