FIX: Apply contract checks when first creating a badge

app/controllers/admin/badges_controller.rb

@@ -48,33 +48,22 @@ class Admin::BadgesController < Admin::AdminController
 
   def create
     badge = Badge.new
-    update_badge_from_params(badge)
-    badge.id = nil
-    badge.save!
-    render_serialized(badge, BadgeSerializer, root: "badge")
+    errors = update_badge_from_params(badge, new: true)
+
+    if errors.present?
+      render_json_error errors
+    else
+      render_serialized(badge, BadgeSerializer, root: "badge")
+    end
   end
 
   def update
     badge = find_badge
 
-    error = nil
-    Badge.transaction do
-      update_badge_from_params(badge)
+    errors = update_badge_from_params(badge)
 
-      # Perform checks to prevent bad queries
-      begin
-        BadgeGranter.contract_checks!(badge.query, { target_posts: badge.target_posts, trigger: badge.trigger })
-      rescue => e
-        # noinspection RubyUnusedLocalVariable
-        error = e.message
-        raise ActiveRecord::Rollback
-      end
-
-      badge.save!
-    end
-
-    if error
-      render_json_error error
+    if errors.present?
+      render_json_error errors
     else
       render_serialized(badge, BadgeSerializer, root: "badge")
     end
@@ -91,16 +80,36 @@ class Admin::BadgesController < Admin::AdminController
       Badge.find(params[:id])
     end
 
-    def update_badge_from_params(badge)
-      allowed = Badge.column_names.map(&:to_sym)
-      allowed -= [:id, :created_at, :updated_at, :grant_count]
-      allowed -= Badge.protected_system_fields if badge.system?
-      params.permit(*allowed)
+    # Options:
+    #   :new - reset the badge id to nil before saving
+    def update_badge_from_params(badge, opts={})
+      errors = []
+      Badge.transaction do
+        allowed = Badge.column_names.map(&:to_sym)
+        allowed -= [:id, :created_at, :updated_at, :grant_count]
+        allowed -= Badge.protected_system_fields if badge.system?
+        params.permit(*allowed)
 
-      allowed.each do |key|
-        badge.send("#{key}=" , params[key]) if params[key]
+        allowed.each do |key|
+          badge.send("#{key}=" , params[key]) if params[key]
+        end
+
+        # Badge query contract checks
+        begin
+          BadgeGranter.contract_checks!(badge.query, { target_posts: badge.target_posts, trigger: badge.trigger })
+        rescue => e
+          errors << [e.message]
+          raise ActiveRecord::Rollback
+        end
+
+        badge.id = nil if opts[:new]
+        badge.save!
       end
 
-      badge
+      if badge.errors
+        errors.push(*badge.errors.full_messages)
+      end
+
+      errors
     end
 end

app/services/badge_granter.rb

@@ -155,6 +155,7 @@ class BadgeGranter
     end
     raise "Contract violation:\nQuery does not return a 'user_id' column" unless sql.match /user_id/
     raise "Contract violation:\nQuery does not return a 'granted_at' column" unless sql.match /granted_at/
+    raise "Contract violation:\nQuery ends with a semicolon. Remove the semicolon; your sql will be used in a subquery." if sql.match /;\s*\z/
   end
 
   # Options:

lib/json_error.rb

@@ -11,6 +11,12 @@ module JsonError
     # If it looks like an activerecord object, extract its messages
     return {errors: obj.errors.full_messages } if obj.respond_to?(:errors) && obj.errors.present?
 
+    # If we're passed an array, it's an array of error messages
+    return {errors: obj.map {|e| e.to_s}} if obj.is_a?(Array) && obj.present?
+
+    # Log a warning (unless obj is nil)
+    Rails.logger.warn("create_errors_json called with unrecognized type: #{obj.inspect}") if obj
+
     # default to a generic error
     JsonError.generic_error
   end