>> diff --git a/app/models/report.rb b/app/models/report.rb >> index 04c81b8..46e6728 100644 >> --- a/app/models/report.rb >> +++ b/app/models/report.rb > [...] >> @@ -24,7 +24,8 @@ class Report < ActiveRecord::Base >> end >> >> def report >> - rep = read_attribute(:report) >> + # Returning a blank report will let us fail gracefully for a bad report >> + rep = read_attribute(:report) rescue Puppet::Transaction::Report.new >> rep.extend(ReportExtensions) unless rep.nil? or >> rep.is_a?(ReportExtensions) >> rep >> end > This approach is problematic because it doesn't check the object it's > using, what type it is or whether it's valid. For example, if I set > the report attribute to a blank string or anything other than a > Puppet::Transaction::Report, this method and/or code that expected it > to work will fail in confusing ways. Also, instantiating a blank > placeholder Puppet::Transaction::Report object is dangerous because > then other code may fail in surprising ways because it may mistake it > for a valid report. > > It may be best for this #report method to either throw a general > TypeError or a custom InvalidReportFormatError if it can't get a valid > Puppet::Transaction::Report object and add exception handlers to code > relying on this method. For example, the > Report#report_contains_metrics method could catch this error and > return false, while the Report#set_attributes could catch this and set > the denormalized values for #success, #time and #host to nil because > it has nothing to set them to. > > An example of the trouble with this patch's approach is that > `Report.new.valid?` should return false, but instead throws a > "NoMethodError: undefined method `failed?' for nil:NilClass" because > the Report#set_attributes method assumes that the #report is always a > valid Puppet::Transaction::Report.
I had trouble working through exactly what your objection was here (perhaps a low-coffee problem). Are you basically saying that 1) rescuing errors is not sufficient because it doesn't handle cases where something other than a Puppet::Transaction::Report is returned from read_attribute and 2) minting a new Puppet::Transaction::Report won't fail gracefully because...and this part I'm still not clear on. -- Markus ----------------------------------------------------------- The power of accurate observation is commonly called cynicism by those who have not got it. ~George Bernard Shaw ------------------------------------------------------------ -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
