From 253cf7a55f89891c0e0b34be25c699e56e2f0854 Mon Sep 17 00:00:00 2001
From: Nick LaMuro <nicklamuro@gmail.com>
Date: Fri, 15 Jun 2018 16:47:47 -0500
Subject: [PATCH] Add save hooks on MiqReportResult to remove groupings

https://bugzilla.redhat.com/show_bug.cgi?id=1590908

The `report.extras[:groupings]` on MiqReportResult were being
serialized to the `report` column, and on certain chargeback reports,
can get quite large.  This data is not needed, so we remove it prior to
saving.

The `after_commit` exists in case there is a use for the saved data after
it has been saved and still used within the original instance of the
MiqReportResult for building the result.  Most likely this is overkill,
but for now it is in place as a safety measure.

Also, `after_commit` is used because a `after_save` will cause an
"stack level too deep" error since editing the report value causes the
record to be put into a `dirty` state, and the ActiveRecord internals
don't like that.
---
 app/models/miq_report_result.rb       | 14 ++++++++++++++
 spec/models/miq_report_result_spec.rb | 17 +++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/app/models/miq_report_result.rb b/app/models/miq_report_result.rb
index 60710ee76e8..a8d63112866 100644
--- a/app/models/miq_report_result.rb
+++ b/app/models/miq_report_result.rb
@@ -35,6 +35,20 @@ class MiqReportResult < ApplicationRecord
     end
   end
 
+  # These two hooks prevent temporary chargeback aggregation data to be
+  # retained in each miq_report_results row, which was found and fixed as part
+  # of the following BZ:
+  #
+  #   https://bugzilla.redhat.com/show_bug.cgi?id=1590908
+  #
+  # These two hooks remove extra data on the `@extras` instance variable, since
+  # it is not necessary to be saved to the DB, but allows it to be retained for
+  # the remainder of the object's instantiation.  The `after_commit` hook
+  # specifically is probably overkill, but allows us to not break existing code
+  # that expects the temporary chargeback data in the instantiated object.
+  before_save  { @_extra_groupings = report.extras.delete(:grouping) if report.kind_of?(MiqReport) }
+  after_commit { report.extras[:grouping] = @_extra_groupings if report.kind_of?(MiqReport) }
+
   delegate :table, :to => :report_results, :allow_nil => true
 
   def result_set
diff --git a/spec/models/miq_report_result_spec.rb b/spec/models/miq_report_result_spec.rb
index 1904194d330..47c2f43efbd 100644
--- a/spec/models/miq_report_result_spec.rb
+++ b/spec/models/miq_report_result_spec.rb
@@ -105,6 +105,23 @@
       expect(report_result.report_results.table.data).not_to be_nil
     end
 
+    it "should not include `extras[:grouping]` in the report column" do
+      MiqReport.seed_report(name = "Vendor and Guest OS")
+      rpt = MiqReport.where(:name => name).last
+      rpt.generate_table(:userid => "test")
+      report_result = rpt.build_create_results(:userid => "test")
+
+      report_result.report
+      report_result.report.extras[:grouping] = { "extra data" => "not saved" }
+      report_result.save
+
+      result_reload = MiqReportResult.last
+
+      expect(report_result.report.kind_of?(MiqReport)).to be_truthy
+      expect(result_reload.report.extras[:grouping]).to be_nil
+      expect(report_result.report.extras[:grouping]).to eq("extra data" => "not saved")
+    end
+
     context "for miq_report_result is used different miq_group_id than user's current id" do
       before do
         MiqUserRole.seed