Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make VersionClass.versions a plain list #332

Open
marksteward opened this issue May 5, 2023 · 3 comments
Open

Make VersionClass.versions a plain list #332

marksteward opened this issue May 5, 2023 · 3 comments

Comments

@marksteward
Copy link
Collaborator

marksteward commented May 5, 2023

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?

@marksteward marksteward changed the title Make versions a plain list Make VersionClass.versions a plain list May 5, 2023
@alliefitter
Copy link

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
Copy link
Collaborator Author

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
Copy link
Contributor

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

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.

TomGoBravo added a commit to TomGoBravo/tourist-with-flask that referenced this issue Aug 16, 2023
Read all transactions into memory to display version history. This is a bit ugly, not tested very well but speeds up generating the render cache from about a minute to a few seconds. It is a work-around for kvesteri/sqlalchemy-continuum#332 .
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

No branches or pull requests

3 participants