Duplicate model instance

[WisdomPill]

Is your feature request related to a problem? Please describe.
Usually in django to duplicate a model and to not copy every field there is a work around that is pretty straightforward and easy to do. This can be done via setting the pk to None. But with tortoise is not that easy, setting also the attribute ._saved_in_db is necessary.

Describe the solution you'd like
Not to access a private variables for this trick to work.

Describe alternatives you've considered
Maybe a copy method as well, that would be much more intuitive.

Additional context
Example of the code needed as of now

book = Book(title='1984', author='George Orwell')

print(book.pk)

await book.save()

print(book.pk)

book.pk = None
book._saved_in_db = False
book.title = 'Animal farm'
await book.save()

print(book.pk)

print(await Book.all().count())

Example of the code that I would like to use instead

book = Book(title='1984', author='George Orwell')

print(book.pk)

await book.save()

print(book.pk)

book.pk = None
book.title = 'Animal farm'
await book.save()

print(book.pk)

print(await Book.all().count())

[WisdomPill]

My question is, what is that attribute used for? I guess is not that easy to change something like this. I read a little bit the code in order to figure out how it is used and why it is needed, but I did not spend much time and did not understand properly the importance of that field

[grigi]

Is a model with pk set an update, or a custom create?

Pk=None shouldn't change, so we can make it understand that aa expected?

[WisdomPill]

Sorry I did not understand your queries, can you please rephrase?

The model is created with .save not with .create.

My question is, let's say that I have a model with multiple fields and I want to create a brand new one that is the same, like the following

class Book(Model)
    author = fields.CharField(max_length=32)
    title = fields.CharField(max_length=32)
    # other fields

and create a copy of a book since they share the same author or other fields for that matter. Let aside the fact that the author can be a foreign key to another model, this is just an example.

Wouldn't be easier behaving like django and making this possible by just setting the pk to None?

[AEnterprise]

null could be a valid PK (usually bad design but technically valid) so just setting it to null on an existing object alone is not gona be enough to determine if it should do a create or a update

hence the _saved_in_db field, that's how it determines if it should try to create or update the object

the simplest solution would probably be a .clone helper that just makes a new instance that isn't saved yet?

[WisdomPill]

@AEnterprise a .clone method is a clean solution in my opinion, as it is also self explanatory instead of setting the primary_key to None. Even if I am in favor of using None trick to in stay favor people coming from django, I am a little biased here, I am one of those people.

But would you encourage bad design?
Furthermore, the primary_key is used in the __eq__ method, so it would be a very bad design to use as a primary_key.

[WisdomPill]

By the way, I would very much open to create a PR for a .clone method.

Are you sure, _saved_in_db is the only thing that needs to be changed for a .clone method?
What happens to the primary_key?

[AEnterprise]

is it bad design to have a null key in your database? absolutely. mysql, mariadb, postrgess and most other database systems do not allow this. However sqlite (that tortoise supports) still allows it for backwards compatible reasons so it techincally is a thing: https://sqlite.org/lang_createtable.html

i can't think of a good reason to do it but it's a thing so the assumtion of "null"/"None" means that there is no pk set is technically not a valid assumption in all cases

as for _saved_in_db: pretty sure that is the case but i didn't design it so i might be missing something i overlooked when working wit it before
if _saved_in_db is false but it has a primary key it'll try to set that as primary key when creating

[WisdomPill]

I understand, but this raises a question, how does django handle this? 😂

Are you open for a PR for the .clone method?

I am eager to start contributing to this library that I just started using.

[grigi]

@AEnterprise Whilst null is a valid PK value, it's usage is not possible with auto-number integer rowid PKs. So we can happily let a null pk be replaced by the "generated" index if it has one. I can't think of a case where it would break in such a case. We know if the field is nullable after all.

@WisdomPill However, a .clone() method would still be useful to create a separate instance, but it can't do it for all cases (e.g. the primary key is not generated/defaulted).
What should be correct behaviour be in that case? Do we raise an error, and expect a new PK passed in via a parameter? e.g. .clone(pk=<something expliit>) would be required in those cases.

Both are viable?

[grigi]

Actually, we will likely have to special case the null as right now if you create an object with autonumber pk like so Tables.create(pk=200) it will insert a PK of 200 instead of letting the DB generate it. Tables.create() will let it generate. But Tables.create(pk=None) is going to try insert a null and explode. Likely the user wanted the generated value instead. So we should handle this edge case in any case.

[WisdomPill]

@WisdomPill However, a .clone() method would still be useful to create a separate instance, but it can't do it for all cases (e.g. the primary key is not generated/defaulted).
What should be correct behaviour be in that case? Do we raise an error, and expect a new PK passed in via a parameter? e.g. .clone(pk=<something expliit>) would be required in those cases.

Both are viable?

I think an error should be raised like when we are creating a model instance in the "standard" way.
I like the idea to have a pk argument.

About the null PK I am very skeptical, I do not see a point in using it, it is bad design as @AEnterprise have said but still valid. I think that is an edge case that should be supported so using the pk to None is not a possibility, although simple and intuitive in my honest opinion.

Django for instance does not let users do it as of my little knowledge based on little reading of the form code. It is a strong point that of making the user use a simple design to guide them through the right path and making bad design difficult to replicate.

[grigi]

Ok, so I am busy implementing this, and the way to handle just setting pk to None just doesn't feel right. It can allow for silently swallowing errors based on a guess what the user meant, which is a REALLY BAD THING.
(I actually did but actually just trying to describe the condition when this behaviour will trigger was a smorgasbord of conditions. So it will likely confuse.)

We also can't disallow PK being nullable as:

1. I have had to use bad databases like that before
2. This would break for anyone that depended on it.

So, I feel that I should only do the .clone() method.
Am busy with tests on it right now. Should be in for next release.

[WisdomPill]

Well, if you need some help count me in, I am busy at work for a couple of days and I thought on working on it as well on the weekend. Can you please assign this issue to me or yourself so that we do not work on the same thing? I also wanted to work on using asyncio test cases that are built in with python3.8 from #416, can you assign that issue to me please?

[grigi]

I'm almost done with this, You're welcome to do #416, so I assigned it to you.

[WisdomPill]

Thanks!

[WisdomPill]

Do you also think these issues should be added to the 0.17 project?

As of now, 0.16 had many releases

[grigi]

Normally we only bump the major release if it is a large change that people need to be aware of. And yes, I agree that 0.16 had by now too many releases 🤷

I originally planned 0.17 to include a field refactor so we can support oracle syntax without too many hacks. And so we can differentiate between preparing/escaping data for parameter-passing or inline-SQL or for LIKE.
e.g. JSON for Postgresql needs to be a string, but we can't treat it as a string when it's inline, because it's supported natively, it must NOT be quoted like a string.

Also, turns out that Tortoise doesn't support nullable PK's for a while now. We generate an error, so my previous case is actually a lot simpler to describe. So the val=None could work for cases where we can generate the pk, and it is now much simpler a condition. pk=None can safely imply _saved_in_db=False.

[grigi]

Released 0.16.13 with clone and implicit change 😄