diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index cabb773..641d532 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -1,52 +1,16 @@ class ReportsController < ApplicationController + before_action :fix_missing_json_content_type + + wrap_parameters false + include ReportsHelper protect_from_forgery :except => [:create] - DATA_KEY = %W!central_tendency error name ips stddev microseconds iterations cycles! - def create - data = request.body.read - - begin - input = JSON.parse data - rescue Exception => err - logger.fatal("Error: #{err.message} || #{data}") - head 400 - return - end - - ary = input["entries"] - - unless ary.kind_of? Array - head 400 - return - end - - ary.each do |j| - needed = DATA_KEY.dup - - j.keys.each do |k| - if DATA_KEY.include? k - needed.delete k - else - head 400 - return - end - end + rep = Report.create! report_params - unless needed.empty? - head 400 - return - end - end - - rep = Report.create report: JSON.generate(ary), - ruby: input["ruby"], - os: input["os"], - arch: input["arch"] - - options = input["options"] || {} + options = params["options"] || {} if options["compare"] rep.compare = true @@ -55,6 +19,8 @@ def create rep.save render json: { id: rep.short_id } + rescue ActionController::ParameterMissing, ActiveRecord::RecordInvalid + head 400 end def show @@ -64,7 +30,7 @@ def show fastest_val = nil note_high_stddev = false - @report.data.each do |part| + @report.entries.each do |part| if !fastest_val || part["ips"] > fastest_val fastest = part fastest_val = part["ips"] @@ -78,4 +44,31 @@ def show @note_high_stddev = note_high_stddev @fastest = fastest end + + private + def report_params + entries_params = params.require(:entries).map do |entry| + entry.permit(:name, :ips, :central_tendency, :error, :stddev, :microseconds, :iterations, :cycles) + end + params.permit(:ruby, :os, :arch).merge(report: entries_params) + end + + # benchmark-ips sends the JSON string in the request body but it does not specify a content type + # when that happens, Rails parses it incorrectly and produces a params key `"{\"entries\":"` + # + # we can detect this and fix the params object to properly parse the request body as JSON and update + # the `params` object + def fix_missing_json_content_type + if params.keys.include?("{\"entries\":") && request.body.present? + begin + json = JSON.parse(request.body.read) + params.delete("{\"entries\":") + params.merge!(json) + rescue JSON::ParserError + # Body was not valid JSON, do nothing + ensure + request.body.rewind + end + end + end end diff --git a/app/models/report.rb b/app/models/report.rb index 216920f..80bd651 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -1,10 +1,12 @@ class Report < ActiveRecord::Base ALPHABET = "123456789abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ" BASE = ALPHABET.length + REQUIRED_ENTRY_ATTRIBUTES = %W(name ips stddev microseconds iterations cycles) - def data - JSON.parse report - end + serialize :report, coder: JSON + alias_attribute :entries, :report + validates :entries, presence: true + validate :validate_entries_attributes def short_id int_val = self.id @@ -28,4 +30,27 @@ def self.find_from_short_id(base58_val) Report.find int_val end + + private + def validate_entries_attributes + return if entries.blank? + + invalid_entries = entries.reject { |entry| valid_entry?(entry) } + + if invalid_entries.any? + errors.add(:entries, "missing attributes: #{ invalid_entries.map { |entry| error_message(entry) }.join(", ")}") + end + end + + def valid_entry?(entry) + REQUIRED_ENTRY_ATTRIBUTES.all? do |attr| + entry[attr].present? + end + end + + def error_message(entry) + missing_attributes = REQUIRED_ENTRY_ATTRIBUTES - entry.keys + + "#{entry} (#{missing_attributes.join(", ")})" + end end diff --git a/app/views/reports/show.html.erb b/app/views/reports/show.html.erb index b7beb8e..75cf7d7 100644 --- a/app/views/reports/show.html.erb +++ b/app/views/reports/show.html.erb @@ -7,7 +7,7 @@ <% end %> iterations/second - <% @report.data.each do |part| %> + <% @report.entries.each do |part| %> <% if part["name"] == @fastest["name"] %> diff --git a/test/controllers/reports_controller_test.rb b/test/controllers/reports_controller_test.rb index 8f9b34c..ffb6f24 100644 --- a/test/controllers/reports_controller_test.rb +++ b/test/controllers/reports_controller_test.rb @@ -1,7 +1,7 @@ -require 'test_helper' +require "test_helper" class ReportsControllerTest < ActionController::TestCase - test "validates the data json" do + test "errors if body is not json" do data = "nope" post :create, body: data @@ -10,23 +10,20 @@ class ReportsControllerTest < ActionController::TestCase end test "validates the data json as valid benchmark data" do - data = <<-DATA -{ - "entries": - [{ - "name": "test", - "ips": 10.1, - "central_tendency": 10.1, - "error": 23666, - "stddev": 0.3, - "microseconds": 3322, - "iterations": 221, - "cycles": 16 - }] -} - DATA + data = { + entries: [{ + name: "test", + ips: 10.1, + central_tendency: 10.1, + error: 23666, + stddev: 0.3, + microseconds: 3322, + iterations: 221, + cycles: 16 + }] + } - post :create, body: data + post :create, params: data, as: :json assert_equal "200", @response.code @@ -34,77 +31,71 @@ class ReportsControllerTest < ActionController::TestCase report = Report.find_from_short_id rep["id"] -raw = <<-DATA - [{ - "name": "test", - "ips": 10.1, - "central_tendency": 10.1, - "error": 23666, - "stddev": 0.3, - "microseconds": 3322, - "iterations": 221, - "cycles": 16 - }] -DATA - - assert_equal JSON.parse(raw), report.data + raw = <<~DATA + [{ + "name": "test", + "ips": 10.1, + "central_tendency": 10.1, + "error": 23666, + "stddev": 0.3, + "microseconds": 3322, + "iterations": 221, + "cycles": 16 + }] + DATA + + assert_equal JSON.parse(raw), report.entries end test "errors on unknown data keys" do - data = <<-DATA -{ - "entries": - [{ - "name": "test", - "ipx": 10.1, - "stddev": 0.3, - "microseconds": 3322, - "iterations": 221, - "cycles": 16 - }] -} - DATA + data = { + entries: [{ + name: "test", + ipx: 10.1, + stddev: 0.3, + microseconds: 3322, + iterations: 221, + cycles: 16 + }] + } - post :create, body: data + post :create, params: data, as: :json assert_equal "400", @response.code end test "errors out if there are keys missing" do - data = <<-DATA -{ - "entries": - [{ - "name": "test" - }] -} - DATA + data = { + entries: [{ + name: "test" + }] + } - post :create, body: data + post :create, params: data, as: :json assert_equal "400", @response.code end test "environment variables are sent, processed, and saved" do data = { - "ruby" => "3.2.1", - "os" => "Linux", - "arch" => "x86_64", - "entries" => [ + ruby: "3.2.1", + os: "Linux", + arch: "x86_64", + entries: [ { - "name" => "test", - "ips" => 10.1, - "central_tendency" => 10.1, - "error" => 23666, - "stddev" => 0.3, - "microseconds" => 3322, - "iterations" => 221, - "cycles" => 16 + name: "test", + ips: 10.1, + central_tendency: 10.1, + error: 23666, + stddev: 0.3, + microseconds: 3322, + iterations: 221, + cycles: 16 } ] } - post :create, body: data.to_json + post :create, params: data, as: :json rep = JSON.parse @response.body diff --git a/test/integration/create_report_test.rb b/test/integration/create_report_test.rb new file mode 100644 index 0000000..f0978e8 --- /dev/null +++ b/test/integration/create_report_test.rb @@ -0,0 +1,13 @@ +require 'test_helper' + +class CreateReportTest < ActionDispatch::IntegrationTest + test "process json body with missing content_type" do + raw_json = "{\"entries\":[{\"name\":\"addition\",\"central_tendency\":27474451.540838767,\"ips\":27474451.540838767,\"error\":122038,\"stddev\":122038,\"microseconds\":1087174.973022461,\"iterations\":29868949,\"cycles\":2715359}],\"options\":{\"compare\":true}}" + + # at the point of writing this, benchmark-ips makes this request with a raw string and no content type + # this mimics that scenario + post "/reports", params: raw_json, headers: { "CONTENT_LENGTH" => raw_json.length } + + assert_equal 200, status + end +end diff --git a/test/integration/routing_test.rb b/test/integration/routing_test.rb index f595106..3009f53 100644 --- a/test/integration/routing_test.rb +++ b/test/integration/routing_test.rb @@ -2,16 +2,14 @@ class RoutingTest < ActionDispatch::IntegrationTest def test_short_id - data = <<-DATA -[{ - "name": "test", - "ips": 10.1, - "stddev": 0.3, - "microseconds": 3322, - "iterations": 221, - "cycles": 16 -}] - DATA + data = [{ + name: "test", + ips: 10.1, + stddev: 0.3, + microseconds: 3322, + iterations: 221, + cycles: 16 + }] report = Report.create! report: data diff --git a/test/models/report_test.rb b/test/models/report_test.rb new file mode 100644 index 0000000..a1eca0d --- /dev/null +++ b/test/models/report_test.rb @@ -0,0 +1,54 @@ +require 'test_helper' + + +class ReportTest < ActiveSupport::TestCase + test "validates required attributes" do + report = Report.new({ + entries: [{ + name: "test", + ips: 10.1, + central_tendency: 10.1, + error: 23666, + stddev: 0.3, + microseconds: 3322, + iterations: 221, + cycles: 16 + }] + }) + + assert report.valid? + end + + test "requires entries" do + report = Report.new() + + assert report.invalid? + + assert_equal report.errors[:entries], ["can't be blank"] + end + + test "shows invalid entries if validation fails" do + report = Report.new({ + entries: [{ + name: "test", + central_tendency: 10.1, + error: 23666, + microseconds: 3322, + iterations: 221, + cycles: 16 + }, + { + name: "test2", + central_tendency: 10.1, + microseconds: 3322, + iterations: 221, + ips: 4, + cycles: 16 + }] + }) + + assert report.invalid? + + assert_equal report.errors[:entries], ['missing attributes: {"name"=>"test", "central_tendency"=>10.1, "error"=>23666, "microseconds"=>3322, "iterations"=>221, "cycles"=>16} (ips, stddev), {"name"=>"test2", "central_tendency"=>10.1, "microseconds"=>3322, "iterations"=>221, "ips"=>4, "cycles"=>16} (stddev)'] + end +end