Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Filter api: systematic use JOIN when building SQL

Do not join tables using the where clause, for better readability and
avoiding risk of issues with operator precedence and 'any condition'
filtering mode.

This commit also removes an unnecessary LEFT JOIN between the bugnote
and bugnote_text tables; since this is a strict 1:1 relationship, an
inner join is sufficient and yields better performance.
  • Loading branch information...
commit c9bc0646d69b2e39cd5e48320be82a83d8e39ebc 1 parent d16988c
Damien Regad dregad authored
Showing with 8 additions and 7 deletions.
  1. +8 −7 core/filter_api.php
15 core/filter_api.php
View
@@ -1092,7 +1092,6 @@ function filter_get_bug_rows( &$p_page_number, &$p_per_page, &$p_page_count, &$p
$t_project_where_clauses = array(
"$t_project_table.enabled = " . db_param(),
- "$t_project_table.id = $t_bug_table.project_id",
);
$t_where_params = array(
1,
@@ -1101,8 +1100,13 @@ function filter_get_bug_rows( &$p_page_number, &$p_per_page, &$p_page_count, &$p
"$t_bug_table.*",
);
- $t_join_clauses = array();
- $t_from_clauses = array();
+ $t_from_clauses = array(
+ $t_bug_table,
+ );
+
+ $t_join_clauses = array(
+ "JOIN $t_project_table ON $t_project_table.id = $t_bug_table.project_id",
+ );
// normalize the project filtering into an array $t_project_ids
if( 'simple' == $t_view_type ) {
@@ -1997,7 +2001,7 @@ function filter_get_bug_rows( &$p_page_number, &$p_per_page, &$p_page_count, &$p
if ( !$t_first ) {
$t_join_clauses[] = "JOIN $t_bug_text_table ON $t_bug_table.bug_text_id = $t_bug_text_table.id";
$t_join_clauses[] = "LEFT JOIN $t_bugnote_table ON $t_bug_table.id = $t_bugnote_table.bug_id";
- $t_join_clauses[] = "LEFT JOIN $t_bugnote_text_table ON $t_bugnote_table.bugnote_text_id = $t_bugnote_text_table.id";
+ $t_join_clauses[] = "JOIN $t_bugnote_text_table ON $t_bugnote_table.bugnote_text_id = $t_bugnote_text_table.id";

this change breaks filtering for any note-less issue, doesn't it?

Roland Becker Collaborator
atrol added a note

You are right.

Damien Regad Owner
dregad added a note

@atrol I'm not so sure. Did you confirm this in-app ?

The outer join I removed was only between the bugnote and bugnote_text tables (1:1 relationship); we still have an outer join between the bug and bugnote tables (0:N relationship) which unless I'm mistaken should cover the case of issues without bugnotes.

Roland Becker Collaborator
atrol added a note

Example on our own tracker

1) Choose project "MantisBT"
2) Goto page "View Issues"
3) Reset Filter
4) See e.g. #15746: Software allows an insecure password.
5) Type "Software" in search box, click "Apply filter"

a) #15746 is not found
b) notice in column #, that there are only issues listed with notes in it

Damien Regad Owner
dregad added a note

Thanks for the reproducing steps - I'll have a look.

Damien Regad Owner
dregad added a note

Fixed in cb4e22c, thanks for reporting the issue !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
$t_where_clauses[] = $t_textsearch_where_clause;
}
}
@@ -2012,9 +2016,6 @@ function filter_get_bug_rows( &$p_page_number, &$p_per_page, &$p_page_count, &$p
log_event(LOG_FILTERING, 'Join operator : ' . $t_join_operator);
- $t_from_clauses[] = $t_project_table;
- $t_from_clauses[] = $t_bug_table;
-
$t_query_clauses['select'] = $t_select_clauses;
$t_query_clauses['from'] = $t_from_clauses;
$t_query_clauses['join'] = $t_join_clauses;
Philipp Kitzberger

this change breaks filtering for any note-less issue, doesn't it?

Roland Becker
Collaborator

You are right.

Damien Regad

@atrol I'm not so sure. Did you confirm this in-app ?

The outer join I removed was only between the bugnote and bugnote_text tables (1:1 relationship); we still have an outer join between the bug and bugnote tables (0:N relationship) which unless I'm mistaken should cover the case of issues without bugnotes.

Roland Becker
Collaborator

Example on our own tracker

1) Choose project "MantisBT"
2) Goto page "View Issues"
3) Reset Filter
4) See e.g. #15746: Software allows an insecure password.
5) Type "Software" in search box, click "Apply filter"

a) #15746 is not found
b) notice in column #, that there are only issues listed with notes in it

Damien Regad

Thanks for the reproducing steps - I'll have a look.

Damien Regad

Fixed in cb4e22c, thanks for reporting the issue !

Please sign in to comment.
Something went wrong with that request. Please try again.