Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Create from super #65

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Feature: Create from super #65

wants to merge 3 commits into from

Conversation

JCoxwell
Copy link

@JCoxwell JCoxwell commented Jan 8, 2014

I'm not sure if you want this feature, but I thought I'd offer it. Basically it allows you to convert a polymorphic model to a new class down the hierarchy (one level). For example, if ModelB subclasses ModelA then an instance of ModelA can become an instance of ModelB by adding the additional fields without changing it's primary key.

I use this feature to create objects that need to be classified later.

There are 2 commits which can be applied separately:

One is the feature itself with a test.

The other modifies the tests to work with MySQL. Apparently sqlite reuses ids where as MySQL does not, so I just push the ids into the repr strings for comparison. This is the simplest solution I could think of, but it's not ideal. At least they pass...

… However, this commit resolves the id problems that appear when testing on mysql in the simplest way possible. Tested on sqlite and mysql, but not postgres
…from an instance of it's superclass. Useful if you don't know what class it should be when initially created.
@vdboor
Copy link
Collaborator

vdboor commented Jan 8, 2014

This is a feature we'd surely can use. However, I'm not happy about the API yet, I'd prefer using something like Model.objects.create_...() as that's how the other ORM API's work.

In one of my projects I've also "upgraded" an object to it's subclass, using:

b = Boss(person_ptr_id=obj.pk)
b.save_base(raw=True)

See https://code.djangoproject.com/ticket/7623 :)

Do you see a way to incorporate these idea's?

@JCoxwell
Copy link
Author

JCoxwell commented Jan 8, 2014

Yes, I agree about the API. I suppose it should live in
PolymorphicQuerySet. I'll look at it and see what I can do.

Thanks for pointing me to the ticket about this feature. I hadn't noticed
it.

Joshua

On Wed, Jan 8, 2014 at 1:26 AM, Diederik van der Boor <
[email protected]> wrote:

This is a feature we'd surely can use. However, I'm not happy about the
API yet, I'd prefer using something like Model.objects.create_...() as
that's how the other ORM API's work.

In one of my projects I've also "upgraded" an object to it's subclass,
using:

b = Boss(person_ptr_id=obj.pk)
b.save_base(raw=True)

See https://code.djangoproject.com/ticket/7623 :)

Do you see a way to incorporate these idea's?


Reply to this email directly or view it on GitHubhttps://github.com//pull/65#issuecomment-31816156
.

@JCoxwell
Copy link
Author

I changed it to live in PolymorphicQuerySet for API consistency and updated
the test. The move didn't require any substantive changes.

If ModelB inherits from ModelA the call would look like:

modelb_instance = ModelB.objects.create_from_super(modela_instance,
field_dict)

I read through ticket 7623. Is someone else already working on this? It
says it's assigned to elektrrrus.

Also, do you have an opinion on the method name?

Joshua

On Wed, Jan 8, 2014 at 1:26 AM, Diederik van der Boor <
[email protected]> wrote:

This is a feature we'd surely can use. However, I'm not happy about the
API yet, I'd prefer using something like Model.objects.create_...() as
that's how the other ORM API's work.

In one of my projects I've also "upgraded" an object to it's subclass,
using:

b = Boss(person_ptr_id=obj.pk)
b.save_base(raw=True)

See https://code.djangoproject.com/ticket/7623 :)

Do you see a way to incorporate these idea's?


Reply to this email directly or view it on GitHubhttps://github.com//pull/65#issuecomment-31816156
.

@JCoxwell
Copy link
Author

Hi Diederik,

Checking in on this... I changed the API as you suggested, but I'm not sure
if you've had a chance to review it. I mostly want to make sure you weren't
waiting for me.

I see there is some recent activity on ticket 7623. Should I post my
solution there for other people to try? assign the ticket to myself? Let me
know how to move forward.

Thanks,
Joshua

On Wed, Jan 8, 2014 at 1:26 AM, Diederik van der Boor <
[email protected]> wrote:

This is a feature we'd surely can use. However, I'm not happy about the
API yet, I'd prefer using something like Model.objects.create_...() as
that's how the other ORM API's work.

In one of my projects I've also "upgraded" an object to it's subclass,
using:

b = Boss(person_ptr_id=obj.pk)
b.save_base(raw=True)

See https://code.djangoproject.com/ticket/7623 :)

Do you see a way to incorporate these idea's?

Reply to this email directly or view it on GitHubhttps://github.com//pull/65#issuecomment-31816156
.

@vdboor
Copy link
Collaborator

vdboor commented Apr 3, 2014

Whoops! I haven't taken the time for it, and didn't notice some GitHub notifications.
will check this asap!

@macropin
Copy link

macropin commented Apr 4, 2014

This is in interesting feature, but it seems only useful for the use case where you want to downcast after creation.

What about the common use case where you need to recast a derived class? Currently this doesn't seem to be possible.

Is there any chance to extend this to support this? Perhaps with a recast method.

@JCoxwell
Copy link
Author

JCoxwell commented Apr 4, 2014

Actually there isn't really a problem with that. They're just entries in a
relational table and django mostly does the right thing. You can call the
create_from_super method in it's current form to achieve recasting. The row
in the old class table remains (and is still accessible), so all you need
to do is create some sql to manually delete the row from the old class
table... or not.

Consider:

class ModelA(PolymorphicModel):
field1 = models.CharField(max_length=10)
class ModelB(ModelA):
field2 = models.CharField(max_length=10)
class ModelC(ModelA):
field3 = models.CharField(max_length=10)

mB = ModelB.objects.create(field1='B1',field2='B2')
mC = ModelC.objects.create_from_super(mB.modela_ptr, field3='C3')
m = ModelA.objects.get(pk=mB.pk)
type(m)
<class 'polymorphic.tests.Model2C'>
m.field1
B1
m.field3
C3
m.modelb.field2
B2

Most use cases will probably want to delete the old class data immediately,
but it's interesting that modelb is automatically loaded and accessible.
There may be other consequences to leaving it around, but mainly just need
to test delete and see if there are any issues there.

Joshua

On Thu, Apr 3, 2014 at 8:51 PM, Andrew Cutler [email protected]:

This is in interesting feature, but it seems only useful for the use case
where you want to downcast after creation.

What about the common use case where you need to recast a derived class?
Currently this doesn't seem to be possible.

Is there any chance to extend this to support this?

Reply to this email directly or view it on GitHubhttps://github.com//pull/65#issuecomment-39529564
.

@macropin
Copy link

@JCoxwell thank you for the concise example. I was lead astray by the following exception:

>>> mC = ModelC.objects.create_from_super(mB, field3='C3') Traceback (most recent call last): File "<console>", line 1, in <module> File ".../src/polymorphic/polymorphic/query.py", line 324, in create_from_super raise Exception('create_from_super can only be used if obj is one level of inheritance up from cls') Exception: create_from_super can only be used if obj is one level of inheritance up from cls

Note the omission of modela_ptr in my usage.

In any case I think this usage could be better documented, or perhaps more obvious.

@JCoxwell
Copy link
Author

I honestly hadn't considered the recasting use case until you brought it
up. Maybe it would be worth having a different method name for that use or
even calling it recast... instead of create... .

The exception means that it will only work to cast one level down from an
existing class instance. I'll try to improve the wording. It would be
possible to recursively handle multiple layers. I chose not to implement
that because I'm not really sure about the ramifications for models with
multiple inheritance. It would be easy for the end user to implement that
for specific cases when the hierarchy is known.

Joshua

On Mon, Apr 14, 2014 at 2:28 AM, Andrew Cutler [email protected]:

@JCoxwell https://github.com/JCoxwell thank you for the concise
example. I was lead astray by the following exception:

mC = ModelC.objects.create_from_super(mB, field3='C3')
Traceback (most recent call last):
File "", line 1, in
File ".../src/polymorphic/polymorphic/query.py", line 324, in
create_from_super
raise Exception('create_from_super can only be used if obj is one level of
inheritance up from cls')
Exception: create_from_super can only be used if obj is one level of
inheritance up from cls

Note the omission of modela_ptr in my usage.

In any case I think this usage could be better documented, or perhaps more
obvious.


Reply to this email directly or view it on GitHubhttps://github.com//pull/65#issuecomment-40347608
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants