Merge pull request #15 from fastruby/Issue-14-more-conventional-rails

Refactor model and controller to follow Rails conventions

Author: etagwerker

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

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

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

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