tortoise / tortoise-orm

Familiar asyncio ORM for python, built with relations in mind
https://tortoise.github.io
Apache License 2.0
4.7k stars 391 forks source link

Add support for rqlite #164

Open superloach opened 5 years ago

superloach commented 5 years ago

Describe the solution you'd like I would like to be able to use Tortoise with rqlite databases. Rqlite is an sqlite-based system that serves over HTTP, and can handle multiple database instances.

Describe alternatives you've considered I've already created the aiorqlite library to support accessing an rqlite database asynchronously. I've also begun work on adding rqlite to tortoise at the add-rqlite branch, but I would like assistance in checking my code - neither aiorqlite nor this branch have been rigorously tested yet.

grigi commented 5 years ago

Interesting project.

For the rqlite tortoise driver, I would try and consider the absolute differences? e.g. is the syntax exactly the same? What about parameter handling? If is is pretty much identical, just use the existing sqlite schema generator and executor, only replacing the client (which is the transport).

For aiorqlite: I see that OrderedDict is being used, since Py3.6 (and pypy5.6 which supports py3.5) regular dictionaries are ordered.

Still would be interesting to see how it progesses :-)

superloach commented 5 years ago

Hey, thanks for your input!

The syntax for rqlite is identical to sqlite, yes. Paramater handling supports both named and qmark style, I don't know what Tortoise uses but I'm sure that could easily be a part of the client.

As for switching to normal dicts in aiorqlite, that's definitely a good idea - I had difficulties getting the Tortoise client to work with OrderedDicts.

grigi commented 5 years ago

Aoisqlite only supports question marks.

grigi commented 5 years ago

For testing the driver, I'd recommend you do a multi-pronged approach:

  1. Choose minimum py version, e.g. py3.5 or py3.6 (this sets your minimum supported level) Don't go under 3.5 as the loss of the async/await syntax will severely impact code readability. 1.1. Consider using something like pyupgrade to help auto-clean-up the code for your base version.
  2. Use all the linters, especially mypy (although this requires typing annotations)
  3. Set up a test suite that runs against an rqlite instance
  4. Get some basic golden-path tests in
  5. make that part of CI, e.g. Travis
  6. Use something like https://hypothesis.readthedocs.io/ to automate testing for edge cases in data.
  7. Now design some edge case tests that could occur.

Other things to consider:

Just a quick dump of thoughts.

superloach commented 5 years ago

I will definitely be looking into adding tests to aiorqlite, as well as test cases for the rqlite client in Tortoise.

Regarding your last two points:

grigi commented 5 years ago

Another thing this was useful for me, is to see what is required from an outside POV what to do to add new DB support to Tortoise, and, yay! it seem simpler :-D

I think we should consider moving the config_generator dict out to the driver init.py, and then add a "Tortoise.register_driver()" so that external drivers can easily be added? :thinking: Just making adding drivers that may never get mainlined easier?

The issue with mainlining drivers, is that we need to support them, which means adding to CI, fixing bugs when we notice them, keeping the drivers up-to-date ito interface etc... So, sometimes we may want to have extra DB support external to the project.

(Not that I'm saying we will refuse rqlite, Just that adding a DB driver is a lot more work)

grigi commented 5 years ago

And I'm going to ask you to write a small essay as to your experiences adding DB driver to the docs when you are done, of course :-D

Hmm, we need an essay section in our docs…

bs32g1038 commented 5 years ago

@grigi I think adding too many databases can lead to bloated projects and difficult maintenance. At present, the existing database support should be better. I've seen in a project that an ORM supports various databases, and its code is very complex, and it's very difficult for other developers to participate in development. It also consumes the extra energy of maintaining the project itself.

So I suggest that we should focus on several databases that are currently supported., and create ecology.

I think the project will be more viable. This is my suggestion.

Last, this is a great project! I hope it gets better.

grigi commented 5 years ago

@bs32g1038 (Jason Li) I agree that this project is not at the point where we consider it mature enough to expand what we need to support. We are not at version 1.0 yet.

However, there is no reason to block someone from doing what they feel they need to do. (This is the spirit of Open Source, after all!) So I'm perfectly happy to alter the codebase to make is easier for an extension to simply and reliably work. (As long as this doesn't increase complexity) And I'm also perfectly happy to do a little bit of mentoring (as time allows).

So, no need to worry, right now we are working towards making this project more mature. The codebase as a whole could benefit from refactoring to make it simpler in a few areas. One of them is how fields and database dialects interact, and this issue could help point out ways of doing that :-)

FYI: I just did a quick check, We only have 4000 LoC on the main project right now, 400 LoC in contrib, and 5500 LoC of tests… So still pretty small. And, I do intend to keep it small for reasons of maintenance.