Project

General

Profile

Actions

Defect #33290

closed

Unnecessary database access when IssueQuery class is defined

Added by Kevin Fischer over 4 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Plugin API
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
Affected version:

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

Related to Redmine - Feature #2679: Ticket groupingClosed2009-02-05

Actions
Related to Redmine - Feature #13803: Implement grouping issues by date (start, due, creation, update, closing dates)ClosedJean-Philippe Lang

Actions
Related to Redmine - Defect #35115: Time entries are broken if grouped by project and issue custom fieldsClosedGo MAEDA

Actions
Has duplicate Redmine - Defect #33121: IssueQuery not usable from pluginsClosed

Actions
Actions #1

Updated by Go MAEDA over 4 years ago

Actions #2

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

Actions #3

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.

Actions #4

Updated by Go MAEDA over 4 years ago

  • Related to Feature #13803: Implement grouping issues by date (start, due, creation, update, closing dates) added
Actions #5

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.

Actions #6

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.

Actions #7

Updated by Marius BĂLTEANU almost 4 years ago

  • Related to Defect #33121: IssueQuery not usable from plugins added
Actions #8

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

Actions #9

Updated by Kevin Fischer almost 4 years ago

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.

Actions #10

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.

Actions #11

Updated by Marius BĂLTEANU over 3 years ago

  • Related to deleted (Defect #33121: IssueQuery not usable from plugins)
Actions #12

Updated by Marius BĂLTEANU over 3 years ago

  • Has duplicate Defect #33121: IssueQuery not usable from plugins added
Actions #13

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

Actions #14

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.

Actions #15

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
Actions #16

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
Actions

Also available in: Atom PDF