litestar-org / advanced-alchemy

A carefully crafted, thoroughly tested, optimized companion library for SQLAlchemy
http://docs.advanced-alchemy.litestar.dev/
MIT License
246 stars 30 forks source link

Enhancement: `BigIntPrimaryKey` is strongly opinionated and reduced to Table Sequence id generator, add other generator strategy #253

Closed cbdiesse closed 3 weeks ago

cbdiesse commented 2 months ago

Summary

I want for example, be able to define my User entity with BigIntPrimaryKey as follow :

class User(BigIntAuditBase):
    __tablename__ = "user_account"
    ...

but if failed with error : NameError: name 'BigInteger' is not defined

After checking, I realize that the related parent classes (BigIntAuditBase --> BigIntPrimaryKey) where defined to restrict the primary key to be a table sequence generator. Don't see another option, as displayed in the following code :

class BigIntPrimaryKey:
    """BigInt Primary Key Field Mixin."""

    # noinspection PyMethodParameters
    @declared_attr
    def id(cls) -> Mapped[int]:
        """BigInt Primary key column."""
        return mapped_column(
            BigIntIdentity,
            Sequence(f"{cls.__tablename__}_id_seq", optional=False),  # type: ignore[attr-defined]
            primary_key=True,
        )

This is a strongly opinionated decision, which don't take in account the basic requirements of model design. I remember that previous version was defined like the actual UUIDPrimaryKey :

class UUIDPrimaryKey:
    """UUID Primary Key Field Mixin."""

    id: Mapped[UUID] = mapped_column(default=uuid4, primary_key=True)
    """UUID Primary key column."""

    @declared_attr
    def _sentinel(cls) -> Mapped[int]:
        return orm_insert_sentinel(name="sa_orm_sentinel")

So, is there a way to go around the actual id method and simply define the id column and use other mind to set the key value (using default value = client_func() / server_func() / stored procedure() ?

The actual UUID type could have been a good choice if it was Long (8 bytes max) size and of int type. We have Generator for unique TimeStamped BigInteger, with all the capacity of (UUID) but with comfortable size, maintenance capacity, distributed system, conflict resolution, no duplicate, etc ... And it feats to medium business and also sectorial activities inside Big companies.

So, Want to put in work our configurable (GUTID : Global Unique TimeStamped Identifier) BigInteger generator :

But to be able to do all that, there is a need for an inception for an open way to define and implement primary key values.

I am not very fluent in python (newbie from the Java side) Thanks for your help.

Basic Example

I am not actually very fleunt in python, but the big idea is to open a gate for many types of Integer value generation form client side or server side.

Seeing how decoration can be used to augment/override the functions/method of a class, it would be must favorable to go for a PrimKey Generator Strategy as with DB predefined generators, DB servers functions/stored procedures, or client/server custom generators functions.

Many examples out there (Java/JPA) could be reused.

Drawbacks and Impact

Actually, many developers for medium size application are not very open to use UUID as key, because its cost in size and sometime inefficiency in the many aspects:

Unresolved questions

My request have been resolved in other environment like Java/JPA and it is a fast go for some one fluent in python to put it in work in the advanced alchemy library. I am open to contribute for it if a minimal coaching is there to show where to find good examples and documentations.

cofin commented 1 month ago

@cbdiesse, it's implemented this way to specifically provide compatibility with all our supported backends. Not all of the backends support autoincrement or identity fields, so the sequence route provided the most support.

If you want to implement a custom UUID or BigInt based backend, for instance, use DefaultBase instead of UUIDBase or BigIntBase. Going this route, you should follow the standard SQLAlchemy approach for setting a server_default or whatever requirements you have for your column.

I should also point out that we already have UUIDv7 and v6 support through UUIDv6Base and UUIDv7Base. This solves some of the issues you mention; especially on the databases that require UUID based primary keys such as CockroachDB and Spanner.

cofin commented 1 month ago

Also, I should have mentioned that if you think there's a more suitable default BigInt implementation that works on all backends, feel free to open a PR. I'm more than happy to discuss these changes.

cbdiesse commented 1 month ago

Thanks for your kind reply ...
I am actually stuck with the migration script error: File "<string>", line 1, in <module> NameError: name 'BigInteger' is not defined make: *** [migrations] Error 1 I cannot determine if it is due to my makefile environment variables or the fact that BigIntIdentity with variant makes the type hinting incorrectly resolve to BigInteger.

Sure, I will try to look how to revert to DefaultBase if I cannot solve the problem with the migration and lint scripts. Thanks.

cofin commented 3 weeks ago

Feel free to re-open the issue if there's more to add. I think for now we will leave it as is because it offers the maximum compatibility.