Fixes/sqlite3 too many sql variables

[jrief]

Summary

Fixes #6746
Ported from #6748 for django-CMS 3.7

Proposed changes in this pull request

Fixes the bug.

General checklist

• I have updated the CHANGELOG.txt
• I have created backports if necessary
• I have updated the documentation and/or amended the upgrade section
  if necessary

[coveralls]

Coverage remained the same at 78.215% when pulling 28c97e8 on jrief:fixes/sqlite3-too-many-SQL-variables into 317ee96 on divio:release/3.7.x.

[jrief]

Additional explanation:
In function get_page_actions_for_user(...), a long list of PKs is generated. This list then is used to generate a filter query checking which page is inside that list.

For SQLite, the Django ORM creates SQL using a ? as placeholder. In SQLite this number of placeholders is limited to 999, hence not more than 999 pages can be descendants of another page. By building chunks of 500, this problem can be avoided.

[FinalAngel]

not a fan of having a magical number in here. As it only affects SQLite we should also probably target only SQLite with such an exception. I'm sadly not deep into the permission system by itself. WDYT @yakky or @czpython ?

[yakky]

I will make this number more explicit by setting a constant (something like PAGE_PERMISSION_CHUNK_SIZE)
We can then have different numbers for different database backends and make it very large (to the point of being infinite) for DBMS other than sqlite

[jrief]

@FinalAngel , @yakky
This limit is hard-coded into SQLite itself using SQLITE_MAX_VARIABLE_NUMBER. Changing it requires the recompilation of the SQLite code. Unfortunately I haven't found any interface to fetch that number using some sort of SQLite dialect. 500 is certainly a safe lower limit which helps to prevent these sorts of errors.

Proposal:

a) I make this number MAX_INT for non-SQLite databases and 500 for SQLite. Making this configurable, in my opinion doesn't make much sense, since rarely anybody knows that limit.
b) Through the Page-model, I check on which database it is running, in order to answer a).

[yakky]

@jrief do you think we can have a test to replicate this behavior? It will probably be very slow (due to the huge number of pages needed) and thus it might be needed to be marked with some skipIf to only run in a subset of the travis matrix

[czpython]

I honestly don't think this is the way to go, for being a limitation of sqlite only, worst-case scenario I would add a condition that avoids this issue only for sqlite.
A more involved solution would be to refactor this into subqueries instead of manual ids.

[jrief]

@yakky puh, writing a test for this scenario would indeed require to add a lot of pages to the tree. I'm not sure if this is worthwhile.

something like PAGE_PERMISSION_CHUNK_SIZE

This is perfectly fine, but that number shall not be configurable through the owners settings.py

@czpython

I would add a condition that avoids this issue only for sqlite.

That's simple. We can use a variable instead of 500 and change that to MAX_INT for non-sqlite database.

A more involved solution would be to refactor this into subqueries instead of manual ids.

I looked at this, but since the queryset which could be used for the subquery, is modified by calling some Django permission functions, I don't believe it's feasible.

Shall I modify that pull request and use a meaningful variable name instead of 500 and apply that only to SQLite?

[yakky]

@jrief
agree PAGE_PERMISSION_CHUNK_SIZE is just an internal constant, not a setting.
As you wrote is set to something like:

if driver == "sqlite3":
   PAGE_PERMISSION_CHUNK_SIZE = 500
else:
   PAGE_PERMISSION_CHUNK_SIZE = MAX_INT

Subqueries would be great, and the with_user call can be probably rewritten, but I don't think it's worth the effort, unless we see some global improvement

[goutnet]

@jrief would this be useful in develop for 3.9.0 ?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This will now be closed due to inactivity, but feel free to reopen it.