Recommend enabling MySQL's STRICT_TRANS_TABLES to prevent silent truncation

[Zhebrak Alexander]

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.

[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.

[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.

[Tim Graham]

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+.

[Claude Paroz]

#15940 is related.

[Claude Paroz]

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

[Claude Paroz]

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

[Tim Graham]

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.

[Mike Dunhem]

Should we also add a note in the documentation?

[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

[Claude Paroz]

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.

[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.

[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.

[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?

[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.

[Adam Johnson]

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

[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?

[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.

[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.

[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.

[Tim Graham]

Left some comments for improvement.

[Claude Paroz <claude@…>]

In 0d3c616:

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

Thanks Tim Graham and Shai Berger for the reviews.

[Claude Paroz <claude@…>]

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.

[Tim Graham <timograham@…>]

In 93deb16:

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

A regression introduced in 0d3c616fbb2f49fa7ff6809e5a6777275352b35b;
refs #26351.