Opened 5 years ago

Closed 13 months ago

#13711 closed Bug (fixed)

Very long field names on models cause problems

Reported by: russellm Owned by: anubhav9042
Component: Database layer (models, ORM) Version: master
Severity: Release blocker Keywords:
Cc: shai Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Under MySQL, you get

_mysql_exceptions.OperationalError: (1059, "Identifier name 'verylongmodelnamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz_id' is too long")

raised as an error during runtime.

Under Postgres, you get

NOTICE:  identifier "verylongmodelnamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz_id" will be truncated to "verylongmodelnamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz"

appear silently in the logs.

This problem (and the solution) is analogous to #13528 for db_table.

Change History (26)

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by russellm

For the record - the patch for #8901 in [13328] contains a model that reveals this problem for MySQL. Removing the anti-MySQL guard for that test would be one part of the fix for this problem.

comment:3 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:4 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:5 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:6 Changed 2 years ago by vernondcole@…

This behavior is also seen in MS SQL Server, so MySQL is not alone in limiting the length of table names.

Furthermore, the test workaround in regressiontests/backends/models.py saying:

if connection.features.supports_long_model_names:

only works if the _default_ database does not support long names. For example, if 'default' is SQLite and 'other' is MySQL, the testing will blow up.

comment:7 Changed 19 months ago by anubhav9042

  • Owner changed from nobody to anubhav9042
  • Status changed from new to assigned

comment:9 Changed 18 months ago by anubhav9042

comment:10 Changed 16 months ago by anubhav9042

  • Has patch set

comment:11 Changed 16 months ago by anubhav9042

Also when db_table name truncation is done, we get a weird message:

#models.py
class aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(models.Model):

name = models.CharField(max_length=40)

python manage.py syncdb

Creating table probs_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa36a3
Warning: Data truncated for column 'name' at row 13

Instead of reporting about the truncated name of the model table, it seems to tell about the first field of that table.
This is something done by the backend I believe.(Correct me if I am wrong.)
Assuming if above is correct, then what we can do is create a warning using warning.warn(...) about the correct issue.
Then we can remove the false warning arising from the db.

comment:12 Changed 16 months ago by anubhav9042

I hope to work on this in GSoC 2014

comment:13 Changed 13 months ago by anubhav9042

  • Version changed from 1.2 to master

New PR: https://github.com/django/django/pull/2738

Check and test added.

Last edited 13 months ago by anubhav9042 (previous) (diff)

comment:14 Changed 13 months ago by timo

  • Patch needs improvement set

We are going to revisit this after working on #13528. I think we might be able to same a similar approach here.

comment:15 Changed 13 months ago by anubhav9042

As commented on #13528, the problem seems much bigger. So perhaps we could get rid of this bug with my patch keeping in mind that this bug affects both single-db and multi-db users.
My patch solves the problem for both cases and I think user should be making Model keeping in mind the database(s) he is going to use and the patch does the same thing.

comment:16 Changed 13 months ago by timo

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:17 Changed 13 months ago by Tim Graham <timograham@…>

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

In 91f1b6dcdc5da47d7794a55e3114820407a5bd62:

Fixed #13711 -- Model check added to ensure that auto-generated column name is within limits of the database.

Thanks russellm for report and Tim Graham for review.

comment:18 Changed 13 months ago by timo

  • Cc shai added
  • Has patch unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Ready for checkin to Accepted

Reopening as these changes prevent the Oracle tests from running:

ERRORS:
backends.VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ: (models.E018) Autogenerated column name too long for field "charfield_is_quite_long_zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz". Maximum length is "30" for database "default".
	HINT: Set the column name manually using 'db_column'.
backends.VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ: (models.E018) Autogenerated column name too long for field "primary_key_is_quite_long_zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz". Maximum length is "30" for database "default".
	HINT: Set the column name manually using 'db_column'.
backends.VeryLongModelNameZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ: (models.E019) Autogenerated column name too long for M2M field "verylongmodelnamezzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz_id". Maximum length is "30" for database "default".
	HINT: Use 'through' to create a separate model for M2M and then set column_name using 'db_column'.
sites_framework.CustomArticle: (models.E018) Autogenerated column name too long for field "places_this_article_should_appear_id". Maximum length is "30" for database "default".
	HINT: Set the column name manually using 'db_column'.

comment:19 Changed 13 months ago by timo

As discussed with Shai on IRC, column names are auto-truncated in Oracle so the model check isn't needed so much there, or at least the errors should be reduced to warnings. 3rd-party backends may implemented similar behavior so we should keep that in mind when deciding on a solution.

comment:20 Changed 13 months ago by anubhav9042

One approach:

Adding a attribute which tells whether truncate_name is called or not, like connection.supports_auto_truncation and then running the model check only for those dbs which have this set to False.

EDITS:
This could be similar to the one which was removed:connection.supports_long_model_names(This was used for the dbs which either did truncation calling truncate_name or by itself by skipping the chars after its max limit. This second part created problems). It would be like renaming this attribute to the above and correcting its usage because the earlier use created problems and the new usage for checking only usage of truncate_name does not.
This should be True for any backends that uses truncate_name.

Last edited 13 months ago by anubhav9042 (previous) (diff)

comment:21 Changed 13 months ago by anubhav9042

If we can make a decision then I will make a patch for this.

comment:22 Changed 13 months ago by anubhav9042

I have made a patch for what I suggested: https://github.com/coder9042/django/compare/gsoc_13711
IMO this is the right way for this issue and also now the check runs for all backends rather than only for the one with minimum allowed length. Also the backends with the connection.feature.supports_auto_truncation set to True are skipped.

comment:23 Changed 13 months ago by timo

The feature flag makes sense to me, but I think a better name would be something like truncates_column_names.

Why does it now make sense to check all backends rather than only the one with the minimum length? As far as I can tell, the user will only care about the error for the database with the minimum length.

comment:24 Changed 13 months ago by shai

  • Resolution fixed deleted
  • Status changed from closed to new

Sorry for not being able to respond earlier.

The feature flag certainly makes sense, and could be used elsewhere; but I don't think it is the right approach for solving this issue. Its name should probably be truncates_names -- @timo is in the right direction, but it isn't just column names that get truncated, other names (tables, indexes) do too.

Oracle has specific related problems: Very long ago, auto-generated M2M table name truncation was implemented in a way that is different from truncation in the other cases (user-specified or auto-generated names for explicitly defined tables and columns). This is usually not a problem, but in some cases it is (it was a problem for South, and I'm not quite sure what the new migrations do about it). Mind you, it is not a problem in Oracle itself, but in the Django Oracle backend, but it cannot be corrected without breaking backwards-compatibility.

Such an issue, when the names are long enough to trigger it, warrants a warning IMO. But a flag "truncates_m2m_table_names_differently" makes no sense, so I don't see room for including such a check in the backend-agnostic core. It belongs in the backend itself.

While at it, we could add other checks about names (sometimes, Oracle sees a quoted keyword as a keyword, not a name, and fails things for no good reason). It's not just the lengths.

While @timo is right about name length checks (checking the minimum is enough), this is not so for these other checks.

I'm not familiar enough with the checks framework -- how hard would it be to define backend-speicific checks in the backend?

If the checks can be defined in the backends, I think it might be better to include all name checks -- length and otherwise -- in the backend, sharing code using the backends' inheritance trees.

I can see an argument that I am conflating length checks, which can be generic, with other checks, but I think it is better to have name checks united.

comment:25 Changed 13 months ago by anubhav9042

I think that this ticket is about solving issue in long name truncation and adding a relevant flag does that.
What @shai says is a much bigger issue which must be dealt separately in a different ticket where we can discuss for available approaches.

comment:26 Changed 13 months ago by anubhav9042

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top