Status of issue 17758: dict ordering bugs

[Łukasz Rekucki]

Hi,

I got a sad message today, that it's most likely that issue #17758
won't go in to 1.4; While most of the problems addressed by this
ticket are harmless, I really don't think it's a good idea to ship 1.4
without resolving the ORM bug. The hash randomization is a security
fix. Right now, people upgrading to Python 2.6.8 or 2.7.3 will start
randomly getting different results on some queries.

If the whole patch can't be merged, lets at least fix that bug[2]. Is
there any work I can do to make it happen?

[1]: https://code.djangoproject.com/ticket/17758
[2]: 
https://github.com/lqc/django/commit/84dc450ec861e34de068fde891537f0481342ef7

-- 
Łukasz Rekucki

-- 
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: Status of issue 17758: dict ordering bugs

[Ramiro Morales]

2012/3/22 Łukasz Rekucki :
> Hi,
>
> I got a sad message today, that it's most likely that issue #17758
> won't go in to 1.4; While most of the problems addressed by this
> ticket are harmless, I really don't think it's a good idea to ship 1.4
> without resolving the ORM bug. The hash randomization is a security
> fix. Right now, people upgrading to Python 2.6.8 or 2.7.3 will start
> randomly getting different results on some queries.
>
> If the whole patch can't be merged, lets at least fix that bug[2]. Is
> there any work I can do to make it happen?
>
> [1]: https://code.djangoproject.com/ticket/17758
> [2]: 
> https://github.com/lqc/django/commit/84dc450ec861e34de068fde891537f0481342ef7

I think part of the problem is that the ticket conflates two (three? more?)
different problem reports under one title ("Do not depend on dict
order in test suite")

The title itself leads to think the issue is something of a lesser
severity/priority
than the other issues.

Maybe we should split these problem reports in their own tickets?

Regards,

-- 
Ramiro Morales

-- 
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: auth.User: The abstract base class idea

[bhuztez]

Maybe we could make every model pluggable. so User model could be
defined like this:

class User(models.Model):
__mixins__ = load_available_mixins(settings.AUTH_MODEL_MIXINS)


But AFAICS, Django will suffer from the same issue as PHP
register_globals / Rails mass-assignment, because of ModelForm. and
Django must provide schema migration tools.




On 3月22日, 上午10时33分, Alex Ogier  wrote:
> I made a topic branch and refactored everything I thought was nicely
> reusable from auth.User into abstract models in
> django/contrib/auth/mixins.py. There are some good reusable pieces inside
> auth.User, even if you want to entirely scrap Django's notion of identity
> and the username and/or email fields. The change is transparent to Django's
> test suite, and I did my best to leave the existing API identical.
>
> My hope is that we can make, for example, contrib.admin only dependent on a
> class implementing PermissionsMixin. Whether we do late binding to
> auth.User with some jiggery-poker as described in the recent auth.User
> reboot thread, load time plugging into the auth.User inheritance list as I
> proposed in the first email in this thread, or just punt on the whole thing
> and ask people to implement their own concrete classes, I think the
> refactoring is a nice backwards-compatible way to expose pieces of
> auth.User for people to reuse and/or depend on.
>
> https://github.com/ogier/django/tree/auth-mixins
>
> Does anyone have any opinions?
>
> -Alex Ogier

-- 
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: Status of issue 17758: dict ordering bugs

[Łukasz Rekucki]

On 22 March 2012 13:38, Ramiro Morales  wrote:
> 2012/3/22 Łukasz Rekucki :
>
> Maybe we should split these problem reports in their own tickets?
>

I can do it later today. There probably should be 3 tickets: the ORM
bug, the dependency_order() bug and the test suite fixes (which can be
split further into HTML, XML, JSON and urlencode() related stuff if
needed).

-- 
Łukasz Rekucki

-- 
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: Better error message for missing trailing comma in ModelForm fields attribute

[Andre Terra]

I have a feeling that this is too much to ask from the framework, after all
it shouldn't have to teach python, so I'm -0 on the proposed change.

Additionally, if users specify fields = 'body' with no parenthesis they
would get the same error message and probably think "but I don't need a
comma, it's just one field" or even write fields = 'body', which is just
horrible..

Cheers,
AT
On Mar 22, 2012 10:44 AM, "Sachin Gupta"  wrote:

> I have the following ModelForm, that has a valid field body.
>
> class PostForm(ModelForm):
>
> class Meta:
> model = Post
> fields = ('body')
>
> However when I validate the models, the following error comes up
>
> django.core.exceptions.**FieldError: Unknown field(s) (y, b, d, o)
> specified for Post
>
> This is because the Django expects a 'list' in the fields attribute. The
> error can be solved by adding a trailing comma
>
> fields = ('body',)
>
> This problem is quite common, as people tend to forget the trailing
> comma. I have written a small hack for this problem, that throws the
> following error message if the trailing comma is missing
>
> django.core.exceptions.**FieldError: Missing trailing comma in fields
> attribute
>
> This error message is more intuitive than the one that is thrown currently
>
> Editions:
>
> In django/forms/models.py
>
> class ModelFormOptions(object):
> def __init__(self, options=None):
> self.model = getattr(options, 'model', None)
> self.fields = getattr(options, 'fields', None)
> self.exclude = getattr(options, 'exclude', None)
> self.widgets = getattr(options, 'widgets', None)
>
> if isinstance(self.fields,str):
> message = 'Missing trailing comma in fields attribute'
> raise FieldError(message)
>
> if isinstance(self.exclude,str):
> message = 'Missing trailing comma in exclude attribute'
> raise FieldError(message)
>
> I am raising a FieldError but a TypeError could also be raised.
>
> Regards
> Sachin Gupta
>
>
>
>
>
>
>  --
> You received this message because you are subscribed to the Google Groups
> "Django developers" group.
> To view this discussion on the web visit
> https://groups.google.com/d/msg/django-developers/-/biHIzxoCrD0J.
> 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.
>

-- 
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: Status of issue 17758: dict ordering bugs

[Aymeric Augustin]

Le 22 mars 2012 13:22, Łukasz Rekucki  a écrit :
> If the whole patch can't be merged, lets at least fix that bug[2]. Is
> there any work I can do to make it happen?
> [2]: 
> https://github.com/lqc/django/commit/84dc450ec861e34de068fde891537f0481342ef7

Hi Łukasz,

Let's try to get this fix in 1.4. Since the change in the tests isn't
significant, it boils down to replacing two `dict`s which
`SortedDict`s. Since a `SortedDict` is a subclass of `dict`, this
change looks safe.

In a comment [1], you said:

> there are a few places that relabel aliases and ruin the order, like 
> QuerySet.change_aliases(). (...) I fixed this, by using change_map = 
> SortedDict() in QuerySet.split_exclude. This should probably apply to all 
> change_maps when working with aliases.

This implies that your fix isn't complete, is it? Could you clarify?

Thanks!


[1] https://code.djangoproject.com/ticket/17758#comment:2

-- 
Aymeric.

-- 
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: auth.User: The abstract base class idea

[Mateusz Marzantowicz]

On Thu, Mar 22, 2012 at 3:33 AM, Alex Ogier  wrote:

> I made a topic branch and refactored everything I thought was nicely
> reusable from auth.User into abstract models in
> django/contrib/auth/mixins.py. There are some good reusable pieces inside
> auth.User, even if you want to entirely scrap Django's notion of identity
> and the username and/or email fields. The change is transparent to Django's
> test suite, and I did my best to leave the existing API identical.
>
> My hope is that we can make, for example, contrib.admin only dependent on
> a class implementing PermissionsMixin. Whether we do late binding to
> auth.User with some jiggery-poker as described in the recent auth.User
> reboot thread, load time plugging into the auth.User inheritance list as I
> proposed in the first email in this thread, or just punt on the whole thing
> and ask people to implement their own concrete classes, I think the
> refactoring is a nice backwards-compatible way to expose pieces of
> auth.User for people to reuse and/or depend on.
>
> https://github.com/ogier/django/tree/auth-mixins
>
> Does anyone have any opinions?
>
> -Alex Ogier
>
>
> It looks OK, but there is one really big issue with your approach -
database blowup.
You can easily choose mixins to compose your User model, but once it's done
your database layout is fixed.
If you then decide to use different mixins - it would be impossible without
very smart db schema migration tool.


Mateusz Marzantowicz

-- 
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: auth.User: The abstract base class idea

[Alex Ogier]

>
> It looks OK, but there is one really big issue with your approach -
> database blowup.
> You can easily choose mixins to compose your User model, but once it's
> done your database layout is fixed.
> If you then decide to use different mixins - it would be impossible
> without very smart db schema migration tool.
>

Any schema migration tool that reuses django's ORM *should* be OK. For
example, South works just fine. Adding or removing a mixin is exactly like
adding or removing the fields in that mixin as far as the ORM is concerned.
If we end up patching the User model in-place, then the tool will have to
be able to operate on models in contrib apps which might be in read-only
locations or shared locations, but this has been on the radar of most
migration tools for a while already. For example, South has the
SOUTH_MIGRATION_MODULESsetting
to handle this.

-Alex Ogier

-- 
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: Status of issue 17758: dict ordering bugs

[Łukasz Rekucki]

On 22 March 2012 16:54, Aymeric Augustin
 wrote:
> Le 22 mars 2012 13:22, Łukasz Rekucki  a écrit :
>> If the whole patch can't be merged, lets at least fix that bug[2]. Is
>> there any work I can do to make it happen?
>> [2]: 
>> https://github.com/lqc/django/commit/84dc450ec861e34de068fde891537f0481342ef7
>
> Hi Łukasz,
>
> Let's try to get this fix in 1.4. Since the change in the tests isn't
> significant, it boils down to replacing two `dict`s which
> `SortedDict`s. Since a `SortedDict` is a subclass of `dict`, this
> change looks safe.
>
> In a comment [1], you said:
>
>> there are a few places that relabel aliases and ruin the order, like 
>> QuerySet.change_aliases(). (...) I fixed this, by using change_map = 
>> SortedDict() in QuerySet.split_exclude. This should probably apply to all 
>> change_maps when working with aliases.
>
> This implies that your fix isn't complete, is it? Could you clarify?
>

Yes, sorry. It's complete in the sense, that it fixes this bug and I
haven't been able to spot any other bugs by reading the code or doing
tests. There is at least one other place that creates a "change_map"
variable (or something similar), that is used to relabel aliases in a
Q object (AFAIR), but it never uses it in a way that could be affected
by ordering.

My other concern is other similar structures (like alias_map or
included_inherited_models). While I failed to find any dangerous uses,
I'm not (yet, I hope) very comfortable with the ORM code, so I
wouldn't trust myself on that 100%. OTOH, I didn't want to blindly
change all the dicts to SortedDict.

-- 
Łukasz Rekucki

-- 
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.

Contrib backwards-compatibility question

[Alex Ogier]

Is it considered a backwards incompatibility to import django.conf.settings
at the top level in a contrib models.py file (specifically, contrib.admin)?
I know it's possible to do so, for example contrib.comments.models imports
it, but it could cause circular dependencies for any project that in turn
imports django.contrib.admin.models in their settings file. It's not a
common situation, probably, and it's an unfortunate restriction for many
reasons. So if I were to propose a patch that did so, would it get shut
down for that reason?

-Alex Ogier

-- 
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: Contrib backwards-compatibility question

[Carl Meyer]

Hi Alex,

On 03/22/2012 11:26 AM, Alex Ogier wrote:
> Is it considered a backwards incompatibility to import
> django.conf.settings at the top level in a contrib models.py file
> (specifically, contrib.admin)? I know it's possible to do so, for
> example contrib.comments.models imports it, but it could cause circular
> dependencies for any project that in turn imports
> django.contrib.admin.models in their settings file. It's not a common
> situation, probably, and it's an unfortunate restriction for many
> reasons. So if I were to propose a patch that did so, would it get shut
> down for that reason?

No, I'm pretty sure importing django.db.models (which any models.py file
is bound to do, even if indirectly) already imports settings, so this
should have no impact. There are areas where we try to avoid
module-level imports of settings if possible, but models.py files aren't
one of them.

In any case, importing anything from Django or your project in your
settings.py file is strongly discouraged, precisely because it tends to
lead to circular import problems.

Carl



signature.asc
Description: OpenPGP digital signature

Re: Status of issue 17758: dict ordering bugs

[Łukasz Rekucki]

Hi,

Thanks Aymeric for committing the ORM related fix. As Ramiro
suggested, I created a separate ticket for the bug in
dependency_order() function - #17954. While it's always nice to have
more bugfixes, it doesn't qualify as a release blocker anymore.
Looking forward to 1.4 final soon :)

-- 
Łukasz Rekucki

-- 
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: auth.User refactor: reboot

[Russell Keith-Magee]

On 17/03/2012, at 12:53 AM, Jacob Kaplan-Moss wrote:

> I think we need to come together and agree on an approach before we move 
> forward, so I'd like to see some concrete proposals for each of these 
> approaches. Since all options have merits and since I think it's unlikely 
> we'll find consensus I'm prepared to make a BDFL ruling here. So if you feel 
> strongly about one approach or another, please write a concrete proposal and 
> post it here or on the wiki. I'll look these over -- and also review Clay's 
> branch -- and (again, baring consensus) make a ruling next week.

To help this along, I've just summarized the state of this discussion on the 
wiki. 

https://code.djangoproject.com/wiki/ContribAuthImprovements

I've tried to keep my comments as objective as possible; if anyone feels I've 
misrepresented something, or missed an important point, feel free to weigh in 
with modifications. However, *please*: 

 * Keep it brief.
 * Keep it on topic.
 * Keep it objective. 

I'll be doing some weeding and pruning to make sure the document remains a 
useful historical summary of our design discussion (just like the similar 
documents we have for class-based views and contrib.messages). This means that 
any length, off-topic, or non-objective comments will be cut -- or at least 
heavily edited.

Yours,
Russ Magee %-)

-- 
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: auth.User refactor: reboot

[Alex Ogier]

On Thu, Mar 22, 2012 at 8:14 PM, Russell Keith-Magee <
russ...@keith-magee.com> wrote:

>
> On 17/03/2012, at 12:53 AM, Jacob Kaplan-Moss wrote:
>
> > I think we need to come together and agree on an approach before we move
> forward, so I'd like to see some concrete proposals for each of these
> approaches. Since all options have merits and since I think it's unlikely
> we'll find consensus I'm prepared to make a BDFL ruling here. So if you
> feel strongly about one approach or another, please write a concrete
> proposal and post it here or on the wiki. I'll look these over -- and also
> review Clay's branch -- and (again, baring consensus) make a ruling next
> week.
>
> To help this along, I've just summarized the state of this discussion on
> the wiki.
>
> https://code.djangoproject.com/wiki/ContribAuthImprovements
>
> I've tried to keep my comments as objective as possible; if anyone feels
> I've misrepresented something, or missed an important point, feel free to
> weigh in with modifications. However, *please*:
>
>  * Keep it brief.
>  * Keep it on topic.
>  * Keep it objective.
>
> I'll be doing some weeding and pruning to make sure the document remains a
> useful historical summary of our design discussion (just like the similar
> documents we have for class-based views and contrib.messages). This means
> that any length, off-topic, or non-objective comments will be cut -- or at
> least heavily edited.
>
> Yours,
> Russ Magee %-)


I hope you don't mind, I added solution 2a. It's basically solution 2 minus
the monkey-patching and resultant circular dependency issues, and
correspondingly requires apps to opt-in to supporting pluggable Users. It
documents the reasoning behind the changes in
https://github.com/ogier/django/tree/auth-mixins

-Alex Ogier

-- 
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: auth.User refactor: reboot

[Russell Keith-Magee]

On 23/03/2012, at 9:50 AM, Alex Ogier wrote:

> I hope you don't mind, I added solution 2a. It's basically solution 2 minus 
> the monkey-patching and resultant circular dependency issues, and 
> correspondingly requires apps to opt-in to supporting pluggable Users. It 
> documents the reasoning behind the changes in 
> https://github.com/ogier/django/tree/auth-mixins

... and I hope you don't mind, I've just corrected some of the assertions you 
made:

 * This solution still has the circular dependency, because any app with a 
ForeignKey(settings.USER_MODEL) needs to have a settings import in the 
models.py file

 * It isn't a transparent migration for any third party app -- they need to be 
updated to use ForeignKey(settings.USER_MODEL), not ForeignKey(User). However, 
the failure modes will be interesting, because the auth_user table will still 
exist -- so apps will have a legal FK to a table that will be empty.

Yours,
Russ Magee %-)

-- 
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: auth.User refactor: reboot

[Alex Ogier]

On Mar 22, 2012 10:55 PM, "Russell Keith-Magee" 
wrote:
>  * This solution still has the circular dependency, because any app with
a ForeignKey(settings.USER_MODEL) needs to have a settings import in the
models.py file

Hmmm. I asked a question about that today and was told that importing
django.db.models already meant implicitly loading settings, and so
importing settings in a models.py would be transparent.
django.contrib.comments uses the same import, though admittedly circularly
importing django.contrib.comments.models from a settings file is far less
likely.

>  * It isn't a transparent migration for any third party app -- they need
to be updated to use ForeignKey(settings.USER_MODEL), not ForeignKey(User).
However, the failure modes will be interesting, because the auth_user table
will still exist -- so apps will have a legal FK to a table that will be
empty.

This is true. The main difficulty is that separating auth.User from
auth.Group and auth.Permission is difficult and so removing
django.contrib.auth from INSTALLED_APPS is currently impossible if you want
permissions for contrib.admin, even if you have no need for auth.User.
There are ways around this particular problem, I think. For example, fail
to resolve foreign keys to User unless it is the current USER_MODEL, or
don't even define the model at all. Both of these solutions are sort of
ugly, but they do make failure modes more aggressively fail fast.
Definitely worth pointing out in the wiki as a limitation.

Alex Ogier

-- 
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: auth.User refactor: reboot

[Alex Ogier]

Also, even if we decide to use django-primate's monkey patch to mount a
user model on django.contrib.auth.models.User I think we should still
consider breaking AbstractUser into orthogonal mixins. As it stands,
AbstractUser is a monolithic model with an ad-hoc method for overriding
fields. As a result, it is difficult to reason about what effect modifying
a particular field will have.

Is it valid to rip out all of the personal info and authentication
mechanisms and just use twitter handles with contrib.admin? If
contrib.admin only depends on a permissions mixin then sure. Otherwise each
field you remove is an unknown risk.

Alex Ogier

-- 
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.