#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)
Change History (48)
comment:1 by , 16 years ago
Status: | new → assigned |
---|
by , 16 years ago
Attachment: | permissions.2.diff added |
---|
by , 16 years ago
Attachment: | permissions.diff added |
---|
comment:2 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
Triage Stage: | Unreviewed → Design decision needed |
comment:3 by , 16 years ago
Component: | Uncategorized → Authentication |
---|
comment:4 by , 16 years ago
milestone: | 1.0 → post-1.0 |
---|---|
Triage Stage: | Design decision needed → 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:6 by , 15 years ago
Triage Stage: | Accepted → Someday/Maybe |
---|
comment:7 by , 14 years ago
milestone: | → 1.3 |
---|
comment:8 by , 14 years ago
milestone: | 1.3 |
---|
This should not be in milestone:1.3 if the Triage stage is someday/maybe.
comment:9 by , 14 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 , 14 years ago
Triage Stage: | Someday/Maybe → Design decision needed |
---|
follow-up: 13 comment:11 by , 14 years ago
Triage Stage: | Design decision needed → 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 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:13 by , 13 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 , 13 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 , 13 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 , 12 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 , 12 years ago
Has patch: | unset |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:18 by , 12 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 , 12 years ago
Cc: | added |
---|
comment:20 by , 11 years ago
I hit this bug today. Why just not make the fields longer and forget about it?
comment:21 by , 11 years ago
Why not just read the comments before posting a question that's answered above? ;)
(See comment 4.)
comment:22 by , 11 years ago
Keywords: | schemamigration added |
---|---|
Summary: | Changes to Permissions model → Increase Permission.name max_length |
Triage Stage: | Someday/Maybe → Accepted |
comment:23 by , 11 years ago
#18866 has been fixed. Now, when this is fixed, we need to remember to un-fix it.
follow-up: 28 comment:24 by , 11 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 , 11 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 , 11 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:28 by , 11 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?!
comment:29 by , 11 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 , 11 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 , 11 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.
follow-up: 33 comment:32 by , 11 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.
comment:33 by , 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:35 by , 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 , 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 , 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 , 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 , 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 , 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 , 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 , 10 years ago
Cc: | 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:
- Schema migration that creates the new text field;
- Data migration that populates the new field;
- Schema migration that removes the old char field and rename the new one
name
.
follow-up: 44 comment:43 by , 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?
comment:44 by , 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 averbose_name
of more than 244 characters?
No, it seems like a sane limitation.
comment:45 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Changeset for permissions model from 50 to 100 char limit.