kvesteri / sqlalchemy-continuum

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

Add caching for properties in VersionClassBase #292

Open AbdealiLoKo opened 2 years ago

AbdealiLoKo commented 2 years ago

Resolves https://github.com/kvesteri/sqlalchemy-continuum/issues/290

Improve performance in properties of VersionClassBase So that they don't do extra sql queries and we can reuse the existing values

marksteward commented 2 years ago

I'm really not convinced this is suitable. What makes it hard for you to cache the property yourself?

AbdealiLoKo commented 2 years ago

Just a better developer experience.

As a user - I was surprised to see that this is generating a query every time. And I felt like developers using sqla-continuum would not realize this until they enable sqlalchemy echo=True (For example - When I use sqlalchemy - I expect that it caches the relationships.) In fact, I thought that this was just a relationship until I realized checked that it was not!

If you think it's not worth it to add to the library I am fine with it - we can close the issue and PR. I do have a workaround I was able to create:

class VersionClassCustom:
    @cached_property
    def previous(self):
        return super().previous

    @cached_property
    def next(self):
        return super().next

    @cached_property
    def index(self):
        return super().index

from sqlalchemy_continuum import make_versioned
from sqlalchemy_utils.functions import get_declarative_base
from myapp.models import MyModel

make_versioned(
    options={
        'base_classes': (VersionClassCustomBase, get_declarative_base(MyModel)),
    },
)
marksteward commented 2 years ago

That's a good point. However, it's a potentially breaking change so would need to be in a major release.

I guess I'm surprised that you need to call .next or .previous multiple times for a version. What sort of use case is it? A template?

AbdealiLoKo commented 2 years ago

We have a stringification logic which takes a version and tries to explain in english what changes have occured (So its kinda like a template - yes) We have multiple functions - handling columns, manytomany/onetomany/etc. relationships, association-proxies, column_properties To each of these functions we pass the version and use version.previous to get the previous version

I don't mind it being in a major release cause i do have a workaround which is elegant. Should I be doing something to mark this as a breaking change ? Or just leave the PR as is ?

marksteward commented 2 years ago

I haven't given much thought to a major release, as I'm focusing on maintenance for now and this is the first thing that would be in it. It would be good to get tests added specifically for this feature.

I'd then be happy to approve it and see if anyone else has feedback on the issue or PR. Then maybe merge it in a few weeks? If you have any other improvements (e.g. #286) maybe we can get them all into one release.

AbdealiLoKo commented 2 years ago

Sure - sounds good

marksteward commented 2 years ago

Can you rebase this? Thanks!

AbdealiLoKo commented 2 years ago

Rebased and found a minor issue - fixed it. Tests pass \^_^/