diff --git a/app/controllers/api/abstract_controller.rb b/app/controllers/api/abstract_controller.rb index 1f48ccd99..b0c102817 100644 --- a/app/controllers/api/abstract_controller.rb +++ b/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 } diff --git a/app/lib/celery_script/checker.rb b/app/lib/celery_script/checker.rb index 12929366f..ea6c75457 100644 --- a/app/lib/celery_script/checker.rb +++ b/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) diff --git a/app/models/celery_script_settings_bag.rb b/app/models/celery_script_settings_bag.rb index 70ffc5a0a..ab4c62491 100644 --- a/app/models/celery_script_settings_bag.rb +++ b/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| diff --git a/app/models/farmware_installation.rb b/app/models/farmware_installation.rb index 235f0c2d2..a3faa7d1d 100644 --- a/app/models/farmware_installation.rb +++ b/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.", diff --git a/spec/controllers/api/regimens/regimens_create_spec.rb b/spec/controllers/api/regimens/regimens_create_spec.rb index 8e3b76b2f..4a8dccc47 100644 --- a/spec/controllers/api/regimens/regimens_create_spec.rb +++ b/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 diff --git a/spec/controllers/api/sequences/sequences_create_spec.rb b/spec/controllers/api/sequences/sequences_create_spec.rb index da90dda31..1bbf96bc1 100644 --- a/spec/controllers/api/sequences/sequences_create_spec.rb +++ b/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 diff --git a/spec/lib/celery_script/checker_spec.rb b/spec/lib/celery_script/checker_spec.rb index 2a5b362aa..b14f82853 100644 --- a/spec/lib/celery_script/checker_spec.rb +++ b/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 diff --git a/spec/models/farmware_installation_spec.rb b/spec/models/farmware_installation_spec.rb index 4249a7950..a4b454127 100644 --- a/spec/models/farmware_installation_spec.rb +++ b/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)