From 34e1b6adddc89116e9358bc6672ad4ea30911490 Mon Sep 17 00:00:00 2001 From: Ariel Juodziukynas Date: Wed, 18 Oct 2023 11:58:04 -0300 Subject: [PATCH 1/2] Use conventions of Rails to require and validate attributes --- app/controllers/reports_controller.rb | 56 ++------- app/models/report.rb | 18 ++- app/views/reports/show.html.erb | 2 +- test/controllers/reports_controller_test.rb | 127 +++++++++----------- test/integration/routing_test.rb | 18 ++- 5 files changed, 96 insertions(+), 125 deletions(-) diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index cabb773..345dfa0 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -3,50 +3,10 @@ class ReportsController < ApplicationController 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 - - unless needed.empty? - head 400 - return - end - end + rep = Report.create! report_params - 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 +15,8 @@ def create rep.save render json: { id: rep.short_id } + rescue ActionController::ParameterMissing, ActiveRecord::RecordInvalid + head 400 end def show @@ -64,7 +26,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 +40,12 @@ 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 end diff --git a/app/models/report.rb b/app/models/report.rb index 216920f..33eff1a 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -1,10 +1,11 @@ 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, JSON + alias_attribute :entries, :report + validate :validate_entries_attributes def short_id int_val = self.id @@ -28,4 +29,15 @@ def self.find_from_short_id(base58_val) Report.find int_val end + + private + def validate_entries_attributes + valid_entries = entries.all? do |entry| + REQUIRED_ENTRY_ATTRIBUTES.all? do |attr| + entry[attr].present? + end + end + + errors.add(:entries, "are missing attributes") unless valid_entries + 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/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 From 83f4e4ad6d5ef01e52d9213fa74d0df872e323a7 Mon Sep 17 00:00:00 2001 From: Ariel Juodziukynas Date: Wed, 18 Oct 2023 16:42:38 -0300 Subject: [PATCH 2/2] Improve validation message and tests --- app/controllers/reports_controller.rb | 2 + app/models/report.rb | 23 +++++++++--- test/models/report_test.rb | 54 +++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 5 deletions(-) create mode 100644 test/models/report_test.rb diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 345dfa0..6e53ae1 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -1,4 +1,6 @@ class ReportsController < ApplicationController + wrap_parameters false + include ReportsHelper protect_from_forgery :except => [:create] diff --git a/app/models/report.rb b/app/models/report.rb index 33eff1a..9f46b35 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -5,6 +5,7 @@ class Report < ActiveRecord::Base serialize :report, JSON alias_attribute :entries, :report + validates :entries, presence: true validate :validate_entries_attributes def short_id @@ -32,12 +33,24 @@ def self.find_from_short_id(base58_val) private def validate_entries_attributes - valid_entries = entries.all? do |entry| - REQUIRED_ENTRY_ATTRIBUTES.all? do |attr| - entry[attr].present? - end + 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 - errors.add(:entries, "are missing attributes") unless valid_entries + "#{entry} (#{missing_attributes.join(", ")})" end end 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