Refactor model and controller to follow Rails conventions

app/controllers/reports_controller.rb

@@ -1,52 +1,14 @@
 class ReportsController < ApplicationController
+  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"]
+    rep = Report.create! report_params
 
-    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: 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 +17,8 @@ def create
     rep.save
 
     render json: { id: rep.short_id }
+  rescue ActionController::ParameterMissing, ActiveRecord::RecordInvalid
+    head 400
   end
 
   def show
@@ -64,7 +28,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 +42,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

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, 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

app/views/reports/show.html.erb

@@ -7,7 +7,7 @@
       <% end %>
       <th>iterations/second</th>
     </tr>
-    <% @report.data.each do |part| %>
+    <% @report.entries.each do |part| %>
       <tr>
         <td>
           <% if part["name"] == @fastest["name"] %>

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,101 +10,92 @@ 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
 
     rep = JSON.parse @response.body
 
     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
 

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
 

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