Defect #33290

Unnecessary database access when IssueQuery class is defined

Added by Kevin Fischer over 1 year ago. Updated 7 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Plugin API
Target version:4.2.0
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.

0001-stop-db-access-on-class-definition.patch Magnifier (1.65 KB) Kevin Fischer, 2020-04-15 10:44

0001-stop-db-access-on-class-definition-v2.patch Magnifier (3.08 KB) Kevin Fischer, 2021-02-05 11:54


Related issues

Related to Redmine - Feature #2679: Ticket grouping Closed 2009-02-05
Related to Redmine - Feature #13803: Implement grouping issues by date (start, due, creation, ... Closed
Related to Redmine - Defect #35115: Time entries are broken if grouped by project and issue c... Closed
Duplicated by Redmine - Defect #33121: IssueQuery not usable from plugins Closed

Associated revisions

Revision 20830
Added by Go MAEDA 7 months ago

Unnecessary database access when IssueQuery class is defined (#33290).

Patch by Kevin Fischer.

History

#1 Updated by Go MAEDA over 1 year ago

#2 Updated by Kevin Fischer over 1 year ago

This only happens from Redmine 4.1 by the way.

The commit bringing the change was r17724

#3 Updated by Alexander Meindl over 1 year ago

I can confirm this problem with Redmine 4.1. It is very difficult to test plugins with the bug.

#4 Updated by Go MAEDA over 1 year ago

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

#5 Updated by Kevin Fischer over 1 year 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.

#6 Updated by Kevin Fischer 9 months 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.

#7 Updated by Marius BALTEANU 9 months ago

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

#8 Updated by Marius BALTEANU 9 months 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)?

#9 Updated by Kevin Fischer 9 months 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.

#10 Updated by Marius BALTEANU 8 months 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.

#11 Updated by Marius BALTEANU 7 months ago

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

#12 Updated by Marius BALTEANU 7 months ago

  • Duplicated by Defect #33121: IssueQuery not usable from plugins added

#13 Updated by Marius BALTEANU 7 months 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

#14 Updated by Go MAEDA 7 months ago

  • Status changed from Confirmed to Closed
  • Resolution set to Fixed

Committed the fix. Thank you for your contribution.

#15 Updated by Go MAEDA 7 months ago

  • Subject changed from Stop DB access when IssueQuery class is defined to Unnecessary database access when IssueQuery class is defined

#16 Updated by Go MAEDA 6 months ago

  • Related to Defect #35115: Time entries are broken if grouped by project and issue custom fields added

Also available in: Atom PDF