Generalize derivation filters and properties from Circuit to all Entities#647
Generalize derivation filters and properties from Circuit to all Entities#647pgetta wants to merge 5 commits into
Conversation
Expose circuit derivations on GET /circuit, derived from the existing
Derivation table (no migration, no denormalized column).
- Filter (always available), both directions mirroring the read fields:
generated_derivation__derivation_type (circuit is the generated/derived
entity) and used_derivation__derivation_type (circuit is the used/source
entity), each with an __in variant. No source-type restriction, so e.g.
an emodel->circuit emodel_circuit derivation matches on the generated side.
- Read columns (opt-in via ?expand=...): generated_derivations and
used_derivations, expandable independently. Unexpanded directions are
omitted; expanded but empty serialize as []. Load-aware properties keep
this safe under raiseload("*") via two viewonly relationships on Circuit.
The paginated id subquery in `_with_subquery` selected ids without DISTINCT, so a one-to-many filter join (e.g. the circuit derivation-type filters, where a circuit can match several Derivation rows) duplicated the row. OFFSET/LIMIT then paged over the duplicated rows while total_items used count(distinct id), so a matching entity could repeat across pages or be skipped. Select DISTINCT ids in the subquery so the limit window operates on distinct entities. This also hardens the pre-existing contribution/mtype/used/generated one-to-many filters. DISTINCT is valid here because every ORDER BY element is already part of the subquery select list. Add a regression test asserting a circuit matched by multiple derivation rows is counted once and does not reappear on a second page.
Lift the Derivation-backed filter and expand-read capabilities from Circuit up to the Entity base so every entity subclass inherits them, and add a new entity-id filter alongside derivation_type. Filter (centralized, no per-service edits): - Move NestedDerivationFilter to EntityFilterMixin, extended with used_id / generated_id (+ __in) to filter by the related entity, not only by derivation_type. - factory.py auto-injects the derivation joins and distinct aliases for any Entity model; joins apply only when the filter is set, so default lists pay nothing. Expand-read (centralized core + per-service param): - Move the viewonly derivations relationships and load-aware properties from Circuit to Entity. - GeneratedDerivationRead / UsedDerivationRead + DerivationReadMixin on both EntityRead and EntityReadWoutAssets; always present, null until expanded. - Shared EntityExpand / ExpandDep / apply_derivation_expand applied in router_read_one and router_read_many. - Wire expand into read_one, admin_read_one, read_many, admin_read_many across all entity services; cell_morphology / em_cell_mesh keep their measurement_annotation expand, unified with the derivation expands. No DB migration: the derivation table already exists and the new relationships are viewonly.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| ) | ||
| if apply_operations: | ||
| query = apply_operations(query) | ||
| query = apply_derivation_expand(query, db_model_class, expand) |
There was a problem hiding this comment.
Shouldn't this be applied only to entities?
There was a problem hiding this comment.
It already is - apply_derivation_expand early-returns unless issubclass(db_model_class, Entity) (and when expand is empty), so it's a no-op for non-entity models.
| try: | ||
| attr = attrgetter(name)(self) | ||
| except AttributeError: | ||
| # A join may be registered (e.g. the derivation joins, added for every entity model) | ||
| # without this filter declaring the matching field; treat it as not filtering. | ||
| return False |
There was a problem hiding this comment.
I am not sure I understand why is this needed. Isn't it possible to avoid silently skipping errors? If nevertheless is essential could you cover it in tests so that it is clear what is the expected behavior of this bit?
There was a problem hiding this comment.
Good catch - these were effectively unreachable, I've removed the try/except block.
| try: | ||
| attr = attrgetter(name)(self) | ||
| except AttributeError: | ||
| return None |
There was a problem hiding this comment.
Should be fixed as well.
|
|
||
| def _is_entity_model(db_model_class: Any) -> bool: | ||
| """Whether the model is an Entity subclass (kept separate to avoid narrowing the caller).""" | ||
| return isinstance(db_model_class, type) and issubclass(db_model_class, Entity) |
There was a problem hiding this comment.
Is it indeed the case that the type needs to be as broad as "Any"?
There was a problem hiding this comment.
Nice, I've narrowed the param to object.
| derivation_type: DerivationType | None = None | ||
| derivation_type__in: list[DerivationType] | None = None | ||
| used_id: uuid.UUID | None = None | ||
| used_id__in: list[uuid.UUID] | None = None |
There was a problem hiding this comment.
Nice, this can be useful for selecting cell morphologies derived from one or more EMDenseReconstructionDataset with derivation_type em_dense_reconstruction_dataset_cell_morphology and support pagination.
See this comment
| ) | ||
|
|
||
| @property | ||
| def generated_derivations(self) -> list["Derivation"] | None: |
There was a problem hiding this comment.
Thinking about naming, what about calling the properties:
generated_from_derivations and used_by_derivations?
Not much longer, but it looks slightly clearer to me.
Other ideas?
There was a problem hiding this comment.
Agreed, clearer. Renamed: generated_derivations -> generated_from_derivations and used_derivations -> used_by_derivations (properties, response fields, and the ?expand= values).
|
|
||
| # generated_derivation / used_derivation are inherited from EntityFilterMixin via | ||
| # ScientificArtifactFilter, so every entity filter exposes them. | ||
|
|
There was a problem hiding this comment.
This comment isn't very useful since it's the same for every entity, so it can be removed,
| @property | ||
| def generated_derivations(self) -> list["Derivation"] | None: | ||
| """Derivations where this entity is the generated entity, or None if not expanded.""" | ||
| if "derivations_as_generated" in sa.inspect(self).unloaded: |
There was a problem hiding this comment.
Do we need that guard?
If the property is accessed without loading the derivations, it could be better to fail rather than returning None.
Removing the guard can be acceptable only if we are sure that the property is accessed only when required.
There was a problem hiding this comment.
I checked better the code and these properties are accessed even when expand is empty, so the guard is needed.
| "generated_derivation": aliased(Derivation, flat=True), | ||
| "used_derivation": aliased(Derivation, flat=True), | ||
| } | ||
|
|
There was a problem hiding this comment.
Mutating the aliases dict seems a bit fragile, and unexpected by the caller.
An alternative could be to provide a function to build the aliases, that is called in each read_many instead of initializing the aliases dict directly.
However, it requires more changes and it would be easier to review if it's done in a separate PR (that can be also done after this one, I can have a look as well).
There was a problem hiding this comment.
Thank you, Gianluca, I'll note this down - can look a bit later (in case you don't address it in the meantime).
| user_context: UserContextDep, | ||
| db: SessionDep, | ||
| id_: uuid.UUID, | ||
| expand: ExpandDep = None, |
There was a problem hiding this comment.
It's a bit annoying to add expand to each endpoint, but I don't see a cleaner way to avoid those repetitions 🤔
- Rename expand keys/properties: generated_derivations -> generated_from_derivations and used_derivations -> used_by_derivations (params, response fields, properties) - Remove unreachable try/except in CustomFilter nested-field helpers so a genuine registered-join/undeclared-field mismatch fails loudly - Drop redundant inherited-derivation comment in CircuitFilter - Narrow _is_entity_model param from Any to object
3 participants
Alternative implementation of #642.
Where #642 adds the derivation-backed filters and expand-read properties to Circuit only, this PR lifts them up to the
Entitybase so every entity subclass inherits them. It also adds an entity-id filter (used_id/generated_id, +__in) alongsidederivation_type.EntityFilterMixin+factory.py(joins auto-injected per entity model, applied only when the filter is set), so no per-service filter wiring.generated_derivations/used_derivationslive onEntityRead/EntityReadWoutAssets, always present andnulluntil?expand=is used;expandis wired intoread_one,admin_read_one,read_many,admin_read_manyacross all entity services.derivationtable already exists and the new relationships areviewonly.