Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

First refactor of Report #298

Merged
merged 6 commits into from

3 participants

Bart ten Brinke Justin Neil Matatall
Bart ten Brinke

Because you challenged me to tackle report.rb.. here goes :

  • The initialisers have been moved to their own files
  • The renderer is now a separate object, making refactoring easier and the namespace less polluted
  • Thanks to the renderer refactor it is now possible to refactor common methods. I've refactored array rendering into a single function, cleaning up the code a lot. This saves about 25% lines of code.

This new structure opens up possibilities to do more refactoring.

barttenbrinke added some commits
Bart ten Brinke barttenbrinke Moved initialisers to separate initialiser directory 5a650b2
Bart ten Brinke barttenbrinke Move renderer to separate object
 - Use locals instead of global binding
 - This opens up the possibility to clean up methods, because rendering and variables can now be seperated.
2b071f2
Bart ten Brinke barttenbrinke Refactored array rendering into a separate function.
 - Cleaned up some ifs
 -  Inlined some functions
15b2374
lib/brakeman/intializers/ok_json.rb
@@ -0,0 +1,9 @@
+#This is so OkJson will work with symbol values
+if mj_engine == :ok_json
Justin Owner

How would this variable get here? I think you could combine this file with multi_json.rb

Copy paste error :x Will fix tomorrow :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Justin

Hi Bart,

Thanks for your contributions!

How do you feel about moving everything to do with reports (templates, renderer, initializers) into a report directory? I'm reluctant to add more files to the lib/brakeman/ directory.

Also, you can see this is failing on Ruby 1.8.7.

Thanks again! :gem:

Bart ten Brinke

Fixed! And Travis agrees.

Justin

Thanks! @oreoshake thoughts?

Neil Matatall

MUCH better. I'd still like to see each report type in it's own class/module, but that's definitely not a blocker.

:walking:

Bart ten Brinke

I believe in doing these things in really small increments. You can't pull this from F to A, because it will affect a lot of other files in the code. I think I've removed the major issues that prevented people from cleaning it up at all (renderer & initializers), so that it can now move forward in the right direction .. it is now possible to write a report class / module that would make sense :)

Justin presidentbeef merged commit d42af6c into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 21, 2013
  1. Bart ten Brinke
  2. Bart ten Brinke

    Move renderer to separate object

    barttenbrinke authored
     - Use locals instead of global binding
     - This opens up the possibility to clean up methods, because rendering and variables can now be seperated.
  3. Bart ten Brinke

    Refactored array rendering into a separate function.

    barttenbrinke authored
     - Cleaned up some ifs
     -  Inlined some functions
Commits on Mar 22, 2013
  1. Bart ten Brinke
  2. Bart ten Brinke
  3. Bart ten Brinke
This page is out of date. Refresh to see the latest.
211 lib/brakeman/report.rb
View
@@ -7,42 +7,8 @@
require "csv"
require 'multi_json'
require 'brakeman/version'
-
-if CSV.const_defined? :Reader
- # Ruby 1.8 compatible
- require 'fastercsv'
- Object.send(:remove_const, :CSV)
- CSV = FasterCSV
-else
- # CSV is now FasterCSV in ruby 1.9
-end
-
-#MultiJson interface changed in 1.3.0, but need
-#to support older MultiJson for Rails 3.1.
-if MultiJson.respond_to? :default_adapter
- mj_engine = MultiJson.default_adapter
-else
- mj_engine = MultiJson.default_engine
-
- module MultiJson
- def self.dump *args
- encode *args
- end
-
- def self.load *args
- decode *args
- end
- end
-end
-
-#This is so OkJson will work with symbol values
-if mj_engine == :ok_json
- class Symbol
- def to_json
- self.to_s.inspect
- end
- end
-end
+require 'brakeman/report/renderer'
+Dir[File.dirname(__FILE__) + 'report/initializers/*.rb'].each {|file| require file}
#Generates a report based on the Tracker and the results of
#Tracker#run_checks. Be sure to +run_checks+ before generating
@@ -71,7 +37,14 @@ def generate_overview html = false
warnings = all_warnings.length
if html
- load_and_render_erb('overview', binding)
+ locals = {
+ :tracker => tracker,
+ :warnings => warnings,
+ :warnings_summary => warnings_summary,
+ :number_of_templates => number_of_templates(@tracker),
+ }
+
+ Brakeman::Report::Renderer.new('overview', :locals => locals).render
else
Terminal::Table.new(:headings => ['Scanned/Reported', 'Total']) do |t|
t.add_row ['Controllers', tracker.controllers.length]
@@ -87,34 +60,26 @@ def generate_overview html = false
def generate_warning_overview html = false
types = warnings_summary.keys
types.delete :high_confidence
+ values = types.sort.collect{|warning_type| [warning_type, warnings_summary[warning_type]] }
+ locals = {:types => types, :warnings_summary => warnings_summary}
- unless types.empty?
- if html
- load_and_render_erb('warning_overview', binding)
- else
- Terminal::Table.new(:headings => ['Warning Type', 'Total']) do |t|
- types.sort.each do |warning_type|
- t.add_row [warning_type, warnings_summary[warning_type]]
- end
- end
- end
- end
+ render_array('warning_overview', ['Warning Type', 'Total'], values, locals, html)
end
#Generate table of errors or return nil if no errors
def generate_errors html = false
- if tracker.errors.any?
- if html
- load_and_render_erb('error_overview', binding)
- else
- Terminal::Table.new(:headings => ['Error', 'Location']) do |t|
- tracker.errors.each do |error|
- t.add_row [error[:error], error[:backtrace][0]]
- end
- end
- end
+ values = tracker.errors.collect{|error| [error[:error], error[:backtrace][0]]}
+ render_array('error_overview', ['Error', 'Location'], values, {:tracker => tracker}, html)
+ end
+
+ def render_array(template, headings, value_array, locals, html = false)
+ return if value_array.empty?
+ if html
+ Brakeman::Report::Renderer.new(template, :locals => locals).render
else
- nil
+ Terminal::Table.new(:headings => headings) do |t|
+ value_array.each { |value_row| t.add_row value_row }
+ end
end
end
@@ -139,17 +104,9 @@ def generate_warnings html = false
stabilizer = 0
warning_messages = warning_messages.sort_by{|row| stabilizer += 1; [row['Confidence'], row['Warning Type'], row['Class'], stabilizer]}
- unless warning_messages.empty?
- if html
- load_and_render_erb('security_warnings', binding)
- else
- Terminal::Table.new(:headings => ["Confidence", "Class", "Method", "Warning Type", "Message"]) do |t|
- warning_messages.each do |row|
- t.add_row [row["Confidence"], row["Class"], row["Method"], row["Warning Type"], row["Message"]]
- end
- end
- end
- end
+ locals = {:warning_messages => warning_messages}
+ values = warning_messages.collect{|row| [row["Confidence"], row["Class"], row["Method"], row["Warning Type"], row["Message"]] }
+ render_array('security_warnings', ["Confidence", "Class", "Method", "Warning Type", "Message"], values, locals, html)
end
#Generate table of template warnings or return nil if no warnings
@@ -177,15 +134,10 @@ def generate_template_warnings html = false
stabilizer = 0
warnings = warnings.sort_by{|row| stabilizer += 1; [row["Confidence"], row["Warning Type"], row["Template"], stabilizer]}
- if html
- load_and_render_erb('view_warnings', binding)
- else
- Terminal::Table.new(:headings => ["Confidence", "Template", "Warning Type", "Message"]) do |t|
- warnings.each do |warning|
- t.add_row [warning["Confidence"], warning["Template"], warning["Warning Type"], warning["Message"]]
- end
- end
- end
+
+ locals = {:warnings => warnings}
+ values = warnings.collect{|warning| [warning["Confidence"], warning["Template"], warning["Warning Type"], warning["Message"]] }
+ render_array('view_warnings', ["Confidence", "Template", "Warning Type", "Message"], values, locals, html)
else
nil
end
@@ -214,15 +166,9 @@ def generate_model_warnings html = false
stabilizer = 0
warnings = warnings.sort_by{|row| stabilizer +=1; [row["Confidence"],row["Warning Type"], row["Model"], stabilizer]}
- if html
- load_and_render_erb('model_warnings', binding)
- else
- Terminal::Table.new(:headings => ["Confidence", "Model", "Warning Type", "Message"]) do |t|
- warnings.each do |warning|
- t.add_row [warning["Confidence"], warning["Model"], warning["Warning Type"], warning["Message"]]
- end
- end
- end
+ locals = {:warnings => warnings}
+ values = warnings.collect{|warning| [warning["Confidence"], warning["Model"], warning["Warning Type"], warning["Message"]] }
+ render_array('model_warnings', ["Confidence", "Model", "Warning Type", "Message"], values, locals, html)
else
nil
end
@@ -252,15 +198,9 @@ def generate_controller_warnings html = false
stabilizer = 0
warnings = warnings.sort_by{|row| stabilizer +=1; [row["Confidence"], row["Warning Type"], row["Controller"], stabilizer]}
- if html
- load_and_render_erb('controller_warnings', binding)
- else
- Terminal::Table.new(:headings => ["Confidence", "Controller", "Warning Type", "Message"]) do |t|
- warnings.each do |warning|
- t.add_row [warning["Confidence"], warning["Controller"], warning["Warning Type"], warning["Message"]]
- end
- end
- end
+ locals = {:warnings => warnings}
+ values = warnings.collect{|warning| [warning["Confidence"], warning["Controller"], warning["Warning Type"], warning["Message"]] }
+ render_array('controller_warnings', ["Confidence", "Controller", "Warning Type", "Message"], values, locals, html)
else
nil
end
@@ -301,15 +241,9 @@ def generate_controllers html=false
end
controller_rows = controller_rows.sort_by{|row| row['Name']}
- if html
- load_and_render_erb('controller_overview', binding)
- else
- Terminal::Table.new(:headings => ['Name', 'Parent', 'Includes', 'Routes']) do |t|
- controller_rows.each do |row|
- t.add_row [row['Name'], row['Parent'], row['Includes'], row['Routes']]
- end
- end
- end
+ locals = {:controller_rows => controller_rows}
+ values = controller_rows.collect{|row| [row['Name'], row['Parent'], row['Includes'], row['Routes']] }
+ render_array('controller_overview', ['Name', 'Parent', 'Includes', 'Routes'], values, locals, html)
end
#Generate listings of templates and their output
@@ -330,7 +264,7 @@ def generate_templates html = false
template_rows = template_rows.sort_by{|name, value| name.to_s}
if html
- load_and_render_erb('template_overview', binding)
+ Brakeman::Report::Renderer.new('template_overview', :locals => {:template_rows => template_rows}).render
else
output = ''
template_rows.each do |template|
@@ -356,24 +290,15 @@ def to_html
generate_warning_overview(true).to_s
# Return early if only summarizing
- if tracker.options[:summary_only]
- return out
- end
-
- if tracker.options[:report_routes] or tracker.options[:debug]
- out << generate_controllers(true).to_s
- end
-
- if tracker.options[:debug]
- out << generate_templates(true).to_s
- end
+ return out if tracker.options[:summary_only]
+ out << generate_controllers(true).to_s if tracker.options[:report_routes] or tracker.options[:debug]
+ out << generate_templates(true).to_s if tracker.options[:debug]
out << generate_errors(true).to_s
out << generate_warnings(true).to_s
out << generate_controller_warnings(true).to_s
out << generate_model_warnings(true).to_s
out << generate_template_warnings(true).to_s
-
out << "</body></html>"
end
@@ -385,9 +310,7 @@ def to_s
truncate_table(generate_warning_overview.to_s) << "\n"
#Return output early if only summarizing
- if tracker.options[:summary_only]
- return out
- end
+ return out if tracker.options[:summary_only]
if tracker.options[:report_routes] or tracker.options[:debug]
out << "\n+CONTROLLERS+\n" <<
@@ -469,13 +392,9 @@ def to_pdf
end
def rails_version
- if version = tracker.config[:rails_version]
- return version
- elsif tracker.options[:rails3]
- return "3.x"
- else
- return "Unknown"
- end
+ return tracker.config[:rails_version] if tracker.config[:rails_version]
+ return "3.x" if tracker.options[:rails3]
+ "Unknown"
end
#Return header for HTML output. Uses CSS from tracker.options[:html_style]
@@ -486,7 +405,15 @@ def html_header
raise "Cannot find CSS stylesheet for HTML: #{tracker.options[:html_style]}"
end
- load_and_render_erb('header', binding)
+ locals = {
+ :css => css,
+ :tracker => tracker,
+ :checks => checks,
+ :rails_version => rails_version,
+ :brakeman_version => Brakeman::Version
+ }
+
+ Brakeman::Report::Renderer.new('header', :locals => locals).render
end
#Generate header for text output
@@ -519,13 +446,9 @@ def warnings_summary
high_confidence_warnings = 0
[all_warnings].each do |warnings|
-
warnings.each do |warning|
summary[warning.warning_type.to_s] += 1
-
- if warning.confidence == 0
- high_confidence_warnings += 1
- end
+ high_confidence_warnings += 1 if warning.confidence == 0
end
end
@@ -549,7 +472,6 @@ def html_message warning, message
if @highlight_user_input and warning.user_input
user_input = CGI.escapeHTML(warning.format_user_input)
-
message.gsub!(user_input, "<span class=\"user_input\">#{user_input}</span>")
end
@@ -561,19 +483,13 @@ def with_context warning, message
context = context_for(@app_tree, warning)
full_message = nil
- if tracker.options[:message_limit] and
- tracker.options[:message_limit] > 0 and
- message.length > tracker.options[:message_limit]
-
+ if tracker.options[:message_limit] and tracker.options[:message_limit] > 0 and message.length > tracker.options[:message_limit]
full_message = html_message(warning, message)
message = message[0..tracker.options[:message_limit]] << "..."
end
message = html_message(warning, message)
-
- if context.empty? and not full_message
- return message
- end
+ return message if context.empty? and not full_message
@element_id += 1
code_id = "context#@element_id"
@@ -736,11 +652,4 @@ def warning_file warning, relative = false
end
end
- private
-
- def load_and_render_erb file, bind
- content = File.read(File.expand_path("templates/#{file}.html.erb", File.dirname(__FILE__)))
- template = ERB.new(content)
- template.result(bind)
- end
end
7 lib/brakeman/report/intializers/faster_csv.rb
View
@@ -0,0 +1,7 @@
+# Ruby 1.8 compatible
+if CSV.const_defined? :Reader
+ require 'fastercsv'
+ Object.send(:remove_const, :CSV)
+ CSV = FasterCSV
+end
+
29 lib/brakeman/report/intializers/multi_json.rb
View
@@ -0,0 +1,29 @@
+#MultiJson interface changed in 1.3.0, but need
+#to support older MultiJson for Rails 3.1.
+mj_engine = nil
+
+if MultiJson.respond_to? :default_adapter
+ mj_engine = MultiJson.default_adapter
+else
+ mj_engine = MultiJson.default_engine
+
+ module MultiJson
+ def self.dump *args
+ encode *args
+ end
+
+ def self.load *args
+ decode *args
+ end
+ end
+end
+
+#This is so OkJson will work with symbol values
+if mj_engine == :ok_json
+ class Symbol
+ def to_json
+ self.to_s.inspect
+ end
+ end
+end
+
22 lib/brakeman/report/renderer.rb
View
@@ -0,0 +1,22 @@
+class Brakeman::Report
+ class Renderer
+ def initialize(template_file, hash = {})
+ hash[:locals] ||= {}
+ singleton = class << self; self end
+
+ hash[:locals].each do |attribute_name, attribute_value|
+ singleton.send(:define_method, attribute_name) { attribute_value }
+ end
+
+ # There are last, so as to make overwriting these using locals impossible.
+ singleton.send(:define_method, 'template_file') { template_file }
+ singleton.send(:define_method, 'template') {
+ File.read(File.expand_path("templates/#{template_file}.html.erb", File.dirname(__FILE__)))
+ }
+ end
+
+ def render
+ ERB.new(template).result(binding)
+ end
+ end
+end
0  lib/brakeman/templates/controller_overview.html.erb → ...man/report/templates/controller_overview.html.erb
View
File renamed without changes
0  lib/brakeman/templates/controller_warnings.html.erb → ...man/report/templates/controller_warnings.html.erb
View
File renamed without changes
0  lib/brakeman/templates/error_overview.html.erb → ...brakeman/report/templates/error_overview.html.erb
View
File renamed without changes
4 lib/brakeman/templates/header.html.erb → lib/brakeman/report/templates/header.html.erb
View
@@ -11,7 +11,7 @@
elem.style.display = "block";
else
elem.style.display = "none";
-
+
elem.parentNode.scrollIntoView();
}
</script>
@@ -33,7 +33,7 @@
<tr>
<td><%= File.expand_path tracker.options[:app_path] %></td>
<td><%= rails_version %></td>
- <td><%= Brakeman::Version %>
+ <td><%= brakeman_version %>
<td>
<%= tracker.start_time %><br><br>
<%= tracker.duration %> seconds
0  lib/brakeman/templates/model_warnings.html.erb → ...brakeman/report/templates/model_warnings.html.erb
View
File renamed without changes
4 lib/brakeman/templates/overview.html.erb → lib/brakeman/report/templates/overview.html.erb
View
@@ -1,4 +1,4 @@
-<h2 id='summary'>Summary</h2>
+<h2 id='summary'>Summary</h2>
<table>
<tr>
<th>Scanned/Reported</th>
@@ -14,7 +14,7 @@
</tr>
<tr>
<td>Templates</td>
- <td><%= number_of_templates(@tracker) %></td>
+ <td><%= number_of_templates %></td>
</tr>
<tr>
<td>Errors</td>
0  lib/brakeman/templates/security_warnings.html.erb → ...keman/report/templates/security_warnings.html.erb
View
File renamed without changes
0  lib/brakeman/templates/template_overview.html.erb → ...keman/report/templates/template_overview.html.erb
View
File renamed without changes
0  lib/brakeman/templates/view_warnings.html.erb → lib/brakeman/report/templates/view_warnings.html.erb
View
File renamed without changes
0  lib/brakeman/templates/warning_overview.html.erb → ...akeman/report/templates/warning_overview.html.erb
View
File renamed without changes
Something went wrong with that request. Please try again.