From ff0549fcce31bd2a73dc868d13f0c775f03084ea Mon Sep 17 00:00:00 2001 From: Christian Rijke Date: Tue, 1 Oct 2013 16:37:20 +0200 Subject: [PATCH 1/2] Restrict selected columns to the 'entries' sub-query of the reporting query for the reporting query. Most filters require additional joins to be added to the report query (e.g. 'JOIN work_packages'). With the default '*' selection and the way the reporting query is structured in reporting_engine, columns from these additional joins appear in the result set. This led to obscure bugs in case of column name clashes and columns with identical names but different values. --- app/models/cost_query/sql_statement.rb | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/app/models/cost_query/sql_statement.rb b/app/models/cost_query/sql_statement.rb index d6827c6204..919cba9b29 100644 --- a/app/models/cost_query/sql_statement.rb +++ b/app/models/cost_query/sql_statement.rb @@ -5,6 +5,21 @@ class CostQuery::SqlStatement < Report::SqlStatement costs overridden_costs type ] + # flag to mark a reporting query consisting of a union of cost and time entries + attr_accessor :entry_union + + def initialize(table, desc = "") + super(table, desc) + @entry_union = false + end + + # this is a hack to ensure that additional joins added by filters do not result + # in additional columns being selected. + def to_s + select(['entries.*']) if select == ['*'] && group_by.empty? && self.entry_union + super + end + ## # Generates SqlStatement that maps time_entries and cost_entries to a common structure. # @@ -78,7 +93,9 @@ class CostQuery::SqlStatement < Report::SqlStatement # # @return [CostQuery::SqlStatement] Generated statement def self.for_entries - new unified_entry(TimeEntry).union(unified_entry(CostEntry), "entries") + sql = new unified_entry(TimeEntry).union(unified_entry(CostEntry), "entries") + sql.entry_union = true + sql end end From d23297a57900fad70844b3dd7625d42ebe49be99 Mon Sep 17 00:00:00 2001 From: Christian Rijke Date: Tue, 1 Oct 2013 16:23:44 +0200 Subject: [PATCH 2/2] Now that the result set is restricted to only the return columns as intended, 'author_id' must be tested separately as it is no longer part of the result set. --- spec/models/cost_query/filter_spec.rb | 80 +++++++++++++++++++++------ 1 file changed, 64 insertions(+), 16 deletions(-) diff --git a/spec/models/cost_query/filter_spec.rb b/spec/models/cost_query/filter_spec.rb index c5b730f4f8..550c08a89c 100644 --- a/spec/models/cost_query/filter_spec.rb +++ b/spec/models/cost_query/filter_spec.rb @@ -7,13 +7,13 @@ describe CostQuery, :reporting_query_helper => true do let!(:user) { FactoryGirl.create(:user, :member_in_project => project) } def create_work_package_with_entry(entry_type, work_package_params={}, entry_params = {}) - work_package_params = {:project => project}.merge!(work_package_params) - work_package = FactoryGirl.create(:work_package, work_package_params) - entry_params = {:work_package => work_package, - :project => work_package_params[:project], - :user => user}.merge!(entry_params) - FactoryGirl.create(entry_type, entry_params) - work_package + work_package_params = {:project => project}.merge!(work_package_params) + work_package = FactoryGirl.create(:work_package, work_package_params) + entry_params = {:work_package => work_package, + :project => work_package_params[:project], + :user => user}.merge!(entry_params) + FactoryGirl.create(entry_type, entry_params) + work_package end describe CostQuery::Filter do @@ -37,10 +37,11 @@ describe CostQuery, :reporting_query_helper => true do end end + # Test Work Package attributes that are included in of the result set + [ [CostQuery::Filter::ProjectId, 'project', "project_id", 2], [CostQuery::Filter::UserId, 'user', "user_id", 2], - [CostQuery::Filter::AuthorId, 'author', "author_id", 2], [CostQuery::Filter::CostTypeId, 'cost_type', "cost_type_id", 1], [CostQuery::Filter::WorkPackageId, 'work_package', "work_package_id", 2], [CostQuery::Filter::ActivityId, 'activity', "activity_id", 1], @@ -50,17 +51,17 @@ describe CostQuery, :reporting_query_helper => true do let!(:object) { send(object_name) } let!(:author) { FactoryGirl.create(:user, :member_in_project => project) } let!(:work_package) { FactoryGirl.create(:work_package, :project => project, - :author => author) } + :author => author) } let!(:cost_type) { FactoryGirl.create(:cost_type) } let!(:cost_entry) { FactoryGirl.create(:cost_entry, :work_package => work_package, - :user => user, - :project => project, - :cost_type => cost_type) } + :user => user, + :project => project, + :cost_type => cost_type) } let!(:activity) { FactoryGirl.create(:time_entry_activity) } let!(:time_entry) { FactoryGirl.create(:time_entry, :work_package => work_package, - :user => user, - :project => project, - :activity => activity) } + :user => user, + :project => project, + :activity => activity) } it "should only return entries from the given #{filter.to_s}" do @query.filter field, :value => object.id @@ -90,6 +91,53 @@ describe CostQuery, :reporting_query_helper => true do end end + # Test author attribute separately as it is not included in the result set + + describe CostQuery::Filter::AuthorId do + let!(:non_matching_entry) { FactoryGirl.create(:cost_entry) } + let!(:author) { FactoryGirl.create(:user, :member_in_project => project) } + let!(:work_package) { FactoryGirl.create(:work_package, :project => project, + :author => author) } + let!(:cost_type) { FactoryGirl.create(:cost_type) } + let!(:cost_entry) { FactoryGirl.create(:cost_entry, :work_package => work_package, + :user => user, + :project => project, + :cost_type => cost_type) } + let!(:activity) { FactoryGirl.create(:time_entry_activity) } + let!(:time_entry) { FactoryGirl.create(:time_entry, :work_package => work_package, + :user => user, + :project => project, + :activity => activity) } + + it "should only return entries from the given CostQuery::Filter::AuthorId" do + @query.filter 'author_id', :value => author.id + @query.result.each do |result| + work_package_id = result["work_package_id"] + WorkPackage.find(work_package_id).author.id.should == author.id + end + end + + it "should allow chaining the same filter" do + @query.filter 'author_id', :value => author.id + @query.filter 'author_id', :value => author.id + @query.result.each do |result| + work_package_id = result["work_package_id"] + WorkPackage.find(work_package_id).author.id.should == author.id + end + end + + it "should return no results for excluding filters" do + @query.filter 'author_id', :value => author.id + @query.filter 'author_id', :value => author.id + 1 + @query.result.count.should == 0 + end + + it "should compute the correct number of results" do + @query.filter 'author_id', :value => author.id + @query.result.count.should == 2 + end + end + it "filters spent_on" do @query.filter :spent_on, :operator=> 'w' @query.result.count.should == Entry.all.select { |e| e.spent_on.cweek == TimeEntry.all.first.spent_on.cweek }.count @@ -283,7 +331,7 @@ describe CostQuery, :reporting_query_helper => true do it "should create classes for custom fields that get added after starting the server" do custom_field - expect { class_name_for('My custom field').constantize }.to_not raise_error + expect { class_name_for('My custom field').constantize }.not_to raise_error end it "should remove the custom field classes after it is deleted" do