tortoise / tortoise-orm

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

Adding OneToOneField #236

Closed sinaso closed 4 years ago

sinaso commented 5 years ago

I am porting few models from a Django project as a test to see if we can replace Django with Starlette+Tortoise and one of the fields that is currently missing in Tortoise is OneToOneField.

One way to avoid this problem is of course to replace that with a ForeignKeyField, and add custom code on the related model to get the first item from the related set.

I was wondering if adding OneToOneField has ever been considered, if anyone here has suggestions on how we can implement it. Hopefully I can contribute that back to the project as well.

grigi commented 5 years ago

Hi, we recently had a conversation talking of OneToOneField, but it hasn't been a priority, since no-one requested it. (Until now). Just for clarity sakes, you are suggesting a subset of a FK that stores a reference in one table, and the reverse assumes the relationship is singular, and not multiple?

I don't think this is hard to do, as we already have much of the infrastructure in place.

If you want to help, I'm happy to let you have a go at it. Else, I'll probably only get to it in a few weeks.

sinaso commented 5 years ago

Thanks for response. I will take a look at it, and will submit a pull req when done. Chances are though that it won't be much faster than few weeks you mentioned any way.

grigi commented 4 years ago

Thank you for your work :-) I'm closing this as we dealt with it in the PR.

grigi commented 4 years ago

@sinaso I noticed your massive branch over at https://github.com/nextfit/tortoise-orm and there are some very good stuff there. I also see you are changing the fields and some parameters to be closer to Django. (I'm not against this, we can do aliases to allow both to work to preserve legacy)

I in particular like the internal naming clarifications, should make it easier for someone to read the code.

I would like to merge the codebases, or at least get a lot closer to each other?

sinaso commented 4 years ago

@grigi by all means, I have a lot of changes to the code base, it all started with our need for OuterRef and Subquery which i have implemented (it is not complete but the code is clean and expandable).

I have also factored a lot of code, reimplemented filtering, so that it creates filters on demand, etc everything passes the original tests, but more test cases need to be added for OuterRef and Subquery at least.

I would be very happy to merge back, but it could be a lot of work, let me know where you would be interested to start and we can find a easy route.

grigi commented 4 years ago

@sinaso Yes, a LOT of work.

I feel we should start by making the diff smaller (currently nearly 6000 line diff), so lets start with the lower risk items first, just to de-clutter.

E.g. the internal variable renaming.

I noted you moved the scheme generation entry into the db-client, which is the opposite of my plans, I would prefer it to be less dependant, so we can do things like offline schema generation easier. Many companies require the developer to submit schema to the DB administrators. So I would prefer to keep that the way it is for now.

The most clutter is the Field renaming to make the field names match Django's. If we do that (we would need a plan to preserve backward compatibility) we can take some of the other parts of Django where one can do models.CharField(...) instead of always doing a fields.CharField(...). We should also not be afraid of differences where we think it's warranted. e.g. the Django Model.objects.all() is a definite step backwards from Model.all() ito usability/readbility. This is only a Django-inspired ORM after all.

Once we got that sorted I'm sure the diff should start to become manageable?

Re the filter refactoring I would like @lntuition to have a look at it since he has been itching to do a refactor and to merge F & Q together. If you could point him to the commits that did that so he can have a look and see if he wants to use it or continue with his work.

sinaso commented 4 years ago

I totally agree with you that there is no need to follow Django everywhere, like the Model.objects.all() case for example.

Currently I have renamed fields inside the package, so Django's models.IntegerField() is currently fields.IntField() and is fields.IntegerField() under my brand. One solution is maybe to keep tortoise and instead add Django like fields under models so that we keep both backwards compatibility and easier Django transition?

The bigger issue however is that for better readability and to reduce coding mistakes I have changed the name of a lot of internal variables. In the process of factoring similar code, I also have two fields in all RelationFields remote_model and related_name which if I remember correctly used to be called two different things for ManyToMany and ForeignKey/OneToOne cases. There are many other cases too.

As it is, it seems like a very hard task to pick and choose commits from one branch and apply to the other. Do you have any suggestions on how to do a merge?

grigi commented 4 years ago

I would just start with the compare branches files changed section and start copying code. e.g. starting with the internal variables. There is a lot in the /backend/ section, etc...

I can start if you like?

Possibly we do a PR that we never merge, just use as a diff tracker? It should get smaller and smaller the more stuff we merge into develop? The aim would be to reduce as much of the cruft as we can till only the important changes are left so we can properly review them.

grigi commented 4 years ago

Also note, that since our branches diverged, a lot of fixes and refactoring happened, so we implemented the same features, just differently. And similar refactoring, just different, etc... And lots of bugfixes.

So, I feel like I should start working by copying some of the changes over.

The more I look at it, the more I realise that this is going to be a very slow process, and there is some changes that I'm likely not going to take.

sinaso commented 4 years ago

Sure, let's do it.

One piece which I think would be a very important addition, specially if you wanted to port Subquery and OuterRef is the use of QueryContext in the calls in queryset.py. It keeps the queries in a stack based structure, and keeps model, table and through tables if any. I noticed somewhere in the code you had to add table as a parameter next to model in most of those calls. This is just a more general case of that.

I have a feeling that it would much faster if we tried to re-implement all the changes made on the main branch on my branch from where they have diverged, although this might be just personal preference.

Do you want to start by listing what changes you don't like (or like) to be back ported?

grigi commented 4 years ago

I definitely want the Subquery stuff. Just not first.

I'm just following a method I have used successfully before, which is migrate the busyest, least-impact stuff first, so that the impactful stuff stops being obscured. It's a thing I do to clean up my own PR's when, whilst adding a feature I get distracted and did an unrelated refactor that now distracts the whole purpose of the PR. Hence my thinking of starting with some internal cleanup, and the Django rename changes (because nearly every test has been affected). That way we can cut down the 5600 line diff to, e.g. 1000 lines (or preferably even smaller) so it can actually be reviewed and discussed properly.

Remember we need to support this for a long time, and most of our users don't want API breakage. So anything official needs to have a clean public interface. And well tested. Refactoring the private interface can be done over time.

I'm less interested in speed, but knowing what happened. I see that your branch is a 5600line diff, and upstream a 8500line diff. So both are just plain massive.

:thinking: I'll have a start this evening after putting the kids to bed, and see how far I get. I'll probably make a copy of your branch locally, to allow me to keep track of the changes that is needed. Possibly to break it up into chunks, and also so the porting isn't against a moving target yet.

When we get to the big new feature, which is subqueries, it may take a while to get it fully tested and documented. I'm assuming this follows the way Django has done it?

sinaso commented 4 years ago

Yes, Subquery and OuterRef work the same way as their Django equivalents.

grigi commented 4 years ago

I started with something here: https://github.com/tortoise/tortoise-orm/pull/336

There is unfortunately quite a few changes that I can't/won't port over, I tried to document them as I get to them (also so I can keep track of them)

I'm stopping for now as its getting late, but will continue.

I think I'll basically do the following:

  1. Port over the simpler changes in that PR
  2. Consider porting over the django field/param convention change in another PR (It needs to be done in a way that doesn't break existing users)
  3. Look at the subquery stuff, but would love a sample app from you for that. Then we can re-asses.