Merge pull request #1097 from RickCarlino/celery_errors

Friendlier errors when sequence variables are missing

app/controllers/api/abstract_controller.rb

@@ -1,3 +1,5 @@
+require "./app/lib/celery_script/checker"
+
 module Api
   # A controller that contains all of the helper methods and shared logic for
   # all API endpoints.
@@ -16,6 +18,10 @@ module Api
     skip_before_action :verify_authenticity_token
     after_action :skip_set_cookies_header
 
+    rescue_from(CeleryScript::TypeCheckError) do |err|
+      sorry err.message, 422
+    end
+
     rescue_from(ActionController::RoutingError) { sorry "Not found", 404 }
     rescue_from(User::AlreadyVerified) { sorry "Already verified.", 409 }
 

app/lib/celery_script/checker.rb

@@ -5,13 +5,24 @@ module CeleryScript
   class Checker
     MISSING_ARG = "Expected node '%s' to have a '%s', but got: %s."
     EXTRA_ARGS  = "'%s' has unexpected arguments: %s. Allowed arguments: %s"
-    BAD_LEAF    = "Expected leaf '%s' within '%s' to be one of: %s but got %s"
+    BAD_LEAF    = "Expected leaf '%{kind}' within '%{parent_kind}'"\
+                  " to be one of: %{allowed} but got %{actual}"
     MALFORMED   = "Expected '%s' to be a node or leaf, but it was neither"
     BAD_BODY    = "Body of '%s' node contains '%s' node. "\
                   "Expected one of: %s"
     UNBOUND_VAR = "Unbound variable: %s"
     T_MISMATCH  = "Type mismatch. %s must be one of: %s. Got: %s"
 
+    # Certain CeleryScript pairing errors are more than just a syntax error.
+    # For instance, A `nothing` node in a `variable_declaration` is often an
+    # indication that the user did not fill out a value for a variable. In these
+    # rare cases, we muct provide information beyond what is found in the
+    # BAD_LEAF template.
+    FRIENDLY_ERRORS = {
+      nothing: {
+        variable_declaration: "You must provide a value for all parameters"
+      }
+    }.with_indifferent_access
     attr_reader :tree, :corpus, :device
 
     # Device is required for security / permission checks.
@@ -43,11 +54,8 @@ module CeleryScript
     def check_leaf(node)
       allowed  = corpus.values(node)
       actual   = node.value.class
-      ok_leaf  = allowed.include?(actual)
-      raise TypeCheckError, (BAD_LEAF % [ node.kind,
-                                          node.parent.kind,
-                                          allowed.inspect,
-                                          actual.inspect]) unless ok_leaf
+
+      maybe_bad_leaf(node.kind, node.parent.kind, allowed, actual)
     end
 
     private
@@ -152,21 +160,27 @@ module CeleryScript
         #  SEE_MY_NOTE =============================^ RC 4 Oct 18
         end
       end
-      ok      = allowed.include?(actual)
-      raise TypeCheckError, (BAD_LEAF % [ value.kind,
-                                          value.parent.kind,
-                                          allowed.inspect,
-                                          actual.inspect ]) unless ok
+
+      maybe_bad_leaf(value.kind, value.parent.kind, allowed, actual)
+    end
+
+    def maybe_bad_leaf(kind, parent_kind, allowed, actual)
+      unless allowed.include?(actual)
+        message = (FRIENDLY_ERRORS.dig(kind, parent_kind) || BAD_LEAF) % {
+          kind:        kind,
+          parent_kind: parent_kind,
+          allowed:     allowed,
+          actual:      actual
+        }
+
+        raise TypeCheckError, message
+      end
     end
 
     def validate_leaf_pairing(key, value)
       actual  = value.value.class
       allowed = corpus.fetchArg(key).allowed_values
-      ok      = allowed.include?(actual)
-      raise TypeCheckError, (BAD_LEAF % [ value.kind,
-                                          value.parent.kind,
-                                          allowed.inspect,
-                                          actual.inspect ]) unless ok
+      maybe_bad_leaf(value.kind, value.parent.kind, allowed, actual)
     end
 
     def bad_body_kind(prnt, child, i, ok)

app/models/celery_script_settings_bag.rb

@@ -78,8 +78,9 @@ module CeleryScriptSettingsBag
   ONLY_ONE_COORD        = "Move Absolute does not accept a group of locations"\
                           " as input. Please change your selection to a "\
                           "single location."
-  ALLOWED_EVERY_POINT_TYPE    = %w(Tool GenericPointer Plant ToolSlot)
-  BAD_EVERY_POINT_TYPE        = '"%s" is not a type of group. Allowed values: %s'
+  SCOPE_DECLARATIONS    = [:variable_declaration, :parameter_declaration]
+  ALLOWED_EVERY_POINT_TYPE = %w(Tool GenericPointer Plant ToolSlot)
+  BAD_EVERY_POINT_TYPE     = '"%s" is not a type of group. Allowed values: %s'
 
   Corpus = CeleryScript::Corpus.new
       .arg(:_else,        [:execute, :nothing])
@@ -249,7 +250,7 @@ module CeleryScriptSettingsBag
       .node(:install_farmware,      [:url])
       .node(:update_farmware,       [:package])
       .node(:remove_farmware,       [:package])
-      .node(:scope_declaration,     [], [:variable_declaration, :parameter_declaration])
+      .node(:scope_declaration,     [], SCOPE_DECLARATIONS)
       .node(:identifier,            [:label])
       .node(:variable_declaration,  [:label, :data_value], [])
       .node(:parameter_declaration, [:label, :data_type], [])
@@ -258,7 +259,7 @@ module CeleryScriptSettingsBag
       .node(:dump_info,             [], [])
       .node(:install_first_party_farmware, [])
       .node(:internal_farm_event,   [], [:variable_declaration])
-      .node(:internal_regimen,      [], [:variable_declaration, :parameter_declaration])
+      .node(:internal_regimen,      [], SCOPE_DECLARATIONS)
       .node(:internal_entry_point,  [], [])
       .node(:every_point,           [:every_point_type], [])
       .node(:resource_update,       RESOURCE_UPDATE_ARGS) do |x|

app/models/farmware_installation.rb

@@ -17,6 +17,8 @@ class FarmwareInstallation < ApplicationRecord
       "The server is online, but the URL could not be opened.",
     SocketError        =>
       "The server at the provided appears to be offline.",
+    Net::OpenTimeout   =>
+      "A timeout error occured.",
     JSON::ParserError  =>
       "Expected Farmware manifest to be valid JSON, "\
       "but it is not. Consider using a JSON validator.",

spec/controllers/api/regimens/regimens_create_spec.rb

@@ -59,5 +59,29 @@ describe Api::RegimensController do
       expect(json[:name]).to eq(name)
       expect(json[:color]).to eq(color)
     end
+
+    it "handles CeleryScript::TypeCheckError" do
+      sign_in user
+      s       = FakeSequence.with_parameters
+      payload = { device: s.device,
+                  name:   "specs",
+                  color:  "red",
+                  body: [
+                    {
+                      kind: "variable_declaration",
+                      args: {
+                        label: "parent",
+                        data_value: { kind: "nothing", args: { } }
+                      }
+                    }
+                  ],
+                  regimen_items: [
+                    { time_offset: 100, sequence_id: s.id }
+                  ] }
+      post :create, body: payload.to_json, format: :json
+      expect(response.status).to eq(422)
+      expect(json.fetch(:error))
+        .to include("must provide a value for all parameters")
+    end
   end
 end

spec/controllers/api/sequences/sequences_create_spec.rb

@@ -251,7 +251,6 @@ describe Api::SequencesController do
     end
 
     it 'prevents type errors from bad identifier / binding combos' do
-      $lol = true
       sign_in user
       input = { name: "type mismatch",
                 args: {
@@ -283,5 +282,34 @@ describe Api::SequencesController do
       expect(response.status).to eq(422)
       expect(json[:body]).to include("not a valid data_type")
     end
+
+
+    it 'provides human readable errors for "nothing" mismatches' do
+      sign_in user
+      input = { name: "type mismatch",
+                args: {
+                  locals: {
+                    kind: "scope_declaration",
+                    args: { },
+                    body: [
+                      {
+                        kind: "variable_declaration",
+                        args: {
+                          label: "x",
+                          data_value: {
+                            kind: "nothing",
+                            args: {}
+                          }
+                        }
+                      }
+                    ]
+                  }
+                },
+                body: [ ]
+              }
+      post :create, body: input.to_json, params: {format: :json}
+      expect(response.status).to eq(422)
+      expect(json[:body]).to include("must provide a value for all parameters")
+    end
   end
 end

spec/lib/celery_script/checker_spec.rb

@@ -264,7 +264,13 @@ describe CeleryScript::Checker do
           body: [
             {
               kind: "variable_declaration",
-              args: { label: "parent", data_value: { kind: "nothing", args: { } } }
+              args: {
+                label: "parent",
+                data_value: {
+                  kind: "nothing",
+                  args: { }
+                }
+              }
             }
           ]
         }
@@ -283,6 +289,7 @@ describe CeleryScript::Checker do
     tree = CeleryScript::AstNode.new(ast)
     chk  = CeleryScript::Checker.new(tree, corpus, device)
     expect(chk.valid?).to be false
-    expect(chk.error.message).to include('but got "nothing"')
+    message = "must provide a value for all parameters"
+    expect(chk.error.message).to include(message)
   end
 end

spec/models/farmware_installation_spec.rb

@@ -63,9 +63,9 @@ describe FarmwareInstallation do
   end
 
   it "handles `package` fetch errors" do
-    malformed_url = "http://#{SecureRandom.base58.downcase}.com"
     fi            = FarmwareInstallation.create!(device: device,
-                                                 url:    malformed_url)
+                                                 url:    "http://lycos.com")
+    expect(fi).to receive(:open).and_raise(SocketError.new("No!"))
     fi.infer_package_name_from_url
     error = FarmwareInstallation::KNOWN_PROBLEMS.fetch(SocketError)
     expect(fi.package_error).to eq(error)