[WIP] Generic primary key implementation #122
Conversation
|
Wow, this PR does a lot. Adds new datatypes even, and pg-specific optimisations, and makes uuid PK-able :-) |
|
Well, it's already should be pk-able, like any hashable value field. It's just that I was already beaten on last line of code and decided to write tests for other field types later |
tortoise/fields.py
Outdated
| """ | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(type=UUID, *args, **kwargs) |
There was a problem hiding this comment.
Should generated not also be auto-set when pk=True?
|
@abondar Do you need help in completing this? It has been sitting around for four weeks now? |
|
Hi, sorry for dragging this for so long, couldn't find time to finish it and now I am on vacation until middle of may. |
|
Cool, enjoy your vacation 😄 |
|
@abondar I rebased on latest master, fixed the linting/typing related issues, fixed the M2M table creation to use the right FK key type, and fixed postgres to work again. I see there is a new attribute I also see you are trying to replace the field classes for postgres, so we can use the native types easier, I didn't try and implement an auto-replace yet. And, we need quite a few more tests and lots of documentation changes. |
|
Could we just finish the test cases for primary keys, and then merge this without the replacing field classes on a per-db basis? This PR does enough and I feel the field refactoring should be a separate issue? Because it is quite large as well. |
|
Turns out both MySQL and SQLite handles ROWID primary key tables and non-ROWID tables very differently. This is one of the many things that is going to complicate the field refactoring #97 |
…update()/delete() to convert types to db type
|
@abondar Please review my changes. I removed the custom PostreSQL uuid field type, as until we have parametrized queries for everything, we are likely to run into escaping issues when the to_db_value doesn't treat it as a string. PostgreSQL seems smart enough to parse UUIDs so for now it is a working limitation. Also, have not updated the documentation. From a code POV I'm happy with what we have here now. Also, can we discuss what is to happen for v0.12.0 ? |
|
Thank you! Regarding 0.12.0 I think this PR is big enough to be sole member of it. |
|
?? That last commit broke stuff spectacularly in Travis, but I don't know why. as it doesn't happen locally. |
|
Ok, that looks much better. |
|
@abondar Please vet the docs |
|
|
||
| checksum = fields.CharField(pk=True) | ||
|
|
||
| guid = fields.UUIDField(pk=True) |
There was a problem hiding this comment.
I think we can also mention, that filtering by .filter(pk__in=...) or whatever other filters is also supported
Closes #36
Hey @grigi
I wrote down this implementation and, surprisingly, it's even works (on sqlite).
I'll try to do cleanup on it in following days and write some more tests, but could you review it by then?
This PR carries many changes and I am afraid there could be some places which I could have done better, but missed them in all this work.
I hope you with fresh view will be able to propose some changes for the better here