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

Add psycopg3 support #131

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add psycopg3 support #131

wants to merge 6 commits into from

Conversation

bufke
Copy link
Contributor

@bufke bufke commented Mar 23, 2023

Fixes #130

Python 3.5/6/7 CI failure is not related and probably should be removed.

I'm not sure how to run tests locally and add a tox configuration that actually tests Django 4.2 with psycopg3. I tested this by running it on another Django project I maintain.

@bufke bufke marked this pull request as ready for review March 23, 2023 19:51
@timabbott
Copy link
Collaborator

A PR to fix CI would be appreciated; I have very little time to maintain this project.

@bufke
Copy link
Contributor Author

bufke commented Mar 24, 2023

Sure - I'll open a new PR to remove Python 3.5, 3.6, 3.7 and add 3.11. As well as test only currently supported django versions. We should wait on Django 4.2 CI until it's released.

If there is an easy way to run the tests locally, please let me know. I'm only testing by pushing to github for CI or running on my own project and poking around.

@timabbott
Copy link
Collaborator

@bufke to what extent have you investigate whether there's a psycopg3 feature replacing this adapters feature?

@mateuszmandera @andersk it'd be great if you can help review this as well.

@bufke
Copy link
Contributor Author

bufke commented Apr 6, 2023

There are still adapters but they appear to be entirely different. See docs. I know very little about psycopg and didn't know what an adapter was until I tried Django 4.2.

Database.extensions.AsIs(int(x)) isn't a very complex adapter. As I understand, take the integer and makes it an integer. And use that as is. It doesn't seem like it needs an adapter at all, but apparently does on psycopg 2. With psycopg 3 it appears to just work.

All I did to test was click around in django admin. I was able to manipulate a bitfield field after omitting the adapter. More testing would be great and it should be added to the tox matrix (along with django 4.2 now that it's released).

@bufke
Copy link
Contributor Author

bufke commented Apr 6, 2023

I added django 4.2. I haven't been able to get the tests to run psycopg 3 yet. https://github.com/bufke/django-bitfield/actions/runs/4631676030/jobs/8194831780?pr=1

@bufke
Copy link
Contributor Author

bufke commented Apr 6, 2023

The test should now use psycopg3 and fail on exactly 3 tests. That seems like an improvement.

@bufke
Copy link
Contributor Author

bufke commented Apr 6, 2023

I'm not sure how to fix this or if it's even possible to fix. I don't think I have the time to address this. On my brief research I think there are two problems.

  1. We need to make a Dumper and maybe a Loader. There is no more AsIs adapter.
  2. We need to add the dumper to the the django connection, but I see no way to do so.

This monstrosity fixes some errors if we edit django's psycopg_any

    @lru_cache
    def get_adapters_template(use_tz, timezone):
        # Create at adapters map extending the base one.
        ctx = adapt.AdaptersMap(adapters)
    
        from psycopg.types.numeric import _IntDumper
        from bitfield.types import Bit, BitHandler
        class AsIsDumper(_IntDumper):
            def dump(self, obj):
                return super().dump(int(obj))
        ctx.adapters.register_dumper(Bit, AsIsDumper)
        ctx.adapters.register_dumper(BitHandler, AsIsDumper)

With that we get one less error. We might need a Loader for it to work fully.

Before the adapter was set globally on Database.extensions but now it's set on the connection. Django has this function that sets them. I don't see how we could hook into this.

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

Successfully merging this pull request may close these issues.

Support psycopg 3
2 participants