OneToOneField #239
OneToOneField #239
Conversation
Pull Request Test Coverage Report for Build 1044
💛 - Coveralls |
Pull Request Test Coverage Report for Build 1002
💛 - Coveralls |
| self._do_prefetch(instance_list, field, related_query) | ||
| for field, related_query in self._prefetch_queries.items() | ||
| ] | ||
| await asyncio.gather(*prefetch_tasks) |
There was a problem hiding this comment.
👍
Yes, since we are now properly concurrency safe, we should probably do more I/O concurrently instead of in loops
| fields.BackwardFKRelation, | ||
| fields.BackwardOneToOneRelation, | ||
| ), | ||
| ): |
There was a problem hiding this comment.
Should probably extend the field definition by addind a "o2o_fields" list instead. o2o may be implemented much like foreign keys, but are conceptually different. So lets keep them separate in describe_model
| @@ -181,11 +182,12 @@ def _get_table_sql(self, model, safe=True) -> dict: | |||
| else "" | |||
| ) | |||
| # TODO: PK generation needs to move out of schema generator. | |||
| if field_object.pk: | |||
| if field_object.pk and not isinstance(field_object.reference, OneToOneField): | |||
There was a problem hiding this comment.
This is OK for now, but we should remember to fix this in #206
|
Thank you @grigi . I will try unit tests, will let you know if help was needed. |
|
@grigi please take a look. Added test to cover the added code. |
|
I'll have a look today :-) |
|
I have a question, form a user's pov, is there such a thing as a backward One-to-One field? The behaviour should be identical to forward foreign key, right? As in |
|
Beautiful PR 👍 Could I ask that you add yourself to CONTRIBUTORS and a quick description of the change in CHANGELOG? (e.g. section 0.15.2) The only change I could request is to merge "o2o_fields" & "backward_o2o_fields" in the describe model. As the user would use them symmetrically. Kind of like how m2M is used symmetrically. One can still see by the type of "Backward..." how the one is implemented if you really NEED to know. Otherwise: Awesome work! 😎 |
|
Thanks @grigi ! I can merge "o2o_fields" & "backward_o2o_fields" but before that, let's make sure if it is not going to be confusing down the road. Note that, contrary to M2M case, our fields on O2O are not really symmetric. For one, in the underlying tables, backward table does not have any columns while the forward table has a column pointing to the other table. This asymmetry is also reflected in the models themselves, setting the o2o value in the forward object, means we are changing the associated column in the table, while changing the backward o2o value should be avoided (hmm, I still had to put a setter for the lazy case, but now I am thinking I need to disallow users to set the backward relation) my guess is because of this asymmetry pretty much in all cases users might need to distinguish between forward and backward o2o even if we merge them into one. I would be happy to discuss this more if needed, but if you feel confident, I can go ahead to merge them. |
|
Right, so not entirely symmetrical. One can set the o2o relation from one side only, but read from both sides the same. Is there anything you feel you need to do before I merge? |
|
Yes let do the additions you wanted and one small fix to remove setter from
the reverse side altogether.
…On Wed, Nov 27, 2019, 10:36 AM Nickolas Grigoriadis < ***@***.***> wrote:
Right, so not entirely symmetrical. One can set the o2o relation from one
side only, but read from both sides the same.
Is there anything you feel you need to do before I merge?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#239?email_source=notifications&email_token=AADUI55NEPAYWUEBRN3BG33QV243XA5CNFSM4JOEDQB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFKM33Y#issuecomment-559205871>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADUI5ZGJYTTAXKZKHRFKLTQV243XANCNFSM4JOEDQBQ>
.
|
|
@grigi I think this should do... |
|
Awesome PR 👍 Merging now. |
|
Release 0.15.3 with this changes :-) |
|
Thank you! |
I implemented most of what is required to make OneToOneField working both in prefetch and lazy mode. I have tested most cases, although there is no unit tests yet. Just wanted you to take a look and comment. It may not be completely ready to be merged yet though.
thanks
--sina