Re: Single table inheritance - working implementation

[Krzysztof Jurewicz]

Dnia 2012-12-21, pią o godzinie 11:05 -0800, Anssi Kääriäinen pisze:
> I went with a different approach than the patch for 1.5. At this stage
> as minimal as possible change to get_default_columns() seemed like a
> good idea. See commit a0155f35343afbfd9e98ab9aa4615f06780f697e in
> stable/1.5.x.
> 
> I patched only 1.5.x. Master will get soon different handling of the
> parent model joining in get_default_columns(). The taken approach in
> master should still allow typedmodels to work. See:
> https://github.com/akaariai/django/compare/names_to_path_to_fields#L3L254
> 
> If you have still problems in 1.5.x please tell us soon. There isn't
> too much time before the release.

Thanks for the patch, it seems to do the job - all tests pass (including
two I added today), both on 1.5.x and on 1.4.x. _fill_fields_cache still
has to be monkeypatched (I've also added similar patch for
_fill_m2m_cache), but this is no change comparing to the previous state.

I haven't yet looked through changes from your names_to_path_to_fields
branch, but tests pass on it too, which is promising.

Regards,
Krzysztof

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Single table inheritance - working implementation

[Anssi Kääriäinen]

On 21 joulu, 16:39, Krzysztof Jurewicz 
wrote:
> On 13.12.2012 15:54, Anssi K ri inen wrote:
>
> > Second, I have created a patch which should allow your work to
> > continue working in master. See
> >https://github.com/akaariai/django/commit/94c417d2a29a0f72b26019fc38e...
> > - the idea is to change get_fields_with_model() so that it doesn't
> > return different values for proxy models compared to concrete models.
> > I think this is a good solution overall, though this change is
> > somewhat scary - get_fields_with_model is used in many places of the
> > ORM...
>
> Thanks for the patch. It causes all typedmodels' tests to pass, however
> I still have to monkey patch _fill_fields_cache to bypass some currently
> untested issues. I will investigate this further and hopefully provide
> some more detailed comments.

I went with a different approach than the patch for 1.5. At this stage
as minimal as possible change to get_default_columns() seemed like a
good idea. See commit a0155f35343afbfd9e98ab9aa4615f06780f697e in
stable/1.5.x.

I patched only 1.5.x. Master will get soon different handling of the
parent model joining in get_default_columns(). The taken approach in
master should still allow typedmodels to work. See:
https://github.com/akaariai/django/compare/names_to_path_to_fields#L3L254

If you have still problems in 1.5.x please tell us soon. There isn't
too much time before the release.

 - Anssi

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Single table inheritance - working implementation

[Krzysztof Jurewicz]

On 13.12.2012 15:54, Anssi Kääriäinen wrote:

Second, I have created a patch which should allow your work to
continue working in master. See
https://github.com/akaariai/django/commit/94c417d2a29a0f72b26019fc38ef400420097aa4
- the idea is to change get_fields_with_model() so that it doesn't
return different values for proxy models compared to concrete models.
I think this is a good solution overall, though this change is
somewhat scary - get_fields_with_model is used in many places of the
ORM...


Thanks for the patch. It causes all typedmodels' tests to pass, however 
I still have to monkey patch _fill_fields_cache to bypass some currently 
untested issues. I will investigate this further and hopefully provide 
some more detailed comments.


Regards,
Krzysztof

--
You received this message because you are subscribed to the Google Groups "Django 
developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Single table inheritance - working implementation

[Anssi Kääriäinen]

On 11 joulu, 17:56, Anssi Kääriäinen  wrote:
> Would a model._meta.get_default_columns() work for you? While this
> would still be internal API, it would be hopefully less likely to
> change in ways that require extensive rewrites to 3rd party apps.

First, the ._meta.get_default_columns() I proposed above doesn't
actually make sense - the get_default_columns() does mostly things
which belong to compiler, not to ._meta.

Second, I have created a patch which should allow your work to
continue working in master. See
https://github.com/akaariai/django/commit/94c417d2a29a0f72b26019fc38ef400420097aa4
- the idea is to change get_fields_with_model() so that it doesn't
return different values for proxy models compared to concrete models.
I think this is a good solution overall, though this change is
somewhat scary - get_fields_with_model is used in many places of the
ORM...

Third, I think it is a good idea to add more abstraction between the
ORM and the Model/Fields layer. So, in this way I do support the idea
of using proxy models for single table inheritance. On the other hand
it should not be surprising if in the future "different fields than
parents" proxy models will break again - it is almost the definition
of proxy model that it has the same fields than its parent.

 - Anssi

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Single table inheritance - working implementation

[Anssi Kääriäinen]

On 7 joulu, 16:18, Krzysztof Jurewicz 
wrote:
> On 29.10.2012 14:34, Krzysztof Jurewicz wrote:
>
> > In February, Craig de Stigter released django-typed-models
> > ,
> > a package implementing inheritance hierarchy for Proxy models with
> > automatic type recasting. I've written a fork of it with additional
> > possibility of adding fields to subclasses, therefore achieving a single
> > table inheritance implementation similar to Ruby on Rails' STI.
>
> Refreshing this thread because it looks like my fork stopped working
> properly with recent Django master. I suspect that commit c2e1e by Anssi
> K ri inen should be blamed for this, especially the line
>
> # Skip all proxy to the root proxied model
> opts = opts.concrete_model._meta
>
> in get_default_columns method. This causes it to generate potentially
> invalid set of columns for instantiated model because children typed
> models are implemented as proxy models and they have different set of
> fields than the root model, which stores all fields from its subclasses.
> In general having richer set of columns than required wouldn't be a
> problem, but those columns are used as positional arguments in model
> constructor and are therefore mapped incorrectly.
>
> I know that proxy models are intended to have the same set of fields as
> the root model and that I was relying on a completely internal
> implementation detail, so it has no chance of being treated as a bug,
> but that being said, I wonder if maybe Django could be changed to make
> such modifications easier. Currently I see the following solutions to
> the encountered problem:
>
> 1. Do not touch Django code directly and adjust django-typed-models to
> the changes made. This would probably require writing custom Manager
> class for TypedModel with custom QuerySet class which will use custom
> Query class with custom SQLCompiler class which will finally have
> get_default_columns method copy-paste-changed. Not an elegant solution.
>
> 2. Take a completely different approach to achieve single table
> inheritance ideas are welcome.
>
> 3. Modify Django code to be more friendly to django-typed-models.
> Current behavior of SQLCompiler looks a bit hackish to me, because in
> ideal world get_default_columns shouldn't care if a model is proxy or
> not this should be handled by model class internally. So perhaps
> another layer of abstraction could be added to Django ORM to allow
> internal handling of such special cases as proxy models. This would
> ideally allow to specify STI as a completely different model type than
> proxy model.
>
> 4. Add single table inheritance as a new inheritance type in Django.
>
> I'm not very familiar with Django ORM internals and additionally I don't
> know which of the above solutions have chances of getting approval by
> core devs, so feedback is welcome.

Would a model._meta.get_default_columns() work for you? While this
would still be internal API, it would be hopefully less likely to
change in ways that require extensive rewrites to 3rd party apps.

 - Anssi

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Single table inheritance - working implementation

[Krzysztof Jurewicz]

On 29.10.2012 14:34, Krzysztof Jurewicz wrote:

In February, Craig de Stigter released django-typed-models
,
a package implementing inheritance hierarchy for Proxy models with
automatic type recasting. I've written a fork of it with additional
possibility of adding fields to subclasses, therefore achieving a single
table inheritance implementation similar to Ruby on Rails' STI.


Refreshing this thread because it looks like my fork stopped working 
properly with recent Django master. I suspect that commit c2e1e by Anssi 
Kääriäinen should be blamed for this, especially the line


# Skip all proxy to the root proxied model
opts = opts.concrete_model._meta

in get_default_columns method. This causes it to generate potentially 
invalid set of columns for instantiated model because children typed 
models are implemented as proxy models and they have different set of 
fields than the root model, which stores all fields from its subclasses. 
In general having richer set of columns than required wouldn't be a 
problem, but those columns are used as positional arguments in model 
constructor and are therefore mapped incorrectly.


I know that proxy models are intended to have the same set of fields as 
the root model and that I was relying on a completely internal 
implementation detail, so it has no chance of being treated as a bug, 
but that being said, I wonder if maybe Django could be changed to make 
such modifications easier. Currently I see the following solutions to 
the encountered problem:


1. Do not touch Django code directly and adjust django-typed-models to 
the changes made. This would probably require writing custom Manager 
class for TypedModel with custom QuerySet class which will use custom 
Query class with custom SQLCompiler class which will finally have 
get_default_columns method copy-paste-changed. Not an elegant solution.


2. Take a completely different approach to achieve single table 
inheritance — ideas are welcome.


3. Modify Django code to be more friendly to django-typed-models. 
Current behavior of SQLCompiler looks a bit hackish to me, because in 
ideal world get_default_columns shouldn't care if a model is proxy or 
not — this should be handled by model class internally. So perhaps 
another layer of abstraction could be added to Django ORM to allow 
internal handling of such special cases as proxy models. This would 
ideally allow to specify STI as a completely different model type than 
proxy model.


4. Add single table inheritance as a new inheritance type in Django.

I'm not very familiar with Django ORM internals and additionally I don't 
know which of the above solutions have chances of getting approval by 
core devs, so feedback is welcome.


Regards,
Krzysztof

--
You received this message because you are subscribed to the Google Groups "Django 
developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: Single table inheritance - working implementation

[Craig de Stigter]

Hi Krzysztof and everyone else

Thanks! Your fork looks very promising. I'll review the fork today and
possibly pull changes in if the changes aren't too disruptive. Some more
tests would probably be good.

I've added a New BSD
License
to
the repo to clear up any confusion.

Would other devs be interested in adding something like django-typed-models
to Django core? What kinds of changes would make that more likely?

Cheers
Craig de Stigter

On Tue, Oct 30, 2012 at 2:34 AM, Krzysztof Jurewicz <
krzysztof.jurew...@gmail.com> wrote:

> Hi all,
>
> In February, Craig de Stigter released 
> django-typed-models,
> a package implementing inheritance hierarchy for Proxy models with
> automatic type recasting. I've written a fork of it with additional
> possibility of adding fields to subclasses, therefore achieving a single
> table inheritance implementation similar to Ruby on Rails' STI. This is
> possible mostly thanks to metaclass magic and monkey patching.
>
> The fork's homepage is at https://github.com/KrzysiekJ/django-typed-models. 
> Note that it should be considered early alpha and there is much room for
> optimisation, code cleanup and maybe additional features. Feel free to fork
> and/or add issues.
>
> I don't know yet what to do with licensing since Craig didn't specify any
> license in the original package. Craig, please comment or send me an email.
>
> Regards,
> Krzysztof Jurewicz
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.