update docs, upgrade signals - #430
Conversation
Changes: - allow signals to interrupt softly deletions and other operations - add inject_filters parameter - update docs - add operation_skipped signal parameter - improve tests for signals
There was a problem hiding this comment.
6 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="edgy/core/db/relationships/relation.py">
<violation number="1" location="edgy/core/db/relationships/relation.py:182">
P2: A skipped many-to-many removal can dispatch `post_relation_remove` before all `pre_relation_remove` receivers have completed. Await or cancel/drain outstanding pre-signal tasks before emitting post-signals to preserve pre/post lifecycle ordering.</violation>
<violation number="2" location="edgy/core/db/relationships/relation.py:187">
P2: In this new SkipOperation handler for SingleRelation.add_many, you call send_post_signal() without first setting operation.signal_params['row_count'] = 0 (unlike every other skip handler added in this PR). Since apply_db() never runs on the skip path, the post_relation_add signal for a skipped one-to-many add will be emitted without row_count/row_count_create, whereas all other skipped operations (save_related and the many-to-many add_many) emit row_count=0. Consumers reading row_count on a skipped signal will therefore observe inconsistent behavior depending on the relationship type. Consider setting row_count=0 here for consistency with the rest of the change.</violation>
</file>
<file name="docs_src/signals/excempt_from_deletion.py">
<violation number="1" location="docs_src/signals/excempt_from_deletion.py:41">
P2: The queryset branch of this doc's `pre_delete` handler injects `{"protected": True}`, which (via `queryset.filter(*injected_filters)`) makes the delete target only the protected rows — the exact rows the example is meant to exempt. The per-model branch above it skips deletion of `protected=True` rows, so the two branches contradict each other. The filter should be `injected_filters.append({"protected": False})` (matching the pattern used in `tests/signals/test_deletion_signals_skip.py` with `{"protection": False}`) so that protected records are left untouched.</violation>
</file>
<file name="docs_src/signals/manipulate_bulk.py">
<violation number="1" location="docs_src/signals/manipulate_bulk.py:38">
P2: `create_params.append((item[0].model_copy()))` appends a bare model, not a `(model, position, field_set)` tuple. `create_params` entries are consumed as 3-tuples (`_iterate_create` uses `item[0]`/`item[1]`/`item[2]`, and `instances_and_created[item[1]]`), so the appended bare object will raise (e.g. indexing a pydantic model) when `apply_db` processes it, breaking the documented example. Append a full tuple: `create_params.append((item[0].model_copy(), item[1], item[2]))`.</violation>
</file>
<file name="edgy/core/db/querysets/bulk.py">
<violation number="1" location="edgy/core/db/querysets/bulk.py:361">
P2: Skipped `post_bulk` payloads omit the operation’s row-count field and include `values` even though skipped payloads must not; receivers that require the same count argument they receive for successful bulk calls fail on a skipped operation. Preserve the skipped contract by omitting `values` and supplying zero for each applicable create/update count.</violation>
</file>
<file name="edgy/core/db/querysets/queryset.py">
<violation number="1" location="edgy/core/db/querysets/queryset.py:1347">
P2: A skipped `bulk_create` post signal omits `row_count_create`, so receivers relying on the documented post-bulk parameter fail instead of observing a zero-row skipped operation. Initialize applicable row counts to zero for skipped bulk operations before dispatching post signals; same gap exists in the other new bulk skip branches.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="edgy/core/db/relationships/relation.py">
<violation number="1" location="edgy/core/db/relationships/relation.py:878">
P3: One-to-many `add_many()` returns child model instances and updates existing children; revise this docstring rather than describing intermediate records or IntegrityError `None` results. `None` here denotes only a skipped operation.</violation>
</file>
<file name="edgy/core/db/querysets/parser.py">
<violation number="1" location="edgy/core/db/querysets/parser.py:27">
P3: The new `row_to_model_raw` duplicates the exact `from_sqla_row` construction params that already live in the `cache_fn` lambda of `row_to_model` (and a third copy exists in `batch_to_models`). Any future change to model construction (e.g. a new `from_sqla_row` argument or different `is_defer_fields` logic) must now be kept in sync across three places. Consider extracting the shared `from_sqla_row(...)` call into a single helper (e.g. a `_build_from_sqla_row(row, tables_and_models)` method or a module-level function) that both the cached and raw parsers call, since `row_to_model_raw` is purely that call without the cache/transform wrapper.</violation>
</file>
<file name="edgy/core/db/querysets/types.py">
<violation number="1" location="edgy/core/db/querysets/types.py:545">
P3: Generated API docs still promise non-null bulk results although skipped pre-signals now return `None` placeholders; update `bulk_create` and `bulk_get_or_create` return documentation to describe nullable entries and skipped operations.</violation>
</file>
<file name="edgy/core/db/querysets/base.py">
<violation number="1" location="edgy/core/db/querysets/base.py:684">
P1: `update_or_create` can still reuse a warmed result-cache entry despite passing `bypass_result_cache=True`, so it may update stale cached state instead of freshly loading the matching row. Gate both early cache-return paths on `not bypass_result_cache` so this flag always reaches `QueryExecutor.get_one`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| async def _get_raw(self, **kwargs: Any) -> tuple[BaseModelType, Any]: | ||
| async def _get_raw( | ||
| self, bypass_result_cache: bool = False, /, **kwargs: Any |
There was a problem hiding this comment.
P1: update_or_create can still reuse a warmed result-cache entry despite passing bypass_result_cache=True, so it may update stale cached state instead of freshly loading the matching row. Gate both early cache-return paths on not bypass_result_cache so this flag always reaches QueryExecutor.get_one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At edgy/core/db/querysets/base.py, line 684:
<comment>`update_or_create` can still reuse a warmed result-cache entry despite passing `bypass_result_cache=True`, so it may update stale cached state instead of freshly loading the matching row. Gate both early cache-return paths on `not bypass_result_cache` so this flag always reaches `QueryExecutor.get_one`.</comment>
<file context>
@@ -678,23 +680,28 @@ async def raw_delete(
- async def _get_raw(self, **kwargs: Any) -> tuple[BaseModelType, Any]:
+ async def _get_raw(
+ self, bypass_result_cache: bool = False, /, **kwargs: Any
+ ) -> tuple[EdgyModel, EdgyEmbedTarget]:
"""
</file context>
| list[BaseModelType | None]: A list of saved intermediate model instances, | ||
| or None for each record that already exists | ||
| (IntegrityError) or when operation was skipped. |
There was a problem hiding this comment.
P3: One-to-many add_many() returns child model instances and updates existing children; revise this docstring rather than describing intermediate records or IntegrityError None results. None here denotes only a skipped operation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At edgy/core/db/relationships/relation.py, line 878:
<comment>One-to-many `add_many()` returns child model instances and updates existing children; revise this docstring rather than describing intermediate records or IntegrityError `None` results. `None` here denotes only a skipped operation.</comment>
<file context>
@@ -875,7 +875,9 @@ async def add_many(self, *children: BaseModelType) -> list[BaseModelType | None]
Returns:
- list[BaseModelType | None]: A list of saved child model instances.
+ list[BaseModelType | None]: A list of saved intermediate model instances,
+ or None for each record that already exists
+ (IntegrityError) or when operation was skipped.
</file context>
| list[BaseModelType | None]: A list of saved intermediate model instances, | |
| or None for each record that already exists | |
| (IntegrityError) or when operation was skipped. | |
| list[BaseModelType | None]: A list of saved child model instances, or None for | |
| each child when operation was skipped. |
| self.queryset = queryset | ||
| self.model_class = queryset.model_class | ||
|
|
||
| async def row_to_model_raw( |
There was a problem hiding this comment.
P3: The new row_to_model_raw duplicates the exact from_sqla_row construction params that already live in the cache_fn lambda of row_to_model (and a third copy exists in batch_to_models). Any future change to model construction (e.g. a new from_sqla_row argument or different is_defer_fields logic) must now be kept in sync across three places. Consider extracting the shared from_sqla_row(...) call into a single helper (e.g. a _build_from_sqla_row(row, tables_and_models) method or a module-level function) that both the cached and raw parsers call, since row_to_model_raw is purely that call without the cache/transform wrapper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At edgy/core/db/querysets/parser.py, line 27:
<comment>The new `row_to_model_raw` duplicates the exact `from_sqla_row` construction params that already live in the `cache_fn` lambda of `row_to_model` (and a third copy exists in `batch_to_models`). Any future change to model construction (e.g. a new `from_sqla_row` argument or different `is_defer_fields` logic) must now be kept in sync across three places. Consider extracting the shared `from_sqla_row(...)` call into a single helper (e.g. a `_build_from_sqla_row(row, tables_and_models)` method or a module-level function) that both the cached and raw parsers call, since `row_to_model_raw` is purely that call without the cache/transform wrapper.</comment>
<file context>
@@ -24,6 +24,31 @@ def __init__(self, queryset: BaseQuerySet | Any) -> None:
self.queryset = queryset
self.model_class = queryset.model_class
+ async def row_to_model_raw(
+ self,
+ row: sqlalchemy.Row | Any,
</file context>
| ignore_conflicts: bool = False, | ||
| resolve_embed: Literal[False] = False, | ||
| ) -> list[EdgyModel]: | ||
| ) -> list[EdgyModel | None]: |
There was a problem hiding this comment.
P3: Generated API docs still promise non-null bulk results although skipped pre-signals now return None placeholders; update bulk_create and bulk_get_or_create return documentation to describe nullable entries and skipped operations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At edgy/core/db/querysets/types.py, line 545:
<comment>Generated API docs still promise non-null bulk results although skipped pre-signals now return `None` placeholders; update `bulk_create` and `bulk_get_or_create` return documentation to describe nullable entries and skipped operations.</comment>
<file context>
@@ -549,18 +540,9 @@ async def bulk_create(
+ ignore_conflicts: bool = False,
resolve_embed: Literal[False] = False,
- ) -> list[EdgyModel]:
+ ) -> list[EdgyModel | None]:
"""
Args:
</file context>
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/signals/test_deletion_signals_skip.py">
<violation number="1" location="tests/signals/test_deletion_signals_skip.py:102">
P2: Teardown typo: `Unrelated.meta.signals.post_delete.disconnect(pre_deleting)` should disconnect `post_deleting`, not `pre_deleting`. As written the `post_deleting` receiver stays registered on `Unrelated.post_delete` after the autouse fixture tears down, leaking it across the test session and causing stale signal handling on any later Unrelated deletion.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| Unrelated.meta.signals.pre_delete.disconnect(pre_deleting) | ||
| Profile.meta.signals.pre_delete.disconnect(pre_deleting) | ||
| User.meta.signals.pre_delete.disconnect(pre_deleting) | ||
| Unrelated.meta.signals.post_delete.disconnect(pre_deleting) |
There was a problem hiding this comment.
P2: Teardown typo: Unrelated.meta.signals.post_delete.disconnect(pre_deleting) should disconnect post_deleting, not pre_deleting. As written the post_deleting receiver stays registered on Unrelated.post_delete after the autouse fixture tears down, leaking it across the test session and causing stale signal handling on any later Unrelated deletion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/signals/test_deletion_signals_skip.py, line 102:
<comment>Teardown typo: `Unrelated.meta.signals.post_delete.disconnect(pre_deleting)` should disconnect `post_deleting`, not `pre_deleting`. As written the `post_deleting` receiver stays registered on `Unrelated.post_delete` after the autouse fixture tears down, leaking it across the test session and causing stale signal handling on any later Unrelated deletion.</comment>
<file context>
@@ -90,8 +96,10 @@ async def post_deleting(sender, **kwargs):
+ Unrelated.meta.signals.pre_delete.disconnect(pre_deleting)
Profile.meta.signals.pre_delete.disconnect(pre_deleting)
User.meta.signals.pre_delete.disconnect(pre_deleting)
+ Unrelated.meta.signals.post_delete.disconnect(pre_deleting)
Profile.meta.signals.post_delete.disconnect(post_deleting)
User.meta.signals.post_delete.disconnect(post_deleting)
</file context>
| Unrelated.meta.signals.post_delete.disconnect(pre_deleting) | |
| Unrelated.meta.signals.post_delete.disconnect(post_deleting) |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="edgy/core/db/querysets/queryset.py">
<violation number="1" location="edgy/core/db/querysets/queryset.py:1245">
P2: Reiterating this queryset after `update_or_create(defaults={})` finds one existing row will read a model/embed target as `result[1]`, causing a subscript error or wrong result. Cache the `(raw_instance, resolved)` pair, matching the executor and `create()` cache format.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| if not args and not kwargs: | ||
| self._cache.update( | ||
| self.model_class, | ||
| values=[resolved], |
There was a problem hiding this comment.
P2: Reiterating this queryset after update_or_create(defaults={}) finds one existing row will read a model/embed target as result[1], causing a subscript error or wrong result. Cache the (raw_instance, resolved) pair, matching the executor and create() cache format.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At edgy/core/db/querysets/queryset.py, line 1245:
<comment>Reiterating this queryset after `update_or_create(defaults={})` finds one existing row will read a model/embed target as `result[1]`, causing a subscript error or wrong result. Cache the `(raw_instance, resolved)` pair, matching the executor and `create()` cache format.</comment>
<file context>
@@ -1236,9 +1236,18 @@ async def update_or_create(
+ if not args and not kwargs:
+ self._cache.update(
+ self.model_class,
+ values=[resolved],
+ cache_keys=[self._cache.create_cache_key(self.model_class, raw_instance)],
+ )
</file context>
| values=[resolved], | |
| values=[(raw_instance, resolved)], |
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="edgy/core/db/querysets/queryset.py">
<violation number="1" location="edgy/core/db/querysets/queryset.py:1240">
P1: Fetched filtered querysets can return rows no longer matching their filter/order after this update because the result cache is retained and only the row value is replaced. Clear result cache here; cached select compilation can still be retained.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| self._clear_cache() | ||
| return cast(EdgyEmbedTarget, get_instance), False | ||
| # we can keep the result cache because we update it | ||
| self._clear_cache(keep_cached_selected=True, keep_result_cache=True) |
There was a problem hiding this comment.
P1: Fetched filtered querysets can return rows no longer matching their filter/order after this update because the result cache is retained and only the row value is replaced. Clear result cache here; cached select compilation can still be retained.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At edgy/core/db/querysets/queryset.py, line 1240:
<comment>Fetched filtered querysets can return rows no longer matching their filter/order after this update because the result cache is retained and only the row value is replaced. Clear result cache here; cached select compilation can still be retained.</comment>
<file context>
@@ -1236,15 +1236,18 @@ async def update_or_create(
await relation.add(model)
- self._clear_cache(keep_cached_selected=True)
+ # we can keep the result cache because we update it
+ self._clear_cache(keep_cached_selected=True, keep_result_cache=True)
# now resolve again
resolved = (await self._embed_parent_in_result(raw_instance))[1]
</file context>
| self._clear_cache(keep_cached_selected=True, keep_result_cache=True) | |
| self._clear_cache(keep_cached_selected=True) |
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="edgy/core/db/querysets/executor.py">
<violation number="1" location="edgy/core/db/querysets/executor.py:351">
P2: A queryset can keep returning a deleted model after a model-based delete affects zero rows. When a cached model is already marked `_db_deleted`, `raw_delete()` returns 0 and this condition preserves the stale result cache; clearing the cache for model-based deletes (or distinguishing skipped operations from no-op deletes) avoids serving deleted objects.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| # clear cache after deletion. | ||
| self.queryset._clear_cache(keep_cached_selected=True) | ||
| if row_count != 0: |
There was a problem hiding this comment.
P2: A queryset can keep returning a deleted model after a model-based delete affects zero rows. When a cached model is already marked _db_deleted, raw_delete() returns 0 and this condition preserves the stale result cache; clearing the cache for model-based deletes (or distinguishing skipped operations from no-op deletes) avoids serving deleted objects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At edgy/core/db/querysets/executor.py, line 351:
<comment>A queryset can keep returning a deleted model after a model-based delete affects zero rows. When a cached model is already marked `_db_deleted`, `raw_delete()` returns 0 and this condition preserves the stale result cache; clearing the cache for model-based deletes (or distinguishing skipped operations from no-op deletes) avoids serving deleted objects.</comment>
<file context>
@@ -344,7 +348,8 @@ async def delete(
# clear cache after deletion.
- self.queryset._clear_cache(keep_cached_selected=True)
+ if row_count != 0:
+ self.queryset._clear_cache(keep_cached_selected=True)
return row_count
</file context>
Changes:
Note however, using SkipOperation causes a logic change in bulk and relation operations: the result is an empty array.