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 mixins #256

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

byrman
Copy link
Contributor

@byrman byrman commented Feb 28, 2022

Add support for mixins to be used with SQLModel. See #254.

@antont
Copy link

antont commented Feb 28, 2022

I thought I was already using mixins, when say this in our fastapi-users integration (using https://github.com/fastapi-users/fastapi-users-db-sqlmodel)

class User(SQLModelBaseUserDB, UserBase, table=True):

But am guessing that works because both those classes inherit SQLModel.

Just double checking: is it a good idea to add this to support mixins that are not SQLModel's? I guess yes.

@byrman
Copy link
Contributor Author

byrman commented Feb 28, 2022

Would it be possible for you to test this with your project, @antont? It would be reassuring if your tests pass as well.

@antont
Copy link

antont commented Feb 28, 2022

Would it be possible for you to test this with your project, @antont? It would be reassuring if your tests pass as well.

Yes, I can do that either later today or else at some point tomorrow.

@bolau
Copy link

bolau commented Mar 1, 2022

Is there hope for a merge at all? I see a long list of useful PRs that haven't been merged and are outdated by now.

@antont
Copy link

antont commented Mar 1, 2022

Is there hope for a merge at all? I see a long list of useful PRs that haven't been merged and are outdated by now.

I just merge the PRs we need in our company fork (public). I guess others do the same. But yes there is hope, tiangolo seems to merge too and release when he has time, is just busy working on these on other fronts too, fastapi, pydantic etc.

SQLModel is quite small and was pretty solid when it was first released, I'd bet most of the PRs are not outdated as not much has changed.

@antont
Copy link

antont commented Mar 1, 2022

@byrman - works for me, so didn't break mixins that derive from SQLModel. makes sense too, didn't suspect problems either and the change seems logical too.

@byrman
Copy link
Contributor Author

byrman commented Mar 1, 2022

Thanks for testing in the wild, @antont!

@amontalban
Copy link

Hope to see this merged soon 🙏

@bolau
Copy link

bolau commented Apr 28, 2022

Until this is merged, here's an easy workaround:

class MyMixin():
    __config__ = None  # <---- THIS!
    # your code here

class MyModel(SQLModel, MyMixin, table=True):
    # your code here

@byrman
Copy link
Contributor Author

byrman commented May 5, 2022

Unfortunately, this use case is not covered: #330. Only methods and simple attributes appear to work.

@AyrtonB
Copy link

AyrtonB commented May 10, 2022

Thanks for testing these solutions.

@bolau I just tried your suggested approach and am having an issue where the introduction of __config__ = None is leading to an AttributeErrorwithinpydantic`. Is this something you've encountered before?

  File "/code/./library/module/utils.py", line 15, in <module>

    class ActiveRecord(SQLModel):

  File "/usr/local/lib/python3.9/site-packages/sqlmodel/main.py", line 277, in __new__

    new_cls = super().__new__(cls, name, bases, dict_used, **config_kwargs)

  File "/usr/local/lib/python3.9/site-packages/pydantic/main.py", line 292, in __new__

    cls.__try_update_forward_refs__()

  File "/usr/local/lib/python3.9/site-packages/pydantic/main.py", line 773, in __try_update_forward_refs__

    update_model_forward_refs(cls, cls.__fields__.values(), cls.__config__.json_encoders, {}, (NameError,))

AttributeError: 'NoneType' object has no attribute 'json_encoders'

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #256 (3c68587) into main (75ce455) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #256   +/-   ##
=======================================
  Coverage   97.76%   97.77%           
=======================================
  Files         187      188    +1     
  Lines        6268     6280   +12     
=======================================
+ Hits         6128     6140   +12     
  Misses        140      140           
Impacted Files Coverage Δ
sqlmodel/main.py 84.92% <100.00%> (ø)
tests/test_mixin.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

📝 Docs preview for commit 3c68587 at: https://630f80b819e9472c1e2ab9ba--sqlmodel.netlify.app

@bolau
Copy link

bolau commented Sep 27, 2022

Sorry, no. Maybe it's not a good workaround after all ;-) Hope that this gets merged sometime soon...

@deajan
Copy link

deajan commented Nov 22, 2022

#256 fixes Mixin usage with https://github.com/absent1706/sqlalchemy-mixins ;)

@github-actions
Copy link

📝 Docs preview for commit 6f0c4bb at: https://639ce01f2b35ad0062e48357--sqlmodel.netlify.app

@deajan
Copy link

deajan commented Dec 27, 2022

Well... up !
Any reason mixin support cannot be merged ?

@thedamnedrhino
Copy link

@byrman will relationship fields in mixins work with this?

@byrman
Copy link
Contributor Author

byrman commented Feb 1, 2023

@byrman will relationship fields in mixins work with this?

I'm afraid not, take a look at #330.

@russellromney
Copy link

Until this is merged, here's an easy workaround:

class MyMixin():
    __config__ = None  # <---- THIS!
    # your code here

class MyModel(SQLModel, MyMixin, table=True):
    # your code here

This worked well for me. I don't see official support for mixins in the documentation yet.

@deajan
Copy link

deajan commented Sep 1, 2023

@tiangolo Sorry to bother, but any reason this actually doesn't get pulled ?

@tiangolo tiangolo added the feature New feature or request label Oct 22, 2023
@jpmckinney
Copy link

The __config__ = None workaround works for me, but causes this mypy error (as expected):

error: Definition of "__config__" in base class "BaseModel" is incompatible with definition in base class "MyMixin"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request investigate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants