Opened 16 years ago

Closed 10 years ago

Last modified 9 years ago

#8162 closed Bug (fixed)

Increase Permission.name max_length

Reported by: Julia Owned by: wilsoniya
Component: contrib.auth Version: dev
Severity: Normal Keywords: schemamigration
Cc: shai@…, Simon Charette Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Splitting ticket #6491 to only include the Permissions change max_length from 50 to 100. The changeset to admin ordering interface was ruled out as personal preference.

Attachments (2)

permissions.2.diff (277 bytes ) - added by Julia 16 years ago.
permissions.diff (547 bytes ) - added by Julia 16 years ago.
Changeset for permissions model from 50 to 100 char limit.

Download all attachments as: .zip

Change History (48)

comment:1 by Julia, 16 years ago

Status: newassigned

by Julia, 16 years ago

Attachment: permissions.2.diff added

by Julia, 16 years ago

Attachment: permissions.diff added

Changeset for permissions model from 50 to 100 char limit.

comment:2 by Julia, 16 years ago

Owner: changed from Julia to nobody
Status: assignednew
Triage Stage: UnreviewedDesign decision needed

comment:3 by anonymous, 16 years ago

Component: UncategorizedAuthentication

comment:4 by Jacob, 16 years ago

milestone: 1.0post-1.0
Triage Stage: Design decision neededAccepted

We won't change this for 1.0 because we don't have a good (or even a bad) system in place to handle schema changes to core models. So we'll punt on this until we can figure out the harder problem.

comment:5 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:6 by Chris Beaven, 15 years ago

Triage Stage: AcceptedSomeday/Maybe

comment:7 by anonymous, 13 years ago

milestone: 1.3

comment:8 by Adam Nelson, 13 years ago

milestone: 1.3

This should not be in milestone:1.3 if the Triage stage is someday/maybe.

comment:9 by sixpackistan, 13 years ago

Even though this is a schema change to a core model, it will not affect functionality in any way since you are only making an existing field larger. I continually have to modify the source code of django to work with one of my projects to get around this. The change of a field length to a greater value without a change in name or datatype should not constitute the same amount of rigorous testing and validation that other changes should. I would hope that this change makes it into the next release.

comment:10 by sixpackistan, 13 years ago

Triage Stage: Someday/MaybeDesign decision needed

comment:11 by Karen Tracey, 13 years ago

Triage Stage: Design decision neededSomeday/Maybe

Making an existing field larger in the code without having any way to migrate a database created with the old code level creates a situation where the form-level validation code will accept as valid a string that will not fit in the database field. The database will then promptly throw an exception on the attempt to save the data. (Or if it's sqlite it will just let you do that or if it's MySQL in non-strict mode it will just quietly truncate the value). We cannot grow fields like this until we have some ability to migrate existing DB schemas to match new definitions.

comment:12 by Luke Plant, 13 years ago

Severity: Normal
Type: Bug

in reply to:  11 comment:13 by Kristian Øllegaard, 12 years ago

Easy pickings: unset
UI/UX: unset

Replying to kmtracey:

Making an existing field larger in the code without having any way to migrate a database created with the old code level creates a situation where the form-level validation code will accept as valid a string that will not fit in the database field. The database will then promptly throw an exception on the attempt to save the data. (Or if it's sqlite it will just let you do that or if it's MySQL in non-strict mode it will just quietly truncate the value). We cannot grow fields like this until we have some ability to migrate existing DB schemas to match new definitions.

With the current availability of 3rd party migration apps, wouldn't it be worth considering making the change and including migrations for e.g. south and nashvegas? Could perhaps be implemented as a contrib app, with management commands as helpers? After all, anyone should be able to do this migration manually without too many problems.

comment:14 by anonymous, 12 years ago

We 've come accross this error too many times (postgres and oracle). The "name" column in "auth_permission" should be at least 100 characters long. We always change it in every new installation of django. We 've had absolutely no problems so far and our latest application handles 100s of tables and vast amounts of data.

comment:15 by anonymous, 12 years ago

I'm also hitting this problem, and believe that the name field length should be increased to at least 100. It's an easy fix, and easy to migrate existing deployments - just give it mention in the release notes which good developers read when upgrading versions.

comment:16 by anonymous, 11 years ago

Same here. We need to modify the django code every time we install a fresh instance. I can't believe this bug has not been addressed yet.

comment:17 by wilsoniya, 11 years ago

Has patch: unset
Owner: changed from nobody to wilsoniya
Status: newassigned

comment:18 by Shai Berger, 11 years ago

Hi wilsoniya,

You just closed #17763 as a dupe of this one, ignoring comments 4 & 5 on that ticket which propose a very viable and simple solution. 5 is my own, I'm quoting it here:

My own suggestion of a fix: Change django.contrib.auth.management._get_all_permissions to read:

def _get_all_permissions(opts):
    "Returns (codename, name) for all permissions in the given opts."
    perms = []
    for action in ('add', 'change', 'delete'):
        perm_name = u'Can %s %s' % (action, opts.verbose_name_raw)
        if len(perm_name)>50:
            perm_name= perm_name[:40] + u'...' + perm_name[-7:]
        perms.append((_get_permission_codename(action, opts), perm_name))
    return perms + list(opts.permissions)

Because the name is just used for UI anyway. Of course, you can play with the "40" and "7" there -- that's what I thought was reasonable. And perhaps the two-line snippet for "make display string fit in length" should be factored out and used elsewhere.

If a core developer finds the approach acceptable, but would rather have this as a patch, just say so.

This is fully backwards-compatible in any self-respecting backend (any permission whose name is affected by this could not have been saved in the db).

It may generate a different perm_name for existing perms on sqlite and some mysql configurations (it wouldn't change anything in existing databases, but the same application in a new deployment will have different perm_names). There too -- this is a UI string; for complete Sqlite/MySql backwards-compatibility, use perm_name[:50] instead of perm_name[:40] + u'...' + perm_name[-7:]. If this route is taken, I would prefer to limit this to only these backends, sacrificing a little cross-database compatibility for the sake of more sensible UI elsewhere.

I must say, I find it odd that this suggestion has been so completely ignored, not even rebuked.

comment:19 by Shai Berger, 11 years ago

Cc: shai@… added

comment:20 by michal.dtz@…, 11 years ago

I hit this bug today. Why just not make the fields longer and forget about it?

comment:21 by Aymeric Augustin, 11 years ago

Why not just read the comments before posting a question that's answered above? ;)

(See comment 4.)

comment:22 by Tim Graham, 11 years ago

Keywords: schemamigration added
Summary: Changes to Permissions modelIncrease Permission.name max_length
Triage Stage: Someday/MaybeAccepted

comment:23 by Shai Berger, 10 years ago

#18866 has been fixed. Now, when this is fixed, we need to remember to un-fix it.

comment:24 by Marc Tamlyn, 10 years ago

More accurately, the code from #18866 should stay, but the 39 changes to something bigger. Just in case someone wants a verbose name of 500 characters. (Why???!)

comment:25 by anonymous, 10 years ago

That depends on how we fix this. If, instead of just making the name field ridiculously long, we take the route from comment:18, then we really don't need the code from #18866.

comment:26 by anonymous, 10 years ago

To fix this, we don't need change schema to core models. We can make default setting PERM_NAME_LENGTH=50 and use them in Permissions model name field.

comment:27 by Russell Keith-Magee, 10 years ago

No, a setting isn't a fix for this.

in reply to:  24 comment:28 by tomek, 10 years ago

I see you guys have some problems with both changing this field size and avoiding migrations enforcing. However, don't you think it's a bit crazy that this totally surprising inconsistency of 100 letter verbose_name and 50 letter "can delete " + verbose_name remains totally unresolved for such a long time?

Maybe that's the time to act a bit more definitely?!

(This is not a reply for comment for 24 as label suggests, it's just general comment)

Last edited 10 years ago by tomek (previous) (diff)

comment:29 by Russell Keith-Magee, 10 years ago

@tomek … and yet, in all the time that this critical, mind-numbingly catastrophic bug has been in existence, Django has managed to grow in popularity, and I'm having difficulty recalling the last time the topic was raised on a mailing list for discussion.

There's a very good reason that this has bug has been unresolved for so long - in the grand scheme of things, it isn't a huge problem. Yes, this is a bug. However, it's a bug that only manifests if you have very long table names and/or permission names, and even then, it the problem should be revealed by a fairly cursory testing regimen. There is a known workaround if you have this problem in practice - manually update the table. And we have a proposed fix - we are just waiting on the introduction of migrations. Contrast this with the wide range of known problems that don't have workarounds, or manifest in common, subtle, or untestable ways, or the feature improvements that have been used by *many* more people than have been affected by this issue.

Now that we have migrations, we can look at implementing a formal fix for this, and the other handful of schema-size related issues (e.g., the length of the email field in the default User model). If this really is a problem that you can't bear to look at, now would be a good time to look at contributing a fix.

comment:30 by tomek, 10 years ago

@russelm thanks a lot for your answer, I'm glad to hear migrations might be an impulse to resolve this issue. Although I have no experience in contributing to Django, I will surely consider your suggestion and possibly fix it myself.

Yet short reply to first sentence of your comment:
Popularity of Django is undeniable but proves only general quality of features and has no real relation with particular feature "Have you fixed this wheel? No, but it doesn't matter, car looks great". Insane, huh? Mentioned popularity seems completely irrelevant to the subject of particular bug solving inability I intended to point.

comment:31 by Russell Keith-Magee, 10 years ago

@tomek Continuing your analogy - you're assuming we're talking about a wheel on a car. My interpretation - and my comment - are pointing out that we're *not* talking about a *wheel* on this car.

A wheel is an essential feature of a car, and the car is useless without it. We have ample evidence to suggest that the problem isn't that serious.

What we're talking about the fact that the radiator cap on this car is prone to leakage when you use the car as an endurance racer. *Most* owners won't notice or care about the problem. However, there is a portion of the population that *will* notice, and they are understandably concerned. We acknowledge that this problem exists, but due to our manufacturing process, the problem isn't easy for us to fix. If you're affected by this problem, you can always go down the street to the endurance racing supply depot and get a custom radiator cap that won't have the problem. And when we move to our new factory, we'll be able to fix the problem permanently.

If we were a well funded project with resources to burn, then we might have addressed this a lot sooner. However, as it stands, we're dependent on the contributions of volunteers. And given that only a small proportion of the user base has this problem, and only a small proportion of that user base is willing to contribute a fix, the population of people willing to help is very small. Add the pre-requisites for a fix, and the problem goes unfixed for a long time.

This doesn't make this problem any less of a bug - it just means that you need to factor in the whole picture when thinking about why this specific bug hasn't been fixed.

comment:32 by anonymous, 10 years ago

There is no validation right now for verbose_name less than 39 characters, and after migrating the app with south, it will fail 'after' creating the tables...so you have a database with all the tables, but no permission rows added.
This is REALLY annoying and this bug is 6 years old. I understand there's always something more important to do, but please it's time to fix it.

in reply to:  32 comment:33 by anonymous, 10 years ago

Replying to anonymous:

There is no validation right now for verbose_name less than 39 characters, and after migrating the app with south, it will fail 'after' creating the tables...so you have a database with all the tables, but no permission rows added.
This is REALLY annoying and this bug is 6 years old. I understand there's always something more important to do, but please it's time to fix it.

Agreed.

comment:34 by Tim Graham, 10 years ago

Has patch: set

comment:35 by Aymeric Augustin, 10 years ago

I would go for 200 while we're there. I'm pretty sure someone will complain that 100 isn't enough.

comment:36 by Russell Keith-Magee, 10 years ago

We should be able to be a bit more scientific that "100 or 200 will be big enough". This is driven by identifier sizes, and those are known quantities on all databases. We should be able to work out exactly what the biggest identifier will be.

comment:37 by Tim Graham, 10 years ago

I don't know of CharField limits other than "Any fields that are stored with VARCHAR column types have their max_length restricted to 255 characters if you are using unique=True for the field." which isn't applicable here.

Anyway, 255 would allow verbose name's up to 244 characters and is basically a paragraph of text. I don't think permissions longer than that would be useful.

Objections?

comment:38 by Shai Berger, 10 years ago

In 18 months, I have yet to hear an argument against the solution suggested in comment:18, which is back-portable as far back as we like.

comment:39 by Tim Graham, 10 years ago

If we were starting from scratch, I think I'd choose to make Permission.name a more reasonable length, not truncate it to some arbitrary length.

comment:40 by Aymeric Augustin, 10 years ago

I have an argument ;-) I don't find it a good idea to truncate model names arbitrarily in permission descriptions because it isn't user friendly:

  • users will be left wondering why the full name isn't shown;
  • permissions for two different models could end with the same name after being shortened that way.

comment:41 by Collin Anderson, 10 years ago

Seems to me if we're making the database change, we might as well make it 255. I don't see an advantage to making it shorter.

comment:42 by Simon Charette, 10 years ago

Cc: Simon Charette added

What about using a TextField instead?

This would lift the length limitation and shouldn't have any user noticeable effect since this field is not queried by Django.

Do all backends support altering a CharField to a TextField? If it's not the case we could ship three migrations:

  1. Schema migration that creates the new text field;
  2. Data migration that populates the new field;
  3. Schema migration that removes the old char field and rename the new one name.

comment:43 by Tim Graham, 10 years ago

I don't see a need for TextField (especially given the potential added complexity in migrations). Do you see a need for a verbose_name of more than 244 characters?

in reply to:  43 comment:44 by Simon Charette, 10 years ago

Replying to timo:

I don't see a need for TextField (especially given the potential added complexity in migrations). Do you see a need for a verbose_name of more than 244 characters?

No, it seems like a sane limitation.

comment:45 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In cf252dbea66d9c4a84aa1bc8da93b5e5cc759b9a:

Fixed #8162 -- Increased Permission.name max_length to 255 characters.

comment:46 by Tim Graham <timograham@…>, 9 years ago

In f1783ee479608a8c4ab627d3be1bb9bd07846bb1:

Corrected Permission.max_length in docs; refs #8162.

Note: See TracTickets for help on using tickets.
Back to Top