Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#26351 closed Cleanup/optimization (fixed)

Recommend enabling MySQL's STRICT_TRANS_TABLES to prevent silent truncation

Reported by: Zhebrak Alexander Owned by: nobody
Component: Documentation Version: 1.9
Severity: Normal Keywords: get_or_create, max_length
Cc: me@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

class Post(models.Model):
    title = models.CharField(max_length=10)


search_title = 'A' * 11

Post.objects.get_or_create(title=search_title)
Post.objects.get_or_create(title=search_title)

This code creates two objects because lookup is trying to find non-truncated title and fails to get one so it creates a new object with a truncated title. The second call does exactly the same creating another object.

I don't know if it supposed to work this way or not but this behavior seems unclear to me.

Change History (26)

comment:1 Changed 7 years ago by Mike Dunhem

What database are you using? That should fail validation and raise an exception rather than truncating the title field. This may really depend on your database type.

comment:2 in reply to:  1 Changed 7 years ago by Zhebrak Alexander

Replying to mdunhem:

What database are you using? That should fail validation and raise an exception rather than truncating the title field. This may really depend on your database type.

We use MySQL (5.1, 5.5). By default it truncates any given string by max_length for the sake of not giving errors. It's not obvious though, especially when using get_or_create.

comment:3 Changed 7 years ago by Tim Graham

Component: Database layer (models, ORM)Documentation
Summary: get_or_create called twice on a model with CharField and field value over max_length creates 2 objectsRecommend enabling MySQL's STRICT_TRANS_TABLES to prevent silent truncation
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I think there's not much Django can do except to recommend enabling STRICT_TRANS_TABLES to avoid silent truncation in the MySQL database notes in docs/ref/databases.txt. It's enabled by default for MySQL 5.7+.

comment:4 Changed 7 years ago by Claude Paroz

#15940 is related.

comment:5 Changed 7 years ago by Claude Paroz

See also the `strict_mode_warning` check in django-mysql.

comment:6 Changed 7 years ago by Claude Paroz

Has patch: set

A possible resolution by implementing a database check (partially copied from django-mysql).

comment:7 Changed 7 years ago by Tim Graham

Patch needs improvement: set

This would be extending the system check framework in a new direction (currently it's mostly limited to static code analysis). While I don't see a big problem with this, I'll raise the alternative of a separate testconnections command as proposed in #24475. Anyway, I left some comments for improvement on the patch as currently proposed.

comment:8 Changed 7 years ago by Adam Johnson

Cc: me@… added

comment:9 Changed 7 years ago by Mike Dunhem

Should we also add a note in the documentation?

comment:10 Changed 7 years ago by Collin Anderson

As a cross reference, there are warnings generated when there's truncation, and they used to be errors, but only in debug mode. #23871

comment:11 Changed 7 years ago by Claude Paroz

Patch needs improvement: unset

I've updated the pull request and separated it in two commits. In the first one, I've extended the DatabaseValidation class to host database-related checks. I also limited database-related checks to only the migrate command so we don't get those warnings/errors for each check run.

comment:12 Changed 7 years ago by Shai Berger

If MySql has already made STRICT_TRANS_TABLES the default, I suggest we do the same. In a few versions, when MySql 5.5 and 5.6 reach EOL, we'll be there anyway.

This comment is almost orthogonal to the solution of database checks, which is a generally needed feature. The only point of contact is the check for STRICT_TRANS_TABLES itself, since if we default to it, a user who explicitly chose otherwise doesn't need to be warned about their choice.

comment:13 Changed 7 years ago by Claude Paroz

I'm +0 about forcing STRICT_TRANS_TABLES. I think the main advantage of the check over forcing the setting is that we don't load each connection creation on runtime with the cruft of SELECTing the variable to check its current state. The other minor advantage is that people are still free to configure their database at their will, even if I don't see now a use case for that.

comment:14 Changed 7 years ago by Tim Graham

I guess it's a good point about whether we should add the check now that MySQL is changing the default of STRICT_TRANS_TABLES. As Shai said, presumably going forward then, this check will only trigger for users configuring the database otherwise, so I wonder if it adds any value except for users of older versions of MySQL?

comment:15 Changed 7 years ago by Claude Paroz

I think having the check is a good mean to warn users that their MySQL configuration (being for an old MySQL or a MySQL with custom config) might interfere with their Django project running properly. Then, if the choice is deliberate, they can safely disable the warning.

comment:16 Changed 7 years ago by Adam Johnson

It's also valuable for MariaDB users where it's not yet the default.

comment:17 Changed 7 years ago by Tim Graham

How does the proposed patch interact with #15940? It's not clear to me whether this solves the concerns of that ticket or if something else might need to be done to address it after adding this check?

comment:18 Changed 7 years ago by Claude Paroz

I think it does. IMHO the most important thing is to make people aware of the possible truncation issue. What they do next is their business.

comment:19 Changed 7 years ago by Tim Graham

Okay. The docs/ref/databases.txt note in your patch recommends STRICT_TRANS_TABLES as opposed to STRICT_ALL_TABLES or 'sql_mode': 'traditional' as mentioned in the other ticket. If there's a reason to prefer one or the other, I didn't see it discussed. Not a MySQL user myself, just wanted to check about that. MySQL SQL mode reference docs.

comment:20 Changed 7 years ago by Claude Paroz

I updated the docs to mention various modes, and also added a link to the MySQL docs. I also accepted traditional as an accepted strict mode.

comment:21 Changed 7 years ago by Tim Graham

Patch needs improvement: set

Left some comments for improvement.

comment:22 Changed 7 years ago by Claude Paroz

Patch needs improvement: unset

comment:23 Changed 7 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:24 Changed 7 years ago by Claude Paroz <claude@…>

In 0d3c616:

Refs #26351 -- Added check hook to support database-related checks

Thanks Tim Graham and Shai Berger for the reviews.

comment:25 Changed 7 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In f9a2a7d:

Fixed #26351 -- Added MySQL check to warn about strict mode option

Thanks Adam Chainz for the initial implementation in django-mysql.
Thanks Adam Chainz, Tim Graham, and Shai Berger for the reviews.

comment:26 Changed 7 years ago by Tim Graham <timograham@…>

In 93deb16:

Fixed #26492 -- Fixed "maximum recursion depth exceeded" migrate error.

A regression introduced in 0d3c616fbb2f49fa7ff6809e5a6777275352b35b;
refs #26351.

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