Auditing/versioning v2 by GianlucaFicarelli · Pull Request #510 · openbraininstitute/entitycore

GianlucaFicarelli · 2026-01-16T12:55:31Z

This PR adds auditing/versioning using an external library sqlalchemy-history (fork of sqlalchemy-continuum).
Only sqlalchemy-continuum would support native_versioning (faster, using potgresql triggers), but the defined triggers have some limitations with polymorphism (they don't create a version of all the tables, and they don't set transaction_end on the previous version), so they would require some changes.

To consider all the deletions through the sqlalchemy models, the ON DELETE CASCADE defined in the db schema have been removed and replaced with a corresponding relationship with cascade.
This is less efficient, especially when deleting entities with many children, but the deletion is not the usual case so it could be acceptable. We can also reconsider the use of triggers later.

The tests are a lot slower because we have twice the number of tables, and we truncate them after each test. Using rollback instead of truncate could improve the performance, but it requires some changes in the instantiation of the sessions during tests.
Alternatively, we can disable versioning during tests that don't check versioning, and truncate only the non-version tables.

* origin/main: Add entity_type filter to measurement_label (#496) Add StructuralDomain.not_applicable (#495) Increase statement_timeout and lock_timeout in db migration (#494) Use scope=function in get_db dependency (#492)

* origin/main: Update `brain-atlas` endpoints (#506) Add published_in filter in ScientificArtifact (#505) Make nullable attributes of EMDenseReconstructionDataset (#503) Remove all endpoints that had a `InBrainRegionDep`, but don't have a brain_region_id (#501) Add pref_label/alt_label ilike_search to ETypeClass/MTypeClass filters (#500) add `number_neurons` to Simulation` (#493) Fix incorrect returned values when getting `ascendants_and_descendants` (#498)

* origin/main: Hotfix migration (#508) Add published_in__ilike filter for circuit (#509)

codecov · 2026-01-16T14:27:34Z

Codecov Report

❌ Patch coverage is 93.91892% with 18 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
app/service/versioning.py	92.92%	4 Missing and 4 partials ⚠️
app/db/triggers.py	61.53%	5 Missing ⚠️
app/service/cell_morphology.py	44.44%	5 Missing ⚠️

Flag	Coverage Δ
pytest	`97.46% <93.91%> (-0.18%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
app/db/model.py	`99.19% <100.00%> (+0.10%)`	⬆️
app/db/utils.py	`83.78% <100.00%> (ø)`
app/db/versioning_plugins/user_context.py	`100.00% <100.00%> (ø)`
app/dependencies/db.py	`100.00% <100.00%> (ø)`
app/routers/__init__.py	`100.00% <ø> (ø)`
app/routers/cell_morphology.py	`100.00% <100.00%> (ø)`
app/routers/versioning.py	`100.00% <100.00%> (ø)`
app/utils/routers.py	`80.00% <100.00%> (+2.72%)`	⬆️
app/db/triggers.py	`81.25% <61.53%> (-18.75%)`	⬇️
app/service/cell_morphology.py	`91.22% <44.44%> (-8.78%)`	⬇️
... and 1 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

GianlucaFicarelli · 2026-01-16T14:29:11Z

+read_mtypes_history = router.get("/{id_}/mtypes/history")(
+    app.service.cell_morphology.read_mtypes_history
+)


This is just an example, it's going to be removed.

GianlucaFicarelli · 2026-01-16T14:29:32Z

    )
+
+
+def read_mtypes_history(


mgeplf

No red flags, sqlalchemy-history seems to not be too intrusive considering what it unlocks; very impressive.

mgeplf · 2026-02-11T08:05:53Z

+            # FIXME: # noqa: FIX001
+            # the creation of a new agent would fail with: Session is already flushing
+            # so the user_id is logged only if it already exists.
+            # Alternative: ensure that the person is created at the beginning of the request,


is the Alternative a note to yourself and should be removed? If not, I don't follow it.

mgeplf · 2026-02-11T08:09:41Z

 required-version = ">=0.8.0"

+[tool.uv.sources]
+sqlalchemy-history = { git = "https://github.com/GianlucaFicarelli/sqlalchemy-history", branch = "obi" }


should this be forked into OBI?

mgeplf · 2026-02-11T08:11:43Z

+        TransactionChangesPlugin(),
+    ],
+    options={
+        "enum_prefix": "",


can you add a comment what this enum_prefix is for?

mgeplf · 2026-02-11T08:20:25Z

        uselist=True,
-        passive_deletes=True,  # rely on PostgreSQL ON DELETE CASCADE to remove association rows
-    )
+    )  # deletion cascade of generation


I don't know enough about the deletion, and cascade behavior that we want to be able to evaluate the correctness of this - I will leave that to @eleftherioszisis.

mgeplf · 2026-02-11T08:22:38Z

    if issubclass(mapper.class_, MeasurableEntityMixin)
    and issubclass(mapper.class_, Entity)
-    and mapper.class_.__tablename__
+    and getattr(mapper.class_, "__tablename__", None)


is this to skip the versioned table? Or something else?

mgeplf · 2026-02-11T08:26:12Z

+    resource_route: ResourceRoute,
+    resource_id: uuid.UUID,
+) -> int:
+    """Return the number fo versions."""


Suggested change

"""Return the number fo versions."""

"""Return the number of versions."""

mgeplf · 2026-02-11T08:27:28Z

+        base_query = constrain_to_accessible_entities(
+            base_query, project_id=user_context.project_id, db_model_class=id_model_class
+        )
+    if version_num < 0:


why would it be negative?

mgeplf · 2026-02-11T08:31:10Z

    libpq-dev \
-    ca-certificates
+    ca-certificates \
+    git


this is for convenience, right? not something to do w/ versioning?

eleftherioszisis · 2026-02-18T10:02:18Z

    EntityType[mapper.class_.__tablename__]: mapper.class_
    for mapper in Base.registry.mappers
-    if hasattr(EntityType, mapper.class_.__tablename__)
+    if getattr(mapper.class_, "__tablename__", None) in EntityType


Was this change a result of some corner case? It would be nice to have a comment of the reason if this is the case.

eleftherioszisis · 2026-02-18T10:05:32Z


    # If polymorphic_identity is different from __tablename__, it's a subclass
-    return mapper_args["polymorphic_identity"] != mapper_class.__tablename__
+    return mapper_args["polymorphic_identity"] != getattr(mapper_class, "__tablename__", None)


Would it worth to create little helper funcs with docstrings to make these more readable?

eleftherioszisis · 2026-02-18T10:13:31Z

 # are executed before the response is sent back to the client.
-SessionDep = Annotated[Session, Depends(get_db, scope="function")]
+PlainSessionDep = Annotated[Session, Depends(get_db, scope="function")]
+SessionDep = Annotated[Session, Depends(get_db_with_user_context, scope="function")]


Maybe UserSessionDep?

GianlucaFicarelli added 21 commits December 23, 2025 16:31

WIP Add auditing

d55a031

Update triggers

b098b34

Update migration, add api example

e2a4337

Update migrations and dependencies after rebase

355f464

Minor, change hasattr to getattr

5e31c85

Fix lint

456371a

Fix tests

42611b4

Add git to docker image

3e4c6ac

Populate versioned tables

c36257a

Merge remote-tracking branch 'origin/main' into audit

ce6a2ee

* origin/main: Add entity_type filter to measurement_label (#496) Add StructuralDomain.not_applicable (#495) Increase statement_timeout and lock_timeout in db migration (#494) Use scope=function in get_db dependency (#492)

Update migrations after merge

7c6b43c

Delete migrations, update model

6061da9

Remove code for generating triggers for versioning

72f56ef

Remove postgresql deletion cascade, use sqlalchemy

057dfdc

Add versioning tables

618d37b

Replace sqlalchemy-continuum with sqlalchemy-history

5968646

Remove _ from relationships, fix user_context plugin

d8b9c47

Add versioning endpoint

51fe55a

Fix lint

ec1da27

Merge remote-tracking branch 'origin/main' into audit_v2

3fd6079

* origin/main: Hotfix migration (#508) Add published_in__ilike filter for circuit (#509)

GianlucaFicarelli self-assigned this Jan 16, 2026

Update uv.lock

4dc746e

GianlucaFicarelli mentioned this pull request Jan 16, 2026

Auditing/Versioning #485

Closed

GianlucaFicarelli force-pushed the audit_v2 branch from c94f464 to 551232b Compare January 16, 2026 14:07

Fix test_routes

11b4e29

GianlucaFicarelli force-pushed the audit_v2 branch from 551232b to 11b4e29 Compare January 16, 2026 14:14

GianlucaFicarelli commented Jan 16, 2026

View reviewed changes

Comment thread app/service/cell_morphology.py

)

def read_mtypes_history(

Copy link
Copy Markdown

Collaborator Author

GianlucaFicarelli Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

GianlucaFicarelli requested a review from eleftherioszisis January 16, 2026 14:37

GianlucaFicarelli requested a review from mgeplf February 2, 2026 10:12

mgeplf reviewed Feb 11, 2026

View reviewed changes

eleftherioszisis reviewed Feb 18, 2026

View reviewed changes

	"""Return the number fo versions."""
	"""Return the number of versions."""

Conversation

GianlucaFicarelli commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 16, 2026

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgeplf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GianlucaFicarelli commented Jan 16, 2026 •

edited

Loading