Unnecessary database access when IssueQuery class is defined
During plugin development I found out that the definition of the Query class accesses the database just by being defined.
The reason for this is the `groupable` method in the `TimestampQueryColumn` class which tries to access the current user.
This method is called during the constructor (which is called in the query column definitions in the class body of IssueQuery)
Why is this a problem?¶
In case your database is not yet created and/or migrated, any source code which just references the IssueQuery class will raise an exception since it cannot find the database/table.
And in general I think it's not good to access the database just by defining a class.
I attached a patch which also acts as a code cleanup/refactoring. It moves the group by statement into its own method. Before the boolean property `groupable` was just redefined to contain the group by statement as well, which was confusing in the first place in my opinion.
I ran all tests locally and they passed.
Updated by Kevin Fischer about 3 years ago
Is there any chance this change will be considered for the next version?
If you reference the IssueQuery class in a plugin's init.rb file (to include a module with new functionality) it becomes impossible to create a new database (which is very inconvenient when running for example tests on a CI system) since the system complains about the User table not yet existing.
A class definition should not need to access the database.
Updated by Kevin Fischer over 2 years ago
- File 0001-stop-db-access-on-class-definition-v2.patch 0001-stop-db-access-on-class-definition-v2.patch added
I fixed the patch so it works for SQLite, too. Also, I refactored the code around `groupable` a little bit more to make it more obvious that it is a boolean property.
Updated by Marius BALTEANU over 2 years ago
- Tracker changed from Patch to Defect
- Status changed from New to Confirmed
- Assignee changed from Jean-Philippe Lang to Go MAEDA
I've confirmed the issue and the second patch posted by Kevin looks good to me. All tests pass: https://gitlab.com/redmine-org/redmine/-/pipelines/272847174