diff --git a/openc3/lib/openc3/config/config_parser.rb b/openc3/lib/openc3/config/config_parser.rb index cc4c711fd1..3c96357b9a 100644 --- a/openc3/lib/openc3/config/config_parser.rb +++ b/openc3/lib/openc3/config/config_parser.rb @@ -319,8 +319,10 @@ def self.handle_nil(value) # primitive. All other values are simply returned. # # @param value [Object] + # @param default [Object] Returned if value is nil # @return [true|false|Object] - def self.handle_true_false(value) + def self.handle_true_false(value, default: nil) + return default if value.nil? if String === value case value.upcase when 'TRUE' diff --git a/openc3/lib/openc3/microservices/interface_microservice.rb b/openc3/lib/openc3/microservices/interface_microservice.rb index 3964b2d064..570daf80ec 100644 --- a/openc3/lib/openc3/microservices/interface_microservice.rb +++ b/openc3/lib/openc3/microservices/interface_microservice.rb @@ -156,8 +156,8 @@ def run next 'SUCCESS' end if msg_hash.key?('interface_cmd') - params = JSON.parse(msg_hash['interface_cmd'], allow_nan: true, create_additions: true) begin + params = JSON.parse(msg_hash['interface_cmd'], allow_nan: true, create_additions: true) @logger.info "#{@interface.name}: interface_cmd: #{params['cmd_name']} #{params['cmd_params'].join(' ')}" @interface.interface_cmd(params['cmd_name'], *params['cmd_params']) InterfaceStatusModel.set(@interface.as_json(), queued: true, scope: @scope) @@ -168,8 +168,8 @@ def run next 'SUCCESS' end if msg_hash.key?('protocol_cmd') - params = JSON.parse(msg_hash['protocol_cmd'], allow_nan: true, create_additions: true) begin + params = JSON.parse(msg_hash['protocol_cmd'], allow_nan: true, create_additions: true) @logger.info "#{@interface.name}: protocol_cmd: #{params['cmd_name']} #{params['cmd_params'].join(' ')} read_write: #{params['read_write']} index: #{params['index']}" @interface.protocol_cmd(params['cmd_name'], *params['cmd_params'], read_write: params['read_write'], index: params['index']) InterfaceStatusModel.set(@interface.as_json(), queued: true, scope: @scope) @@ -232,21 +232,28 @@ def run target_name = msg_hash['target_name'] if target_name and not @interface.cmd_target_enabled[target_name] + # A released critical command was already acked as pending so it must get a result + next "Target #{target_name} is disabled" if release_critical next nil # Return and don't ack given target_name if disabled end cmd_name = msg_hash['cmd_name'] - manual = ConfigParser.handle_true_false(msg_hash['manual']) + manual = ConfigParser.handle_true_false(msg_hash['manual'], default: false) cmd_params = nil range_check = true raw = false cmd_buffer = nil hazardous_check = nil if msg_hash['cmd_params'] - cmd_params = JSON.parse(msg_hash['cmd_params'], allow_nan: true, create_additions: true) - range_check = ConfigParser.handle_true_false(msg_hash['range_check']) - raw = ConfigParser.handle_true_false(msg_hash['raw']) - hazardous_check = ConfigParser.handle_true_false(msg_hash['hazardous_check']) + begin + cmd_params = JSON.parse(msg_hash['cmd_params'], allow_nan: true, create_additions: true) + rescue => e + @logger.error "#{@interface.name}: Invalid cmd_params: #{e.message}" + next "Invalid cmd_params: #{e.message}" + end + range_check = ConfigParser.handle_true_false(msg_hash['range_check'], default: true) + raw = ConfigParser.handle_true_false(msg_hash['raw'], default: false) + hazardous_check = ConfigParser.handle_true_false(msg_hash['hazardous_check'], default: true) elsif msg_hash['cmd_buffer'] cmd_buffer = msg_hash['cmd_buffer'] end @@ -278,6 +285,7 @@ def run command.received_count = orig_command.received_count command.received_time = Time.now else + next "Target #{command.target_name} is disabled" if release_critical next nil # Don't ack disabled targets end rescue RangeError => e @@ -325,7 +333,7 @@ def run end end - validate = ConfigParser.handle_true_false(msg_hash['validate']) + validate = ConfigParser.handle_true_false(msg_hash['validate'], default: true) begin if @interface.connected? result = true @@ -347,7 +355,7 @@ def run @count += 1 @metric.set(name: 'interface_cmd_total', value: @count, type: 'counter') if @metric - log_message = ConfigParser.handle_true_false(msg_hash['log_message']) + log_message = ConfigParser.handle_true_false(msg_hash['log_message'], default: true) if log_message @logger.info(msg_hash['cmd_string'], user: msg_hash['username'], scope: @scope) end diff --git a/openc3/python/openc3/microservices/interface_microservice.py b/openc3/python/openc3/microservices/interface_microservice.py index c77160b0fc..87f0876599 100644 --- a/openc3/python/openc3/microservices/interface_microservice.py +++ b/openc3/python/openc3/microservices/interface_microservice.py @@ -201,8 +201,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): self.interface.stop_raw_logging() return "SUCCESS" if msg_hash.get(b"interface_cmd"): - params = json.loads(msg_hash[b"interface_cmd"]) try: + params = json.loads(msg_hash[b"interface_cmd"]) str_params = " ".join([str(i) for i in params["cmd_params"]]) self.logger.info(f"{self.interface.name}: interface_cmd: {params['cmd_name']} {str_params}") self.interface.interface_cmd(params["cmd_name"], *params["cmd_params"]) @@ -213,8 +213,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): return str(e) return "SUCCESS" if msg_hash.get(b"protocol_cmd"): - params = json.loads(msg_hash[b"protocol_cmd"]) try: + params = json.loads(msg_hash[b"protocol_cmd"]) str_params = " ".join([str(i) for i in params["cmd_params"]]) self.logger.info( f"{self.interface.name}: protocol_cmd: {params['cmd_name']} {str_params} read_write: {params['read_write']} index: {params['index']}" @@ -286,10 +286,15 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): ) return str(e) - target_name = msg_hash[b"target_name"].decode() + target_name = msg_hash.get(b"target_name") + target_name = target_name.decode() if target_name is not None else None if target_name and not self.interface.cmd_target_enabled.get(target_name, False): + # A released critical command was already acked as pending so it must get a result + if release_critical: + return f"Target {target_name} is disabled" return None # Return and don't ack given target_name if disabled - cmd_name = msg_hash[b"cmd_name"].decode() + cmd_name = msg_hash.get(b"cmd_name") + cmd_name = cmd_name.decode() if cmd_name is not None else None manual = ConfigParser.handle_true_false(msg_hash.get(b"manual", b"FALSE").decode()) cmd_params = None range_check = True @@ -297,7 +302,11 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): cmd_buffer = None hazardous_check = None if msg_hash.get(b"cmd_params") is not None: - cmd_params = json.loads(msg_hash.get(b"cmd_params"), cls=JsonDecoder) + try: + cmd_params = json.loads(msg_hash.get(b"cmd_params"), cls=JsonDecoder) + except ValueError as e: + self.logger.error(f"{self.interface.name}: Invalid cmd_params: {e}") + return f"Invalid cmd_params: {e}" range_check = ConfigParser.handle_true_false(msg_hash.get(b"range_check", b"TRUE").decode()) raw = ConfigParser.handle_true_false(msg_hash.get(b"raw", b"FALSE").decode()) hazardous_check = ConfigParser.handle_true_false(msg_hash.get(b"hazardous_check", b"TRUE").decode()) @@ -324,6 +333,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): if enabled_target_name == "UNKNOWN" and target_name: enabled_target_name = target_name if not self.interface.cmd_target_enabled.get(enabled_target_name, False): + if release_critical: + return f"Target {command.target_name} is disabled" return None # Don't ack disabled targets orig_command = System.commands.packet(command.target_name, command.packet_name) @@ -393,10 +404,11 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): if command.validator and validate: try: result, reason = command.validator.pre_check(command) - except Exception: + except Exception as e: result = False - reason = traceback.format_exc() - if not result: + reason = str(e) + # Explicitly check for False to allow None to represent unknown + if result is False: message = f"pre_check returned false for {command.extra['cmd_string']} due to {reason}" raise WriteRejectError(message) @@ -419,9 +431,9 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): if command.validator and validate: try: result, reason = command.validator.post_check(command) - except Exception: + except Exception as e: result = False - reason = traceback.format_exc() + reason = str(e) command.extra["cmd_success"] = result if reason: command.extra["cmd_reason"] = reason @@ -430,7 +442,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): CommandTopic.write_packet(command, scope=self.scope) InterfaceStatusModel.set(self.interface.as_json(), queued=True, scope=self.scope) - if not result: + # Explicitly check for False to allow None to represent unknown + if result is False: message = f"post_check returned false for {command.extra['cmd_string']} due to {reason}" raise WriteRejectError(message) diff --git a/openc3/python/test/microservices/test_interface_microservice.py b/openc3/python/test/microservices/test_interface_microservice.py index d5304ddfa4..d1ce77ca34 100644 --- a/openc3/python/test/microservices/test_interface_microservice.py +++ b/openc3/python/test/microservices/test_interface_microservice.py @@ -777,13 +777,14 @@ def test_process_cmd_with_all_fields_and_missing_optional_fields(self): result = handler.process_cmd(topic, msg_id, minimal_msg_hash, None) self.assertEqual(result, "SUCCESS") - # Missing target_name raises KeyError (required field uses direct [] access) - with self.assertRaises(KeyError): - handler.process_cmd(topic, msg_id, {b"cmd_name": b"ABORT"}, None) - - # Missing cmd_name raises KeyError (required field uses direct [] access) - with self.assertRaises(KeyError): - handler.process_cmd(topic, msg_id, {b"target_name": b"INST"}, None) + # Missing target_name or cmd_name is acked with an error rather than raising + cmd_params = json.dumps({}).encode() + result = handler.process_cmd(topic, msg_id, {b"cmd_name": b"ABORT", b"cmd_params": cmd_params}, None) + self.assertIsInstance(result, str) + self.assertNotEqual(result, "SUCCESS") + result = handler.process_cmd(topic, msg_id, {b"target_name": b"INST", b"cmd_params": cmd_params}, None) + self.assertIsInstance(result, str) + self.assertNotEqual(result, "SUCCESS") # Disabled target returns None (no ack) handler.interface.cmd_target_enabled["INST"] = False @@ -953,6 +954,89 @@ def test_process_cmd_command_error_and_hazardous_branches(self): ) self.assertEqual(result, "Interface not connected: INST_INT") + def test_process_cmd_acks_malformed_json(self): + """Malformed JSON in a directive or cmd_params is acked with an error + instead of killing the command handler thread.""" + im = InterfaceMicroservice("DEFAULT__INTERFACE__INST_INT") + self.addCleanup(im.shutdown) + handler = im.handler_thread + msg_id = f"{int(time.time() * 1000)}-0" + + for directive in [b"interface_cmd", b"protocol_cmd"]: + result = handler.process_cmd("{DEFAULT__CMD}INTERFACE__INST_INT", msg_id, {directive: b"{not json"}, None) + self.assertIsInstance(result, str) + self.assertNotEqual(result, "SUCCESS") + + result = handler.process_cmd( + "{DEFAULT__CMD}TARGET__INST", + msg_id, + {b"target_name": b"INST", b"cmd_name": b"ABORT", b"cmd_params": b"{not json"}, + None, + ) + self.assertIn("Invalid cmd_params", result) + + def test_process_cmd_acks_released_critical_cmd_for_disabled_target(self): + """A released critical command was already acked as pending, so a target + disabled in the meantime must produce an error ack, not silence.""" + im = InterfaceMicroservice("DEFAULT__INTERFACE__INST_INT") + self.addCleanup(im.shutdown) + handler = im.handler_thread + msg_id = f"{int(time.time() * 1000)}-0" + + critical_model = Mock() + critical_model.approver = "approver" + critical_model.cmd_hash = { + b"target_name": b"INST", + b"cmd_name": b"ABORT", + b"cmd_params": json.dumps({}).encode(), + b"cmd_string": b"cmd('INST ABORT')", + } + handler.interface.cmd_target_enabled["INST"] = False + with patch("openc3.microservices.interface_microservice.CriticalCmdModel", create=True) as model_class: + model_class.get_model.return_value = critical_model + result = handler.process_cmd( + "{DEFAULT__CMD}INTERFACE__INST_INT", msg_id, {b"release_critical": b"abc-123"}, None + ) + self.assertEqual(result, "Target INST is disabled") + + def test_process_cmd_validator_results(self): + """Only an explicit False from pre_check/post_check rejects a command (None + means unknown), and a raising validator reports its message, not a traceback.""" + im = InterfaceMicroservice("DEFAULT__INTERFACE__INST_INT") + thread = threading.Thread(target=im.run) + thread.start() + self.addCleanup(thread.join, 5) + self.addCleanup(im.shutdown) + time.sleep(0.1) + + handler = im.handler_thread + topic = "{DEFAULT__CMD}TARGET__INST" + msg_id = f"{int(time.time() * 1000)}-0" + msg_hash = { + b"target_name": b"INST", + b"cmd_name": b"ABORT", + b"cmd_params": json.dumps({}).encode(), + b"cmd_string": b"cmd('INST ABORT')", + } + + validator = Mock() + packet = System.commands.packet("INST", "ABORT") + packet.validator = validator + self.addCleanup(setattr, packet, "validator", None) + + validator.pre_check.return_value = (None, None) + validator.post_check.return_value = (None, None) + self.assertEqual(handler.process_cmd(topic, msg_id, msg_hash, None), "SUCCESS") + + validator.pre_check.return_value = (False, "not ready") + result = handler.process_cmd(topic, msg_id, msg_hash, None) + self.assertEqual(result, "pre_check returned false for cmd('INST ABORT') due to not ready") + + validator.pre_check.return_value = (True, None) + validator.post_check.side_effect = RuntimeError("post boom") + result = handler.process_cmd(topic, msg_id, msg_hash, None) + self.assertEqual(result, "post_check returned false for cmd('INST ABORT') due to post boom") + def test_process_cmd_decodes_ruby_nan_cmd_params(self): """cmd_params from a Ruby caller encode NaN as a json_class Float object; it must reach build_cmd as a float, not a dict.""" diff --git a/openc3/spec/config/config_parser_spec.rb b/openc3/spec/config/config_parser_spec.rb index 781cab08e9..628f607633 100644 --- a/openc3/spec/config/config_parser_spec.rb +++ b/openc3/spec/config/config_parser_spec.rb @@ -537,6 +537,13 @@ module OpenC3 expect(ConfigParser.handle_true_false("HI")).to eql "HI" expect(ConfigParser.handle_true_false(5.0)).to eql 5.0 end + + it "returns the default for nil" do + expect(ConfigParser.handle_true_false(nil)).to be_nil + expect(ConfigParser.handle_true_false(nil, default: true)).to be true + expect(ConfigParser.handle_true_false(nil, default: false)).to be false + expect(ConfigParser.handle_true_false('FALSE', default: true)).to be false + end end describe "self.handle_true_false_strict" do diff --git a/openc3/spec/microservices/interface_microservice_spec.rb b/openc3/spec/microservices/interface_microservice_spec.rb index 899b704a76..8d0bfa2262 100644 --- a/openc3/spec/microservices/interface_microservice_spec.rb +++ b/openc3/spec/microservices/interface_microservice_spec.rb @@ -271,6 +271,91 @@ def handle_interface_details(interface) end end + describe "command message handling" do + # Drive the command handler block directly with one crafted message and + # return the block's result, which becomes the ack (nil means no ack) + def handle(msg_hash, topic: "{DEFAULT__CMD}TARGET__INST") + handler = InterfaceCmdHandlerThread.new(@interface, double("tlm").as_null_object, scope: "DEFAULT") + result = :not_run + mutex = Mutex.new + allow(InterfaceTopic).to receive(:receive_commands) do |*_args, **_kwargs, &block| + # The microservice's own handler thread may also land here, so only run the message once + mutex.synchronize do + result = block.call(topic, "#{(Time.now.to_f * 1000).to_i}-0", msg_hash, nil) if result == :not_run + end + end + handler.run + result + end + + before(:each) do + @im = InterfaceMicroservice.new("DEFAULT__INTERFACE__INST_INT") + @interface = @im.instance_variable_get(:@interface) + allow(@interface).to receive(:connected?).and_return(true) + allow(@interface).to receive(:write) + end + + after(:each) do + @im.shutdown + sleep 0.1 # Allow threads to exit + end + + it "acks malformed interface_cmd and protocol_cmd JSON with an error" do + %w(interface_cmd protocol_cmd).each do |directive| + result = handle({ directive => '{not json' }, topic: "{DEFAULT__CMD}INTERFACE__INST_INT") + expect(result).to be_a(String) + expect(result).to_not eql 'SUCCESS' + end + end + + it "acks malformed cmd_params JSON with an error" do + result = handle({ 'target_name' => 'INST', 'cmd_name' => 'ABORT', 'cmd_params' => '{not json' }) + expect(result).to match(/Invalid cmd_params/) + end + + it "does not ack a command for a disabled target" do + @interface.cmd_target_enabled['INST'] = false + expect(handle({ 'target_name' => 'INST', 'cmd_name' => 'ABORT', 'cmd_params' => '{}' })).to be_nil + end + + it "acks a released critical command whose target is now disabled" do + critical_model = double("CriticalCmdModel", approver: 'approver', + cmd_hash: { 'target_name' => 'INST', 'cmd_name' => 'ABORT', 'cmd_params' => '{}', 'cmd_string' => 'cmd("INST ABORT")' }) + stub_const("OpenC3::CriticalCmdModel", double(get_model: critical_model)) + @interface.cmd_target_enabled['INST'] = false + result = handle({ 'release_critical' => 'abc-123' }, topic: "{DEFAULT__CMD}INTERFACE__INST_INT") + expect(result).to eql "Target INST is disabled" + end + + it "range checks a command that omits range_check" do + result = handle({ 'target_name' => 'INST', 'cmd_name' => 'COLLECT', 'cmd_params' => JSON.generate({ 'TYPE' => 'NORMAL', 'DURATION' => 100 }) }) + expect(result).to match(/DURATION/) + end + + it "hazardous checks a command that omits hazardous_check" do + result = handle({ 'target_name' => 'INST', 'cmd_name' => 'CLEAR', 'cmd_params' => '{}', 'cmd_string' => 'cmd("INST CLEAR")' }) + expect(result).to start_with("HazardousError") + end + + it "validates and logs a command that omits validate and log_message" do + validator = double("validator") + allow_any_instance_of(Packet).to receive(:validator).and_return(validator) + expect(validator).to receive(:pre_check).and_return([true, nil]) + expect(validator).to receive(:post_check).and_return([true, nil]) + expect(Logger).to receive(:info).with('cmd("INST ABORT")', anything).at_least(:once) + allow(Logger).to receive(:info) + result = handle({ 'target_name' => 'INST', 'cmd_name' => 'ABORT', 'cmd_params' => '{}', 'cmd_string' => 'cmd("INST ABORT")' }) + expect(result).to eql 'SUCCESS' + end + + it "does not log a command with log_message false" do + expect(Logger).to_not receive(:info).with('cmd("INST ABORT")', anything) + allow(Logger).to receive(:info) + result = handle({ 'target_name' => 'INST', 'cmd_name' => 'ABORT', 'cmd_params' => '{}', 'cmd_string' => 'cmd("INST ABORT")', 'log_message' => 'false' }) + expect(result).to eql 'SUCCESS' + end + end + describe "handle_packet" do it "does not write the status model after cancel_thread is set" do # A packet already buffered can be returned from read() after stop()