From 13ab8a72fe31b9b619f0e4829bb2fd863a0c76ee Mon Sep 17 00:00:00 2001 From: Martijn Versluis Date: Thu, 12 Sep 2024 13:57:14 +0200 Subject: [PATCH 1/5] Fix endless reply loop Resolves https://github.com/stekker/alliander-poc/issues/26 Co-authored-by: Bob --- lib/s2/connection.rb | 11 +++-------- spec/lib/s2/connection_spec.rb | 19 ++++++++----------- 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/lib/s2/connection.rb b/lib/s2/connection.rb index 4634d3b..e776c26 100644 --- a/lib/s2/connection.rb +++ b/lib/s2/connection.rb @@ -4,19 +4,19 @@ class Connection on S2::Messages::ReceptionStatus do |message| unless message_sent?(message.subject_message_id) - # next reply message, status: S2::Messages::ReceptionStatusValues::InvalidContent @logger.error("Received ReceptionStatus for unknown message ID #{message.subject_message_id}") end case message.status when S2::Messages::ReceptionStatusValues::Ok - # no-op else @logger.error( "Received ReceptionStatus with status #{message.status} " \ "for message ID #{message.subject_message_id}", ) end + + delete_sent_message(message) end def initialize(ws, logger: Rails.logger) @@ -31,7 +31,6 @@ def receive_message(message_json) message = deserialize_message(message_json) handle_message(message) - delete_sent_message(message) if message.is_a?(S2::Messages::ReceptionStatus) rescue JSON::ParserError @logger.error("Received invalid JSON: #{message_json}") rescue KeyError => e @@ -61,11 +60,7 @@ def close(message: nil) def reply(message, status:) if message.is_a?(S2::Messages::ReceptionStatus) - send_message( - S2::Messages::ReceptionStatus, - status:, - subject_message_id: message.subject_message_id, - ) + @logger.error("Cannot reply to ReceptionStatus message") else send_message( S2::Messages::ReceptionStatus, diff --git a/spec/lib/s2/connection_spec.rb b/spec/lib/s2/connection_spec.rb index 51074f7..77a8038 100644 --- a/spec/lib/s2/connection_spec.rb +++ b/spec/lib/s2/connection_spec.rb @@ -47,20 +47,17 @@ .with(/Received ReceptionStatus with status INVALID_CONTENT for message ID 123/) end - it "sends an error response when the original message is not known", - skip: "sending this error causes an endless loop" do - error_response = build(:s2_reception_status, :invalid_content, subject_message_id: "123") - - logger = Logger.new(nil) - connection = described_class.new(ws, logger:) + it "logs an error when the original message is not known" do + logger = Logger.new(nil) + connection = described_class.new(ws, logger:) - allow(logger).to receive(:error) + allow(logger).to receive(:error) - reception_status = build(:s2_reception_status, :invalid_content, subject_message_id: "123") - connection.receive_message(reception_status.to_json) + reception_status = build(:s2_reception_status, :invalid_content, subject_message_id: "123") + connection.receive_message(reception_status.to_json) - expect(ws).to have_sent_message(error_response.to_json) - end + expect(logger).to have_received(:error).with("Received ReceptionStatus for unknown message ID 123") + end end end From a20bb5860aa45048b9fd259471e0c61fee871b89 Mon Sep 17 00:00:00 2001 From: Bob Forma <1178544+bforma@users.noreply.github.com> Date: Thu, 12 Sep 2024 14:07:19 +0200 Subject: [PATCH 2/5] Exclude file naming convention for lib/s2-ruby.rb Since we can't change the file entry point for a gem. --- .rubocop.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.rubocop.yml b/.rubocop.yml index 8fdf6b9..147fc59 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -216,6 +216,10 @@ Naming/AccessorMethodName: Exclude: - app/models/**/*.rb +Naming/FileName: + Exclude: + - lib/s2-ruby.rb + Naming/MethodParameterName: MinNameLength: 2 From df8e60d8047f4964cd6ca14bd737a8eb6eb153a0 Mon Sep 17 00:00:00 2001 From: Bob Forma <1178544+bforma@users.noreply.github.com> Date: Thu, 12 Sep 2024 14:08:40 +0200 Subject: [PATCH 3/5] Add comment to empty when-block --- lib/s2/connection.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/s2/connection.rb b/lib/s2/connection.rb index e776c26..b4c1f23 100644 --- a/lib/s2/connection.rb +++ b/lib/s2/connection.rb @@ -9,6 +9,7 @@ class Connection case message.status when S2::Messages::ReceptionStatusValues::Ok + # no-op else @logger.error( "Received ReceptionStatus with status #{message.status} " \ From a6f7469e8c0d43d4795a949cc8ea994861036ece Mon Sep 17 00:00:00 2001 From: Bob Forma <1178544+bforma@users.noreply.github.com> Date: Thu, 12 Sep 2024 14:16:47 +0200 Subject: [PATCH 4/5] Add spec for removing sent messages --- lib/s2/connection.rb | 28 +++++++++++++++------------- spec/lib/s2/connection_spec.rb | 31 +++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/lib/s2/connection.rb b/lib/s2/connection.rb index b4c1f23..0501c3a 100644 --- a/lib/s2/connection.rb +++ b/lib/s2/connection.rb @@ -2,22 +2,24 @@ module S2 class Connection include S2::MessageHandler - on S2::Messages::ReceptionStatus do |message| - unless message_sent?(message.subject_message_id) - @logger.error("Received ReceptionStatus for unknown message ID #{message.subject_message_id}") - end + attr_reader :sent_messages - case message.status - when S2::Messages::ReceptionStatusValues::Ok - # no-op + on S2::Messages::ReceptionStatus do |message| + if message_sent?(message.subject_message_id) + case message.status + when S2::Messages::ReceptionStatusValues::Ok + # no-op + else + @logger.error( + "Received ReceptionStatus with status #{message.status} for " \ + "unknown message ID #{message.subject_message_id}", + ) + end + + delete_sent_message(message) else - @logger.error( - "Received ReceptionStatus with status #{message.status} " \ - "for message ID #{message.subject_message_id}", - ) + @logger.error("Received ReceptionStatus for unknown message ID #{message.subject_message_id}") end - - delete_sent_message(message) end def initialize(ws, logger: Rails.logger) diff --git a/spec/lib/s2/connection_spec.rb b/spec/lib/s2/connection_spec.rb index 77a8038..dde3b9e 100644 --- a/spec/lib/s2/connection_spec.rb +++ b/spec/lib/s2/connection_spec.rb @@ -44,10 +44,10 @@ connection.receive_message(reception_status.to_json) expect(logger).to have_received(:error) - .with(/Received ReceptionStatus with status INVALID_CONTENT for message ID 123/) + .with(/Received ReceptionStatus with status INVALID_CONTENT for unknown message ID 123/) end - it "logs an error when the original message is not known" do + it "logs an error when the original message has not been sent" do logger = Logger.new(nil) connection = described_class.new(ws, logger:) @@ -58,6 +58,33 @@ expect(logger).to have_received(:error).with("Received ReceptionStatus for unknown message ID 123") end + + it "does not remove the original message from the sent messages when it was not sent" do + allow(SecureRandom).to receive(:uuid).and_return("123") + + logger = Logger.new(nil) + connection = described_class.new(ws, logger:) + + reception_status = build(:s2_reception_status, :invalid_content, subject_message_id: "123") + + expect do + connection.receive_message(reception_status.to_json) + end.not_to change { connection.sent_messages.size } + end + + it "removes the original message from the sent messages" do + allow(SecureRandom).to receive(:uuid).and_return("123") + + logger = Logger.new(nil) + connection = described_class.new(ws, logger:) + + connection.send_message(S2::Messages::Handshake, { role: S2::Messages::EnergyManagementRole::Cem }) + reception_status = build(:s2_reception_status, :invalid_content, subject_message_id: "123") + + expect do + connection.receive_message(reception_status.to_json) + end.to change { connection.sent_messages.size }.from(1).to(0) + end end end From 2c9fd14d4dc7205348fd83dda36bd1bbe32fbdf7 Mon Sep 17 00:00:00 2001 From: Bob Forma <1178544+bforma@users.noreply.github.com> Date: Thu, 12 Sep 2024 14:26:32 +0200 Subject: [PATCH 5/5] Close connection when receiving permanent error --- lib/s2/connection.rb | 13 +------------ spec/lib/s2/connection_spec.rb | 31 +++++++++++++++---------------- 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/lib/s2/connection.rb b/lib/s2/connection.rb index 0501c3a..78bc3ab 100644 --- a/lib/s2/connection.rb +++ b/lib/s2/connection.rb @@ -6,17 +6,8 @@ class Connection on S2::Messages::ReceptionStatus do |message| if message_sent?(message.subject_message_id) - case message.status - when S2::Messages::ReceptionStatusValues::Ok - # no-op - else - @logger.error( - "Received ReceptionStatus with status #{message.status} for " \ - "unknown message ID #{message.subject_message_id}", - ) - end - delete_sent_message(message) + close if message.status == S2::Messages::ReceptionStatusValues::PermanentError else @logger.error("Received ReceptionStatus for unknown message ID #{message.subject_message_id}") end @@ -71,8 +62,6 @@ def reply(message, status:) subject_message_id: message.message_id, ) end - - @ws.close if status == S2::Messages::ReceptionStatusValues::PermanentError end def message_sent?(message_id) diff --git a/spec/lib/s2/connection_spec.rb b/spec/lib/s2/connection_spec.rb index dde3b9e..2ff0196 100644 --- a/spec/lib/s2/connection_spec.rb +++ b/spec/lib/s2/connection_spec.rb @@ -31,22 +31,6 @@ end context "when the message is a ReceptionStatus" do - it "logs an error if the status is not OK" do - allow(SecureRandom).to receive(:uuid).and_return("123") - - logger = Logger.new(nil) - connection = described_class.new(ws, logger:) - - allow(logger).to receive(:error) - - connection.send_message(S2::Messages::Handshake, { role: S2::Messages::EnergyManagementRole::Cem }) - reception_status = build(:s2_reception_status, :invalid_content, subject_message_id: "123") - connection.receive_message(reception_status.to_json) - - expect(logger).to have_received(:error) - .with(/Received ReceptionStatus with status INVALID_CONTENT for unknown message ID 123/) - end - it "logs an error when the original message has not been sent" do logger = Logger.new(nil) connection = described_class.new(ws, logger:) @@ -85,6 +69,21 @@ connection.receive_message(reception_status.to_json) end.to change { connection.sent_messages.size }.from(1).to(0) end + + it "closes the connection when the status is PermanentError" do + allow(SecureRandom).to receive(:uuid).and_return("123") + + logger = Logger.new(nil) + connection = described_class.new(ws, logger:) + allow(connection).to receive(:close) + + connection.send_message(S2::Messages::Handshake, { role: S2::Messages::EnergyManagementRole::Cem }) + reception_status = build(:s2_reception_status, :permanent_error, subject_message_id: "123") + + connection.receive_message(reception_status.to_json) + + expect(connection).to have_received(:close) + end end end