Kezabelle test case 992

[ojii]

Fix proposed by kezabelle (#992 (comment)) slightly edited to always force a 'cmsplugin_' in front of plugin tables. if we do this kind of magic, let's do it consistently.

[kezabelle]

I don't really know the details of the decision to change the way table name generation occurs, so I can't really comment on the pros and cons, but as I see it, this is still kind of a painful solution, though (for all I know - it is beyond my understanding) it may be the only one available.

• There's a certain expectation that options declared on a model are to be taken 'as-is' - in Django itself, if I declare a model with a db_table name, that is it's name. What's gone into 2.2, and is being proposed here for 2.2.x is thus counter-intuitive for developers with prior Django experience.
• As a result of the issue coming to light, this needs documenting, somewhere in the docs, and preferably with an explanation as to why it needs to mess with user-defined configuration in such a non-obvious way.
• What crept into the 2.2 release was a backwards incompatible change for some people (myself included), and this changeset remains so, because what might've previously been my_table would now expect the prefix cmsplugin_. This needs covering in the upgrade path from 2.1.x to 2.2
  • Upgrading to 2.2 may be a complete non-starter for users not familiar with schema migrations or SQL, as they may need to rename tables to fit the new system.
  • For users familiar with south, or another migration tool, the problem is solvable, but not in an automated way, relying on (for south) the generation of an empty schema migration script into which one must manually call rename_table. (That might be a costly operation to run under different DB vendors, or large tables. That much is speculation, though.)

[ojii]

I agree that the whole table-renaming mess sucks. However, we can't easily change this without breaking backwards compatibility in a spectacular and annoying way. So unfortunately, removing the 'cmsplugin_' prefix won't be possible. I still blame @piquadrat for this :D

The initial idea was to rename the core plugins (eg 'text') to 'cmsplugin_text'. The idea was to change the appname (as in physically move the subfolder and write a smart migration), not monkeypatching table names. So now we pollute the Django namespace with super generic names ('text', 'picture', 'link') on top of messing with ORM internals...

What is the exact changeset in 2.2 that is backwards incompatible?

[kezabelle]

In 2.1.x, the table name only got prefixed with cmsplugin_ if app_label was at the start of the table name (as per table names generated by django). As far as I can tell, this left one with the ability to call the table whatever_i_like - as long as it didn't contain the app_label at the beginning, this table would be left alone and would be used as-is for the plugin's DB configuration backend.

Now, the 2.2 changes are forcing it to unilaterally apply cmsplugin_ to the beginning (which certainly makes things more consistent) regardless of settings. Your modifications of the test cases bear this out, as the only time the db_table is left alone is, I think, when it already starts with cmsplugin?

If I'm reading that correctly, and the intention is to always prepend the table name with the cmsplugin prefix, then it is for better or worse, backwards incompatible for some people, even if it is the result of edge cases slipping through previous version.

Hence, if my whatever_i_like table wants to work with 2.2, I believe I need to do a south migration and rename the table to cmsplugin_whatever_i_like.

Does that make sense? Am I even right? I don't know anymore :o)

[ojii]

All sounds correct to me. The intention of my changes were to be consistent. But compatibility is a good reason not to change that. Still wish we could drop the whole thing :(

[beniwohli]

@ojii: jokes in the office is one thing, but publicly blaming me for piece of shit code that I did not write overstreches it a bit, IMHO. There's enough piece of shit code that I actually did write, you can blame me for that as much as you like ;-)

History buffs might be interested in ticket #74.

[ojii]

@piquadrat: I hope you (and everyone else) noticed the ":D" at the end. Text just sucks as a medium to indicate that certain parts of it are not intended seriously.

So just to make this clear: It's not actually piquadrats fault, and it doesn't matter who's it is. We just need to find a way to make this somewhat work.

[ojii]

@kezabelle: what's the least intrusive way to handle this (as in: not breaking backwards compatiblity?)

[kezabelle]

I can't, at the moment, see a solution that would get back to complete 2.1.x compatibility, because by it's nature this is attempting to correct for historic slips through the cracks.

The only way I can see to account for the edge case of having used something completely incongruous like my_table is to introduce a new option on Meta which allows one to forcibly skip any attempts to compute the db_table name, instead falling back to whatever the user has declared in their model's Meta. But that's kind of dreadful, and introduces new configuration just to correct for previous 'bad behaviour' either on the part of the CMS or the developer.

At least one of the original reports (in #992) was able to fix it for themselves by checking whether cmsplugin was already in the name, which indicates that the problem would be solved for them. It'd be interesting to see what @ajmirsky's declared table names were -- there is the possibility that we're striving for backwards compatibility for an edge case that people aren't encountering; if that's the case, it may not be worth the effort [nor code complexity] to do more than note it in the docs as a change, and leave the problem to the developers who've so woefully abused table names.

[ojii]

since this pull request seems to make things even worse, I'm closing it.

[ojii]

re-opened after discussion with @kezabelle on IRC

[kezabelle]

To confirm then, the new changeset only does the following:

• If the table name received contains the app label, it is assumed to be computed, and so the app label is replaced by the cmsplugin prefix.
• If it doesn't start with the app label, it either already has the prefix, or is otherwise customised, and is left as is.

This certainly seems to return the functionality one could expect of 2.1.x

postscript edit

I've just thought - while this should return the functionality of the previous version, it does still mean, at least in theory, the db_table may not be the configured value, in the instance where the user has specifically hard-coded the db_table to the same values as those computed? (ie: beginning with app_label)

That, however, is still what the previous version did, and I don't think anyone's ever raised a ticket for it.

[ojii]

the goal of this ticket is to fix #992 and remain as backwards compatible as possible. The fact is that being backwards compatible means messing around with db_table.