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

Support SQLAlchemy 2's DeclarativeBase and MappedAsDataclass #1140

Closed
tadams42 opened this issue Nov 21, 2022 · 20 comments · Fixed by #1215
Closed

Support SQLAlchemy 2's DeclarativeBase and MappedAsDataclass #1140

tadams42 opened this issue Nov 21, 2022 · 20 comments · Fixed by #1215

Comments

@tadams42
Copy link

tadams42 commented Nov 21, 2022

I've been playing with mapped_column and MappedAsDataclass (new stuff in SQLAlchemy 2.x):

Declarative Table with mapped_column()
Declarative Dataclass Mapping

Example:

mkdir sqlaplayground
cd sqlaplayground
pyenv local 3.11 
python -m venv .venv 
source .venv/bin/activate
pip install -U pip wheel setuptools
cat requirements.txt
# sqlalchemy >= 2.0.0b3
# flask-sqlalchemy @ git+https://github.com/pallets-eco/flask-sqlalchemy@3.0.x
# flask >= 2.0.0
pip install -r requirements.txt
from __future__ import annotations

from typing import Optional

from sqlalchemy import ForeignKey, String, create_engine, orm
from sqlalchemy.orm import (
    DeclarativeBase, Mapped, MappedAsDataclass, mapped_column, sessionmaker
)

CONFIG = {
    "SQLALCHEMY_DATABASE_URI": "sqlite+pysqlite://",
}

Engine = create_engine(CONFIG["SQLALCHEMY_DATABASE_URI"], future=True)
Session = sessionmaker(bind=Engine, future=True)
session = Session()


class BaseModel(MappedAsDataclass, DeclarativeBase):
    """subclasses will be converted to dataclasses"""

    pass


class Book(BaseModel):
    __tablename__ = "books"

    id: Mapped[Optional[int]] = mapped_column(
        init=False, primary_key=True, autoincrement=True
    )

    title: Mapped[Optional[str]] = mapped_column(String(length=64), default=None)

    author_id: Mapped[Optional[int]] = mapped_column(
        ForeignKey("authors.id"), nullable=False, index=True, default=None
    )
    author: Mapped[Optional[Author]] = orm.relationship(
        "Author", uselist=False, back_populates="books", default=None
    )


class Author(BaseModel):
    __tablename__ = "authors"

    id: Mapped[Optional[int]] = mapped_column(
        init=False, primary_key=True, autoincrement=True
    )
    name: Mapped[Optional[str]] = mapped_column(String(length=64), default=None)
    books: Mapped[list[Book]] = orm.relationship(
        "Book", uselist=True, back_populates="author", default_factory=list
    )

BaseModel.metadata.create_all(Engine)

book = Book(title="42", author=Author(name="Name"))
session.add(book)
session.commit()

This rather verbose declaration of models gives us some nice things:

  • more precise mypy and PyRight static typechecking.
  • dataclass-like __init__
repr(book)
# "Book(id=1, title='42', author_id=1, author=Author(id=1, name='Name', books=[...]))"
?book.__init__
# book.__init__(title=None, author_id=None, author=None)

First try at using my BaseModel with Flask-SQLAlchemy gets me into trouble with metaclass inheritance:

import flask
import flask_sqlalchemy


app = flask.Flask(__name__)
app.config.from_mapping(CONFIG)

db = flask_sqlalchemy.SQLAlchemy(model_class=BaseModel)

# TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) 
# subclass of the metaclasses of all its bases

which is fair enough...

I did try few things from Advanced Customization but so far came empty handed.

Can Flask-SQLAlchemy support this "new" declarative models?
Should it?
Maybe avoid this problem by somehow replacing metaclass magic with __init_subclass__ from
PEP 487 - Simpler customisation of class creation

@davidism
Copy link
Member

davidism commented Nov 21, 2022

I looked into both DeclarativeBase and __init_subclass__ while writing Flask-SQLAlchemy 3, but concluded that it was not clear how to support DeclarativeBase or __init_subclass__ given the features we provide. I'd be happy for someone to show me how we could do both of those things.

Flask-SQLAlchemy allows passing either a plain class or a declarative base class as model_class. It creates a declarative base from the plain class if needed. It does that by looking at isinstance(model_class, DeclarativeMeta).

if not isinstance(model, sa.orm.DeclarativeMeta):
metadata = self._make_metadata(None)
model = sa.orm.declarative_base(
metadata=metadata, cls=model, name="Model", metaclass=DefaultMeta
)

Since you're using SQLAlchemy 2's new DeclarativeBase, you're not getting DeclarativeMeta as a base class, leading to that error since Flask-SQLAlchemy tries to make a new declarative base.

Our code for setting table names and supporting joined-table inheritance requires running before and after DeclarativeMeta.__init__. __init_subclass__ runs strictly before the metaclass, I couldn't figure out a way to support what we need.

if should_set_tablename(cls):
cls.__tablename__ = camel_to_snake_case(cls.__name__)
super().__init__(name, bases, d, **kwargs)
# __table_cls__ has run. If no table was created, use the parent table.
if (
"__tablename__" not in cls.__dict__
and "__table__" in cls.__dict__
and cls.__dict__["__table__"] is None
):
del cls.__table__

I'm optimistic that there's some way to refactor our code to provide support for both DeclarativeMeta and DeclarativeBase. But I need someone else to step up and help figure it out, it's not something I have time to devote to.

@davidism davidism changed the title SQLAlchemy 2.x typing via mapped_column and dataclasses-like models via MappedAsDataclass Support SQLAlchemy 2's DeclarativeBase and MappedAsDeclarative Nov 21, 2022
@davidism davidism changed the title Support SQLAlchemy 2's DeclarativeBase and MappedAsDeclarative Support SQLAlchemy 2's DeclarativeBase and MappedAsDataclass Nov 21, 2022
@tadams42
Copy link
Author

I understand what and why is happening but, unfortunately, have no useful input on how.

Thanks for looking into this.

@baggiponte
Copy link

Ciao! I had a look at the previous issues and this seems to me the first time the compatibility with SQLAlchemy 2.0 is discussed, am I right? This seems like a pretty advanced topic to start contributing to, but I'd be willing to help out with some guidance.

@davidism
Copy link
Member

Not sure that it matters, but this is the first time compatibility with DeclarativeBase was brought up, not with SQLAlchemy 2 in general.

I don't have any guidance to give besides what I've written above. I don't know how to solve this any more than you do, I would be doing the same investigation.

@behai-nguyen

This comment was marked as off-topic.

@jdimmerman
Copy link
Contributor

jdimmerman commented Dec 17, 2022

I can take a look at this if that works for you @davidism @baggiponte (though not suggesting I'll definitely make the the required changes, in case others have time/appetite)

@davidism
Copy link
Member

No need to ask to work on something, go for it.

@jdimmerman
Copy link
Contributor

jdimmerman commented Dec 18, 2022

Query result typehints (my selfish concern) actually work fine when upgrading to the latest sqlalchemy 2.0 beta without using DeclarativeBase.

That being said, a few things:

  1. I did need to change the return value typehint on _make_scoped_session for session to be properly typed and therefore types to flow to query results properly. I'm unclear why this change was necessary but is likely clear to others. Should I submit a PR with this change?

    # current
    from sqlalchemy import sa
     def _make_scoped_session(self, options: dict[str, t.Any]) -> sa.orm.scoped_session
    
    # what works for typing `db.session` properly:
    from sqlalchemy.orm import scoped_session
    def _make_scoped_session(self, options: dict[str, t.Any]) -> scoped_session
    
  2. Types don't flow through the extension object methods and convenience Model.query as of now. I didn't look at this, probably worth a separate issue to track. Example:

    todo1 = db.session.get(Todo, my_id)  # properly typed as Todo | None
    todo2 = db.get_or_404(Todo, my_id)  # improperly typed as Any
    todo3 = db.session.query(Todo).all()  # properly typed as List[Todo]
    todo4 = Todo.query.all()  # improperly typed as Any
    

    the extension object methods can be solved but must use internals from sqlalchemy. Example:

    from sqlalchemy.orm._typing import _O
    from sqlalchemy.orm.session import _EntityBindKey
    def get_or_404(
        self,
        entity: _EntityBindKey[_O],
        ... (etc)
    ) -> t.Optional[_O]:
    

    Not sure yet about the query property on Model.

  3. Finally note that there is also a DeclarativeBaseNoMeta equivalent to DeclarativeBase that their docs suggest could be used when you're already using a meta class.

@pamelafox
Copy link
Contributor

pamelafox commented May 23, 2023

Thanks @jdimmerman for the investigation, very helpful.

  1. This suggestion is being made in pr Fix little problem with type hints and adopt a test to make sure it works in all platform #1207
  2. I made an attempt for typing the view functions in pr Add type to get_or_404, update tests #1208
  3. DeclarativeBaseNoMeta seems to work for me, with a slight mod of this package.

In flask_sqlalchemy/extension.py:make_declarative_base, I changed the if to:

if not isinstance(model, sa.orm.DeclarativeMeta) and not issubclass(model, sa.orm.DeclarativeBaseNoMeta):

Then in my own code, I inherited from DeclarativeBaseNoMeta:

class BaseModel(DeclarativeBaseNoMeta):
    pass

And specified that as the model class:

db = SQLAlchemy(model_class=BaseModel)

And that enabled me to use the Mapped attributes.

See full change in:
https://github.com/pamelafox/flask-db-quiz-example/pull/18/files

I'll see if I can make the change to extension.py inside flask-sqlalchemy itself with tests.

I haven't tried data classes however, don't know if that'll work too.

@pamelafox
Copy link
Contributor

Okay, so I've done more investigation of my approach inside the flask-sqlalchemy codebase itself. This comment may be a bit long as I share my findings.

First, in order to understand the metaclass/subclass distinction better, I made a diagram of relevant SQLAlchemy classes:
sqlalchemy drawio

I realized that my above approach was also compatible with using DeclarativeBase (versus DeclarativeBaseNoMeta) as long as I checked for it in the make_declarative_base function.

With that small change, I could use Flask-SQLAlchemy features like get_or_404. However, I couldn't use the functionality provided by the mixins in model.py, like table name generation, bind keys, and custom repr. All of those are implemented as metaclass mixins, which isn't compatible with the new subclass approach.

The SQLAlchemy2.0 docs suggest 2 main approaches for using mixins with the DeclarativeBase:

  1. Defining mixin classes, and mixing those into the actual table subclasses (e.g. class Table(MyMixin, Base))
    https://docs.sqlalchemy.org/en/20/orm/declarative_mixins.html
  2. Augmenting the Base class itself. https://docs.sqlalchemy.org/en/20/orm/declarative_mixins.html#augmenting-the-base

A third possibility mentioned in some GitHub threads is to use @event listeners, but I could not find an event that was fired at the appropriate time for setting tablename. Only one event is thrown before that happens, and that particular event doesn't provide access to the cls object anyway.
https://docs.sqlalchemy.org/en/20/core/event.html#events

I tried both approaches #1 and #2 but it felt weird for Flask-SQLAlchemy to create its own Base, so I settled on #1 as the better of those options. Here's what it looks like:
https://github.com/pallets-eco/flask-sqlalchemy/compare/3.0.x...pamelafox:flask-sqlalchemy:mixin-base?expand=1

I basically combined all the previous mixins into a single DefaultMixin class in model.py, and then manually added those to my user-defined classes. And there you see the drawbacks to this approach: 1) the mixins are mashed together for ease of adding 2) the dev has to add the mixin themselves. I don't know of a way in Python to auto-add a mixin to newly created db.Model subclasses.

So, in conclusion:

  • 🙂 It looks like a fairly minor change to get some aspects of flask-sqlalchemy working with the 2.0 classes.
  • 😢 Getting the mixins working is harder, and its not obvious what the API should look like. It also introduces some code/test duplication.

I'm happy to send the PR for the minor change if that still seems useful/desired.
A next step might be asking the SQLAlchemy team for their insight in the GitHub discussions.

@pamelafox
Copy link
Contributor

I've made another branch with a different approach:
https://github.com/pallets-eco/flask-sqlalchemy/compare/3.0.x...pamelafox:flask-sqlalchemy:mixin-alternate?expand=1

In this one, the dev only needs to specify the model class of DeclarativeBase:

b = SQLAlchemy(app, model_class=DeclarativeBase)

Then, in the flask-sqlalchemy code, it dynamically creates a Base class that mixes in a subclass version of each mixin:

  model = types.new_class(
      "Base",
      (BindMixin, NameMixin, ReprMixin, sa.orm.DeclarativeBase),
      {"metaclass": type(sa.orm.DeclarativeBase)},
      lambda ns: ns.update(body),
  )

This code passes at least basic tests for the mixin functionality - bind key, name, repr, etc. And if someone wanted just some of those, they could create their own Base class.

This seems fairly promising as an approach.

@davidism
Copy link
Member

Your diagram is a much more organzied form of what I kept in my head when working on all this. 😅 Add the timing of when metaclass.__new__ vs parent.__init_subclass__ is called, and notice that we have some behavior that needs to run before and then after table_cls is called, and you'll represent the full problem.

I'm not tied to using any specific solution. metaclasses, mixins, or __init_subclass__, single vs multiple classes, are all fine and I don't mind breaking compatibility at that level. The second approach sounds promising, if you want to make a PR and keep working through it.

@pamelafox
Copy link
Contributor

Thanks @davidism for the reply, glad this seems promising. I realized after posting that last comment that the original request was from @tadams42 wanting MappedAsDataclass and my suggested interface didn't account for that.
So, I have take #3!

https://github.com/pallets-eco/flask-sqlalchemy/compare/3.0.x...pamelafox:flask-sqlalchemy:mixin-three?expand=1

In this one, the developer writes the BaseModel themselves, such as:

class BaseModel(MappedAsDataclass, DeclarativeBase):
    """subclasses will be converted to dataclasses"""
    pass

db = SQLAlchemy(app, model_class=BaseModel)

And then flask-sqlalchemy constructs db.Model using the bases to create the mixed in version:

      body = {"__fsa__": self}
      model = types.new_class(
          "Base",
          (BindMixin, NameMixin, ReprMixin, *model.__bases__),
          {"metaclass": type(declarative_bases[0])},
          lambda ns: ns.update(body),
      )

I've parameterized the test to check that it works with various combos of DeclarativeBase, DeclarativeBaseNoMeta, and MappedAsDataclass.
Notably, if you use MappedAsDataClass, the SQLAlchemy repr will win (over the flask-sqlalchemy repr), due to how they assign it in the subclass. That seems correct from a developer perspective since repr() is a big selling point for data classes.

I'll keep running tests and also look into the timing, and send a PR based on this approach, since it seems like the best so far.

@jbhanks
Copy link

jbhanks commented Jun 5, 2023

I am currently trying to figure out a way to define my model that doesn't depend on Flask-SQLAlchemy. I have tried SQLAlchemy(model_class=Base), with Base imported from my model. That model is defined in a file that I hope to share with non-Flask projects.

The error I have been getting is 'Base' has no attribute '_run_ddl_visitor'. Is this because my Base is like:

class Base(MappedAsDataclass, DeclarativeBase):
    """subclasses will be converted to dataclasses"""

And should I make another help thread about my specific case, or is my issue basically this issue? It sounds like it's being worked on but not there yet?

@pamelafox
Copy link
Contributor

That error is coming from SQLAlchemy (_run_ddl_visitor is defined in that codebase, not this one). I'm somewhat surprised you got error instead of a metaclass error, but perhaps you've got a slightly different setup. I don't believe it's possible to use that Base with the current Flask-SQLAlchemy codebase.

I sent out a PR last week with changes to support a Base like that, so you could try bringing in my changes and see if they work for you.

#1215

@jbhanks
Copy link

jbhanks commented Jun 6, 2023

That error is coming from SQLAlchemy (_run_ddl_visitor is defined in that codebase, not this one). I'm somewhat surprised you got error instead of a metaclass error, but perhaps you've got a slightly different setup. I don't believe it's possible to use that Base with the current Flask-SQLAlchemy codebase.

I sent out a PR last week with changes to support a Base like that, so you could try bringing in my changes and see if they work for you.

#1215

I did also get metaclass errors with some things slightly different. I'm up for trying your changes, but I will almost certainly need some hand-holding.

@pamelafox
Copy link
Contributor

To try the branch, first make sure Flask-SQLalchemy is uninstalled in your environment and then install it from the branch. Here's how I did that for a project using pip:

pip uninstall flask-sqlalchemy

Then I replaced flask-sqlalchemy in my requirements.txt with this:

git+https://github.com/pamelafox/flask-sqlalchemy.git@mixin-three

And ran:

pip install -r requirements.txt`

You can also directly install with:

pip install git+https://github.com/pamelafox/flask-sqlalchemy.git@mixin-three

Here's a sample app I just tested using the same base classes you're looking for - this diff shows my diff between SQLAlchemy 1.4 and 2.0:
pamelafox/flask-db-quiz-example@main...mapper

@pallets-eco pallets-eco deleted a comment from behai-nguyen Jun 6, 2023
@jbhanks
Copy link

jbhanks commented Jun 7, 2023

Thank you! I was able to install your branch and now I can it least run db = SQLAlchemy(app, model_class=MyModelGitPackage.MySharedSchemaUsingMappedDataclasses.Base) without error.

@pallets-eco pallets-eco deleted a comment from jbhanks Jun 8, 2023
@yomajo
Copy link

yomajo commented Jul 7, 2023

Installed third version of @pamelafox repo (git+https://github.com/pamelafox/flask-sqlalchemy.git@mixin-three):

        "flask-sqlalchemy": {
            "git": "https://github.com/pamelafox/flask-sqlalchemy.git",
            "ref": "8fdc47acf862a000747c4c593cce7f21ac00e42e"

After setting up minimal flask app I got suggestions out of the box, except for db.select, even though it's valid. Also results have to be typehinted, but I suppose there's zero chance to avoid it.

Overall, much much more pleasurable to see reduced white color in vscode. Great job! Hope this gets into release soon!

image

@tadams42
Copy link
Author

Finally got time to try all this out.

Fully converted small Flask app with mixin 3 approach and latest SQLAlchemy 2. App has 10 or so models of various complexity and probably enough code to hit any problems in new flask-sqlalchemy. I'd refactored all models and mixins to new Mapped and mapped_column declarations, tried adding/changing few of them and then generating migrations via alembic, etc...

All in all, everything works perfectly! 🎊 🎉

@pamelafox Awesome work, thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

8 participants