mimic AR join alias calculation for query (#5682)

* mimic AR join alias calculation for query

* remove unused columns parameter on v3 query get


[ci skip]

app/models/query.rb

@@ -75,50 +75,48 @@ class Query < ActiveRecord::Base
                     sortable: "#{WorkPackage.table_name}.id",
                     groupable: false),
     QueryColumn.new(:project,
-                    sortable: "#{Project.table_name}.name",
+                    sortable: "name",
                     groupable: true),
     QueryColumn.new(:subject,
                     sortable: "#{WorkPackage.table_name}.subject"),
     QueryColumn.new(:type,
-                    sortable: "#{::Type.table_name}.position",
+                    sortable: "position",
                     groupable: true),
     QueryColumn.new(:parent,
-                    sortable: ["#{WorkPackage.table_name}.root_id",
-                               "#{WorkPackage.table_name}.lft"],
+                    sortable: ["root_id",
+                               "lft"],
                     default_order: 'asc'),
     QueryColumn.new(:status,
-                    sortable: "#{Status.table_name}.position",
+                    sortable: "position",
                     groupable: true),
     QueryColumn.new(:priority,
-                    sortable: "#{IssuePriority.table_name}.position",
+                    sortable: "position",
                     default_order: 'desc',
                     groupable: true),
     QueryColumn.new(:author,
-                    sortable: ["#{User.table_name}.lastname",
-                               "#{User.table_name}.firstname",
-                               "#{WorkPackage.table_name}.author_id"],
+                    sortable: ["lastname",
+                               "firstname",
+                               "id"],
                     groupable: true),
     QueryColumn.new(:assigned_to,
-                    sortable: ["#{User.table_name}.lastname",
-                               "#{User.table_name}.firstname",
-                               "#{WorkPackage.table_name}.assigned_to_id"],
+                    sortable: ["lastname",
+                               "firstname",
+                               "id"],
                     groupable: true),
     QueryColumn.new(:responsible,
-                    sortable: ["#{User.table_name}.lastname",
-                               "#{User.table_name}.firstname",
-                               "#{WorkPackage.table_name}.responsible_id"],
-                    groupable: true,
-                    join: 'LEFT OUTER JOIN users as responsible ON ' +
-                          "(#{WorkPackage.table_name}.responsible_id = responsible.id)"),
+                    sortable: ["lastname",
+                               "firstname",
+                               "id"],
+                    groupable: true),
     QueryColumn.new(:updated_at,
                     sortable: "#{WorkPackage.table_name}.updated_at",
                     default_order: 'desc'),
     QueryColumn.new(:category,
-                    sortable: "#{Category.table_name}.name",
+                    sortable: "name",
                     groupable: true),
     QueryColumn.new(:fixed_version,
-                    sortable: ["#{Version.table_name}.effective_date",
-                               "#{Version.table_name}.name"],
+                    sortable: ["effective_date",
+                               "name"],
                     default_order: 'desc',
                     groupable: true),
     # Put empty start_dates in the far future rather than in the far past
@@ -386,18 +384,6 @@ class Query < ActiveRecord::Base
     column_names.empty?
   end
 
-  ##
-  # Returns the columns involved in this query, including those only needed for sorting or grouping
-  # puposes, not only the ones displayed (see :columns).
-  def involved_columns
-    columns = self.columns.map(&:name)
-
-    columns << group_by.to_sym if group_by
-    columns += sort_criteria.map { |x| x.first.to_sym }
-
-    columns.uniq
-  end
-
   def sort_criteria=(arg)
     if arg.is_a?(Hash)
       arg = arg.keys.sort.map { |k| arg[k] }
@@ -410,13 +396,6 @@ class Query < ActiveRecord::Base
     read_attribute(:sort_criteria) || []
   end
 
-  def sort_criteria_sql
-    criteria = SortHelper::SortCriteria.new
-    criteria.available_criteria = sortable_key_by_column_name
-    criteria.criteria = sort_criteria
-    criteria.to_sql
-  end
-
   def sort_criteria_key(arg)
     sort_criteria && sort_criteria[arg] && sort_criteria[arg].first
   end
@@ -448,13 +427,6 @@ class Query < ActiveRecord::Base
     sort_criteria.any?
   end
 
-  # Returns the SQL sort order that should be prepended for grouping
-  def group_by_sort_order
-    if grouped? && (column = group_by_column)
-      Array(column.sortable).map { |s| "#{s} #{column.default_order}" }.join(',')
-    end
-  end
-
   # Returns true if the query is a grouped query
   def grouped?
     !group_by_column.nil?

app/models/query/results.rb

@@ -70,15 +70,11 @@ class ::Query::Results
   end
 
   def work_packages
-    includes = ([:status, :project] +
-      includes_for_columns(query.involved_columns) + (options[:include] || [])).uniq
-
     WorkPackage
       .visible
       .where(query.statement)
       .where(options[:conditions])
-      .includes(includes)
-      .joins((query.group_by_column ? query.group_by_column.join : nil))
+      .includes(all_includes)
       .order(order_option)
       .references(:projects)
   end
@@ -88,7 +84,7 @@ class ::Query::Results
   # If there is a reason: This is a somewhat DRY way of using the sort criteria.
   # If there is no reason: The :work_package method can die over time and be replaced by this one.
   def sorted_work_packages
-    work_packages.order(query.sort_criteria_sql)
+    work_packages.order(sort_criteria_sql)
   end
 
   def versions
@@ -133,7 +129,7 @@ class ::Query::Results
   end
 
   def order_option
-    order_option = [query.group_by_sort_order, options[:order]].reject(&:blank?).join(', ')
+    order_option = [group_by_sort_order].reject(&:blank?).join(', ')
     order_option = nil if order_option.blank?
 
     order_option
@@ -141,15 +137,15 @@ class ::Query::Results
 
   private
 
+  def all_includes
+    (%i(status project) +
+      includes_for_columns(include_columns) +
+      (options[:include] || [])).uniq
+  end
+
   def includes_for_columns(column_names)
     column_names = Array(column_names)
-    includes = (WorkPackage.reflections.keys.map(&:to_sym) & column_names.map(&:to_sym))
-
-    if column_names.any? { |column| custom_field_column?(column) }
-      includes << { custom_values: :custom_field }
-    end
-
-    includes
+    (WorkPackage.reflections.keys.map(&:to_sym) & column_names.map(&:to_sym))
   end
 
   def custom_field_column?(name)
@@ -203,4 +199,95 @@ class ::Query::Results
   def transform_custom_field_keys(custom_field, groups)
     groups.transform_keys { |key| custom_field.cast_value(key) }
   end
+
+  ##
+  # Returns the columns that need to be included to allow:
+  # * sorting
+  # * grouping
+  def include_columns
+    columns = query.sort_criteria.map { |x| x.first.to_sym }
+
+    columns << query.group_by.to_sym if query.group_by
+
+    columns.uniq
+  end
+
+  def sort_criteria_sql
+    criteria = SortHelper::SortCriteria.new
+    criteria.available_criteria = aliased_sorting_by_column_name
+    criteria.criteria = query.sort_criteria
+    criteria.to_sql
+  end
+
+  def aliased_sorting_by_column_name
+    sorting_by_column_name = query.sortable_key_by_column_name
+
+    aliases = include_aliases
+
+    reflection_includes.each do |inc|
+      sorting_by_column_name[inc.to_s] = Array(sorting_by_column_name[inc.to_s]).map { |column| "#{aliases[inc]}.#{column}" }
+    end
+
+    sorting_by_column_name
+  end
+
+  # Returns the SQL sort order that should be prepended for grouping
+  def group_by_sort_order
+    if query.grouped? && (column = query.group_by_column)
+      aliases = include_aliases
+
+      Array(column.sortable).map do |s|
+        aliased_group_by_sort_order(s, column, aliases[column.name])
+      end.join(',')
+    end
+  end
+
+  def aliased_group_by_sort_order(sortable, column, alias_name)
+    if alias_name
+      "#{alias_name}.#{sortable} #{column.default_order}"
+    else
+      "#{sortable} #{column.default_order}"
+    end
+  end
+
+  # To avoid naming conflicts, joined tables are aliased if they are joined
+  # more than once. Here, joining tables that are referenced by multiple
+  # columns are of particular interest.
+  #
+  # Mirroring the way AR creates aliases for included/joined tables: Normally,
+  # included/joined associations are not aliased and as such, they simply use
+  # the table name. But if an association is joined/included that relies on a
+  # table which an already joined/included association also relies upon, that
+  # name is already taken in the DB query. Therefore, the #alias_candidate
+  # method is used which will concatenate the pluralized association name with
+  # the table name the association is defined for.
+  #
+  # There is no handling for cases when the same association is joined/included
+  # multiple times as the rest of the code should prevent that.
+  def include_aliases
+    counts = Hash.new do |h, key|
+      h[key] = 0
+    end
+
+    reflection_includes.each_with_object({}) do |inc, hash|
+      reflection = WorkPackage.reflections[inc.to_s]
+      table_name = reflection.klass.table_name
+
+      hash[inc] = reflection_alias(reflection, counts[table_name])
+
+      counts[table_name] += 1
+    end
+  end
+
+  def reflection_includes
+    WorkPackage.reflections.keys.map(&:to_sym) & all_includes.map(&:to_sym)
+  end
+
+  def reflection_alias(reflection, count)
+    if count.zero?
+      reflection.klass.table_name
+    else
+      reflection.alias_candidate(WorkPackage.table_name)
+    end
+  end
 end

app/models/query_column.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2017 the OpenProject Foundation (OPF)
@@ -33,7 +34,6 @@ class QueryColumn
                 :groupable,
                 :summable,
                 :available,
-                :join,
                 :default_order
   alias_method :summable?, :summable
   include Redmine::I18n
@@ -50,8 +50,6 @@ class QueryColumn
 
     self.available = options.fetch(:available, true)
 
-    self.join = options.delete(:join)
-
     @caption_key = options[:caption] || name.to_s
   end
 
@@ -112,7 +110,7 @@ class QueryColumn
 
     # Explicitly checking for true because apparently, we do not want
     # truish values to count here.
-    if (value == true)
+    if value == true
       name.to_s
     else
       value

frontend/app/components/wp-query/url-params-helper.test.ts

@@ -126,7 +126,6 @@ describe('UrlParamsHelper', function() {
       let decodedQueryParams = UrlParamsHelper.buildV3GetQueryFromJsonParams(params);
 
       let expected = {
-        'columns[]': ['type', 'status', 'soße'],
         showSums: true,
         timelineVisible: true,
         showHierarchies: true,

frontend/app/components/wp-query/url-params-helper.ts

@@ -111,7 +111,7 @@ export class UrlParamsHelperService {
   public buildV3GetQueryFromJsonParams(updateJson:any) {
     var queryData:any = {
       pageSize: this.PaginationService.getPerPage()
-    }
+    };
 
     if (!updateJson) {
       return queryData;
@@ -119,9 +119,6 @@ export class UrlParamsHelperService {
 
     var properties = JSON.parse(updateJson);
 
-    if (properties.c) {
-      queryData["columns[]"] = properties.c.map((column:any) => column);
-    }
     if (!!properties.s) {
       queryData.showSums = properties.s;
     }

spec/models/query/results_spec.rb

@@ -34,22 +34,25 @@ describe ::Query::Results, type: :model do
                       show_hierarchies: false
   end
   let(:query_results) do
-    ::Query::Results.new query, include: [:assigned_to,
-                                          :type,
-                                          :priority,
-                                          :category,
-                                          :fixed_version],
-                                order: 'work_packages.root_id DESC, work_packages.lft ASC'
+    ::Query::Results.new query,
+                         include: %i(
+                           assigned_to
+                           type
+                           priority
+                           category
+                           fixed_version
+                         ),
+                         order: 'work_packages.root_id DESC, work_packages.lft ASC'
   end
   let(:project_1) { FactoryGirl.create :project }
   let(:role_pm) do
     FactoryGirl.create(:role,
-                       permissions: [
-                         :view_work_packages,
-                         :edit_work_packages,
-                         :create_work_packages,
-                         :delete_work_packages
-                       ])
+                       permissions: %i(
+                         view_work_packages
+                         edit_work_packages
+                         create_work_packages
+                         delete_work_packages
+                       ))
   end
   let(:role_dev) do
     FactoryGirl.create(:role,
@@ -172,8 +175,7 @@ describe ::Query::Results, type: :model do
 
         it 'returns all work packages of project 2' do
           work_packages = query
-                          .results(include: [:assigned_to, { custom_values: :custom_field }],
-                                   order: 'work_packages.root_id, work_packages.lft')
+                          .results
                           .work_packages
                           .page(1)
                           .per_page(10)
@@ -191,8 +193,7 @@ describe ::Query::Results, type: :model do
 
         it 'returns all work packages of project 2' do
           work_packages = query
-                          .results(include: [:responsible, { custom_values: :custom_field }],
-                                   order: 'work_packages.root_id, work_packages.lft')
+                          .results
                           .work_packages
                           .page(1)
                           .per_page(10)
@@ -225,6 +226,166 @@ describe ::Query::Results, type: :model do
     end
   end
 
+  describe '#sorted_work_packages' do
+    let(:work_package1) { FactoryGirl.create(:work_package, project: project_1) }
+    let(:work_package2) { FactoryGirl.create(:work_package, project: project_1) }
+    let(:work_package3) { FactoryGirl.create(:work_package, project: project_1) }
+    let(:sort_by) { [['parent', 'asc']] }
+    let(:columns) { %i(id subject) }
+
+    let(:query) do
+      FactoryGirl.build_stubbed :query,
+                                show_hierarchies: false,
+                                group_by: group_by,
+                                sort_criteria: sort_by,
+                                project: project_1,
+                                column_names: columns
+    end
+
+    let(:query_results) do
+      ::Query::Results.new query
+    end
+
+    let(:user_a) { FactoryGirl.create(:user, firstname: 'AAA', lastname: 'AAA') }
+    let(:user_m) { FactoryGirl.create(:user, firstname: 'MMM', lastname: 'MMM') }
+    let(:user_z) { FactoryGirl.create(:user, firstname: 'ZZZ', lastname: 'ZZZ') }
+
+    context 'grouping by assigned_to, having the author column selected' do
+      let(:group_by) { 'assigned_to' }
+      let(:columns) { %i(id subject author) }
+
+      before do
+        allow(User).to receive(:current).and_return(user_1)
+
+        work_package1.assigned_to = user_m
+        work_package1.author = user_m
+
+        work_package1.save(validate: false)
+
+        work_package2.assigned_to = user_z
+        work_package2.author = user_a
+
+        work_package2.save(validate: false)
+
+        work_package3.assigned_to = user_m
+        work_package3.author = user_a
+
+        work_package3.save(validate: false)
+      end
+
+      it 'sorts first by assigned_to (group by), then by sort criteria' do
+        # Would look like this in the table
+        #
+        # user_m
+        #   work_package 1
+        #   work_package 3
+        # user_z
+        #   work_package 2
+        expect(query_results.sorted_work_packages)
+          .to match [work_package1, work_package3, work_package2]
+      end
+    end
+
+    context 'sorting by author, grouping by assigned_to' do
+      let(:group_by) { 'assigned_to' }
+      let(:sort_by) { [['author', 'asc']] }
+
+      before do
+        allow(User).to receive(:current).and_return(user_1)
+
+        work_package1.assigned_to = user_m
+        work_package1.author = user_m
+
+        work_package1.save(validate: false)
+
+        work_package2.assigned_to = user_z
+        work_package2.author = user_a
+
+        work_package2.save(validate: false)
+
+        work_package3.assigned_to = user_m
+        work_package3.author = user_a
+
+        work_package3.save(validate: false)
+      end
+
+      it 'sorts first by group by, then by assigned_to' do
+        # Would look like this in the table
+        #
+        # user_m
+        #   work_package 3
+        #   work_package 1
+        # user_z
+        #   work_package 2
+        expect(query_results.sorted_work_packages)
+          .to match [work_package3, work_package1, work_package2]
+
+        query.sort_criteria = [['author', 'desc']]
+
+        # Would look like this in the table
+        #
+        # user_m
+        #   work_package 1
+        #   work_package 3
+        # user_z
+        #   work_package 2
+        expect(query_results.sorted_work_packages)
+          .to match [work_package1, work_package3, work_package2]
+      end
+    end
+
+    context 'sorting by author and responsible, grouping by assigned_to' do
+      let(:group_by) { 'assigned_to' }
+      let(:sort_by) { [['author', 'asc'], ['responsible', 'desc']] }
+
+      before do
+        allow(User).to receive(:current).and_return(user_1)
+
+        work_package1.assigned_to = user_m
+        work_package1.author = user_m
+        work_package1.responsible = user_a
+
+        work_package1.save(validate: false)
+
+        work_package2.assigned_to = user_z
+        work_package2.author = user_m
+        work_package3.responsible = user_m
+
+        work_package2.save(validate: false)
+
+        work_package3.assigned_to = user_m
+        work_package3.author = user_m
+        work_package3.responsible = user_z
+
+        work_package3.save(validate: false)
+      end
+
+      it 'sorts first by group by, then by assigned_to (neutral as equal), then by responsible' do
+        # Would look like this in the table
+        #
+        # user_m
+        #   work_package 3
+        #   work_package 1
+        # user_z
+        #   work_package 2
+        expect(query_results.sorted_work_packages)
+          .to match [work_package3, work_package1, work_package2]
+
+        query.sort_criteria = [['author', 'desc'], ['responsible', 'asc']]
+
+        # Would look like this in the table
+        #
+        # user_m
+        #   work_package 1
+        #   work_package 3
+        # user_z
+        #   work_package 2
+        expect(query_results.sorted_work_packages)
+          .to match [work_package1, work_package3, work_package2]
+      end
+    end
+  end
+
   # Introduced to ensure being able to group by custom fields
   # when running on a MySQL server.
   # When upgrading to rails 5, the sql_mode passed on with the connection