Defect #33290
closedUnnecessary database access when IssueQuery class is defined
0%
Description
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.
https://github.com/redmine/redmine/blob/4.1.1/app/models/query.rb#L89-L93
This method is called during the constructor (which is called in the query column definitions in the class body of IssueQuery)
https://github.com/redmine/redmine/blob/4.1.1/app/models/issue_query.rb#L36-L51
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.
Patch¶
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.
Files
Related issues
Updated by Go MAEDA over 4 years ago
- Related to Feature #2679: Ticket grouping added
Updated by Kevin Fischer over 4 years ago
This only happens from Redmine 4.1 by the way.
The commit bringing the change was r17724
Updated by Alexander Meindl over 4 years ago
I can confirm this problem with Redmine 4.1. It is very difficult to test plugins with the bug.
Updated by Go MAEDA over 4 years ago
- Related to Feature #13803: Implement grouping issues by date (start, due, creation, update, closing dates) added
Updated by Kevin Fischer over 4 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 almost 4 years ago
Trying the patch out with the most recent master, the tests don't seem to pass for SQLite.
I will fix the patch and post an updated version soon.
Updated by Marius BĂLTEANU almost 4 years ago
- Related to Defect #33121: IssueQuery not usable from plugins added
Updated by Marius BĂLTEANU almost 4 years ago
- Category set to Plugin API
- Assignee set to Jean-Philippe Lang
- Target version set to 4.1.2
Jean-Philippe, can you take a look on this and the related issue (#33121)?
Updated by Kevin Fischer almost 4 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 BĂLTEANU over 3 years ago
- Target version changed from 4.1.2 to 4.2.0
I'm assigning this to 4.2.0 because the changes are too big for a minor version.
Updated by Marius BĂLTEANU over 3 years ago
- Related to deleted (Defect #33121: IssueQuery not usable from plugins)
Updated by Marius BĂLTEANU over 3 years ago
- Has duplicate Defect #33121: IssueQuery not usable from plugins added
Updated by Marius BĂLTEANU over 3 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
Updated by Go MAEDA over 3 years ago
- Status changed from Confirmed to Closed
- Resolution set to Fixed
Committed the fix. Thank you for your contribution.
Updated by Go MAEDA over 3 years ago
- Subject changed from Stop DB access when IssueQuery class is defined to Unnecessary database access when IssueQuery class is defined
Updated by Go MAEDA over 3 years ago
- Related to Defect #35115: Time entries are broken if grouped by project and issue custom fields added