Opened 7 years ago

Closed 12 months ago

Last modified 8 months ago

#8162 closed Bug (fixed)

Increase Permission.name max_length

Reported by: juliae Owned by: wilsoniya
Component: contrib.auth Version: master
Severity: Normal Keywords: schemamigration
Cc: shai@…, charettes 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 juliae 7 years ago.
permissions.diff (547 bytes) - added by juliae 7 years ago.
Changeset for permissions model from 50 to 100 char limit.

Download all attachments as: .zip

Change History (48)

comment:1 Changed 7 years ago by juliae

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Status changed from new to assigned

Changed 7 years ago by juliae

Changed 7 years ago by juliae

Changeset for permissions model from 50 to 100 char limit.

comment:2 Changed 7 years ago by juliae

  • Owner changed from juliae to nobody
  • Status changed from assigned to new
  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 7 years ago by anonymous

  • Component changed from Uncategorized to Authentication

comment:4 Changed 7 years ago by jacob

  • milestone changed from 1.0 to post-1.0
  • Triage Stage changed from Design decision needed to Accepted

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 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:6 Changed 6 years ago by SmileyChris

  • Triage Stage changed from Accepted to Someday/Maybe

comment:7 Changed 5 years ago by anonymous

  • milestone set to 1.3

comment:8 Changed 5 years ago by adamnelson

  • milestone 1.3 deleted

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

comment:9 Changed 4 years ago by sixpackistan

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 Changed 4 years ago by sixpackistan

  • Triage Stage changed from Someday/Maybe to Design decision needed

comment:11 follow-up: Changed 4 years ago by kmtracey

  • Triage Stage changed from Design decision needed to Someday/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 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:13 in reply to: ↑ 11 Changed 4 years ago by KristianDK

  • 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 Changed 3 years ago by anonymous

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 Changed 3 years ago by anonymous

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 Changed 3 years ago by anonymous

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 Changed 2 years ago by wilsoniya

  • Has patch unset
  • Owner changed from nobody to wilsoniya
  • Status changed from new to assigned

comment:18 Changed 2 years ago by shai

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 Changed 2 years ago by shai

  • Cc shai@… added

comment:20 Changed 2 years ago by michal.dtz@…

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

comment:21 Changed 2 years ago by aaugustin

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

(See comment 4.)

comment:22 Changed 23 months ago by timo

  • Keywords schemamigration added
  • Summary changed from Changes to Permissions model to Increase Permission.name max_length
  • Triage Stage changed from Someday/Maybe to Accepted

comment:23 Changed 21 months ago by shai

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

comment:24 follow-up: Changed 21 months ago by mjtamlyn

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 Changed 21 months ago by anonymous

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 Changed 16 months ago by anonymous

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 Changed 16 months ago by russellm

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

comment:28 in reply to: ↑ 24 Changed 14 months ago by tomek

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 14 months ago by tomek (previous) (diff)

comment:29 Changed 14 months ago by russellm

@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 Changed 14 months ago by tomek

@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 Changed 14 months ago by russellm

@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 follow-up: Changed 14 months ago by 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.

comment:33 in reply to: ↑ 32 Changed 12 months ago by anonymous

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 Changed 12 months ago by timo

  • Has patch set

comment:35 Changed 12 months ago by aaugustin

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

comment:36 Changed 12 months ago by russellm

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 Changed 12 months ago by timo

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 Changed 12 months ago by shai

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 Changed 12 months ago by timo

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 Changed 12 months ago by aaugustin

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 Changed 12 months ago by CollinAnderson

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 Changed 12 months ago by charettes

  • Cc charettes 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 follow-up: Changed 12 months ago by 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?

comment:44 in reply to: ↑ 43 Changed 12 months ago by charettes

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 Changed 12 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In cf252dbea66d9c4a84aa1bc8da93b5e5cc759b9a:

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

comment:46 Changed 8 months ago by Tim Graham <timograham@…>

In f1783ee479608a8c4ab627d3be1bb9bd07846bb1:

Corrected Permission.max_length in docs; refs #8162.

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