Type: Feat - Add support for MariaDB connection string

tebrown commented 9 months ago

Description

Fixes: #490

Add support for "mariadb" connection strings. This is needed because sqlalchemy has a bit of special sauce for MariaDB vs MySQL, however the GIS interface between the two is identical.

Adding to mariadb to geoalchemy2/admin/__init__.py was necessary, but not sufficient. I had to also add compiles for the mysql dialect (geoalchemy2/admin/dialects/mysql.py, geoalchemy2/types/__init__.py) that are for mariadb. I thought about splitting them out, but because they are 100% identical, it didn't make sense.

Lastly, I did add some tests. However, because MariadB and MySQL run on the same port, I didn't actually add any tests to it yet.

Checklist

This pull request is:

[ ] A documentation / typographical error fix
- Good to go, no issue or tests are needed
[ ] A short code fix
- [ ] Please include: Fixes: #<issue number> in the description if it solves an existing issue (which must include a complete example of the issue).
- [ ] Please include tests that fail with the main branch and pass with the provided fix.
[X] A new feature implementation or update an existing feature
- [X] Please include: Fixes: #<issue number> in the description if it solves an existing issue (which must include a complete example of the feature).
- [N/A] Please include tests that cover every lines of the new/updated feature.
- [N/A] Please update the documentation to describe the new/updated feature.

adrien-berchet commented 9 months ago

Ah and also, could you write something in the docs about the compatibility with MariaDB please? (for know it should be clearly mentioned that it's experimental)

adrien-berchet commented 9 months ago

Looks good! Let's see what the CI thinks about it :) I guess it will complain if we don't set up a MariaDB instance in CI?

tebrown commented 9 months ago

Tested and all these changes work. Thank you for the feedback!

tebrown commented 9 months ago

Looks good! Let's see what the CI thinks about it :) I guess it will complain if we don't set up a MariaDB instance in CI?

TBD! If it does, I can see about updating that. MariaDB is free, but I am not 100% sure how to handle the port conflicts. I will noodle on that.

tebrown commented 9 months ago

It looks like it failed on codespell. I'll see if I can fix that.

tebrown commented 9 months ago

It actually caught a bug! Neat!

adrien-berchet commented 9 months ago

Looks good! Let's see what the CI thinks about it :) I guess it will complain if we don't set up a MariaDB instance in CI?

TBD! If it does, I can see about updating that. MariaDB is free, but I am not 100% sure how to handle the port conflicts. I will noodle on that.

Cool, thanks!

tebrown commented 9 months ago

Looks like I'll be writing some tests!

tebrown commented 9 months ago

It also looks like my queries are using the PGSQL version of the function instead of the mysql.. so there is still work to do.

tebrown commented 9 months ago

@adrien-berchet I think I might be stuck. I see the mapping for the ST_AsWEKB' to 'ST_AsBinary' inadmin/dialects/mysql.py`, and I believe that this should be responsible for making the query SQLAlchemy/GeoAlchemy generates correct. However, this code:

async def test():
    async with async_session() as session:
        await session.execute(select(LocationAlert).where(LocationAlert.psap == 'psap'))

Generates the following SQL:

sqlalchemy.exc.OperationalError: (asyncmy.errors.OperationalError) (1305, 'FUNCTION orthoplex.ST_AsEWKB does not exist')
[SQL: SELECT location_alerts.psap, location_alerts.call_type, location_alerts.dst_agency, location_alerts.units, ST_AsEWKB(location_alerts.mapped_geom) AS mapped_geom, location_alerts.id 
FROM location_alerts 
WHERE location_alerts.psap = %s]
[parameters: ('psap',)]
(Background on this error at: https://sqlalche.me/e/20/e3q8)

If I manually take that SQL and change it to ST_AsBinary, it works great. But for some reason, this is generating the wrong function name. I went through every place that I could find that relates to MySQL, and it all looks kosher. What is even worse, is that it is properly generating the tables, so it MUST be partially working.

tebrown commented 9 months ago

Well, I spoke too soon. I missed a spot. I am the worst!

adrien-berchet commented 9 months ago

Ahah nice catch :⁠-⁠)

tebrown commented 9 months ago

Ok, I ran the tests locally this time, and I see errors for sqlalatest adn sqla14...however I didn't change those, and it is probably unlikely I impacted them.

ERROR:   py37-sqla14: commands failed
ERROR:   py37-sqlalatest: commands failed
ERROR:   py38-sqla14: commands failed
ERROR:   py38-sqlalatest: commands failed
ERROR:   py39-sqla14: commands failed
ERROR:   py39-sqlalatest: commands failed
ERROR:   py310-sqla14: commands failed
ERROR:   py310-sqlalatest: commands failed
ERROR:   py311-sqla14: commands failed
ERROR:   py311-sqlalatest: commands failed
ERROR:   pypy3-sqla14: commands failed
ERROR:   pypy3-sqlalatest: commands failed

adrien-berchet commented 9 months ago

I reverted the tests so for me it's ok to merge if the CI passes.

adrien-berchet commented 9 months ago

Thanks for this work @tebrown ! I'm going to push a new release so you can use it in your workflow.

geoalchemy / geoalchemy2

Type: Feat - Add support for MariaDB connection string #491

Description

Checklist