search

kvesteri / sqlalchemy-continuum

Versioning extension for SQLAlchemy.
BSD 3-Clause "New" or "Revised" License
568 stars 128 forks source link

Make VersionClass.versions a plain list #332

Open marksteward opened 1 year ago

marksteward commented 1 year ago

This (VersionClass.versions) uses a lazy='dynamic' relationship, which is on its way out:

first_version = article.versions[0]

This also supports negative indexes, which a lot of the tests use. They were never performant and are removed in SQLAlchemy 2.0. While we could create an accessor that uses order_by to do the right thing, it's probably less surprising to just make .versions return a list instead of using lazy='dynamic'.

We could expose a separate helper to get particular versions by index or version count. Do people think this is useful?

alliefitter commented 1 year ago

I'm not very fluent in SqlAlchemy. Would this require reading all versions when a model is read? Also, the PR I put up removed negative indexing in the tests in favor of .all()[-1], so if tests are a major consideration in this decision, that's already solved. Btw, I just pushed a commit to bump Flask-SqlAlchemy to the version I had installed locally, so I believe, at least for sqlite, all tests should pass except for savepoints.

marksteward commented 1 year ago

The previous code would read all versions anyway, but without that being obvious. I'm wondering whether we go the whole way and just always return all versions.

TomGoBravo commented 11 months ago

For my use (iterating through all versions of a small database with about 2-3 versions per object) returning all versions will be significantly faster, but for other access patterns (such as getting only previous version of object with large number of versions) it could be significantly slower. @marksteward 's suggestion of adding helper to get particular versions by index or version count seems like a good way to support both access patterns. I'm not that familiar with sqlalchemy internals but tried changing https://github.com/kvesteri/sqlalchemy-continuum/blob/a7a6bd7952185b1f82af985c0271834d886a617c/sqlalchemy_continuum/model_builder.py#L135 from dynamic to selectin (found at https://docs.sqlalchemy.org/en/14/orm/loading_relationships.html#selectin-eager-loading ) but it didn't seem to help the performance of my code.