Nick, I think you have a good start on this ticket, but more work is needed. Please see my comments inline...
On Tue, Oct 5, 2010 at 4:15 PM, Nick Lewis <[email protected]> wrote: > While validation was already present, deserializing the report > during the before_validation step of assigning certain parameters > was failing, before the validation even got the chance to protect > anything. This patch simply uses a blank report until validation, > when it then properly fails. It also logs errors and will return > 406 Not Acceptable for an invalid report. [...] > diff --git a/app/controllers/reports_controller.rb > b/app/controllers/reports_controller.rb > index 1a48456..068ff1a 100644 > --- a/app/controllers/reports_controller.rb > +++ b/app/controllers/reports_controller.rb > @@ -10,7 +10,12 @@ class ReportsController < InheritedResources::Base > return > end > > - create! > + create! do |success,failure| > + failure.html do > + �[email protected]_messages.each { |msg| Rails.logger.debug msg } This creates multiple error messages without any context, which isn't useful for someone reading the log. I'd rather see something similar to: Rails.logger.warn("WARNING! ReportsController#create failed: #{joined error messages here}") Which might create a useful log message like: WARNING! ReportsController#create failed: The report is in an invalid format > 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'd like to see tests on this too. Thanks, -igal -- 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.
