From faaedaa2fb46f3c371d13121ba4bd5244ac69750 Mon Sep 17 00:00:00 2001 From: Saghm Rossi Date: Thu, 15 Nov 2018 19:07:27 -0500 Subject: [PATCH 1/6] RUBY-1587 Properly handle error labels sent by server --- lib/mongo/error/operation_failure.rb | 10 ++++++++++ lib/mongo/operation/get_more/result.rb | 2 +- spec/support/transactions/operation.rb | 4 ++-- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/mongo/error/operation_failure.rb b/lib/mongo/error/operation_failure.rb index 70bab1a3d6..5680b2eb75 100644 --- a/lib/mongo/error/operation_failure.rb +++ b/lib/mongo/error/operation_failure.rb @@ -182,6 +182,16 @@ def initialize(message = nil, result = nil, options = {}) @code = options[:code] @code_name = options[:code_name] super(message) + + if result + err_doc = result.send(:first_document) + + if err_doc && err_doc['errorLabels'] + err_doc['errorLabels'].each do |label| + add_label(label) + end + end + end end end end diff --git a/lib/mongo/operation/get_more/result.rb b/lib/mongo/operation/get_more/result.rb index caeccbd289..b1a5953879 100644 --- a/lib/mongo/operation/get_more/result.rb +++ b/lib/mongo/operation/get_more/result.rb @@ -52,7 +52,7 @@ def cursor_document end def first_document - @first_document ||= reply.documents[0] + @first_document ||= reply && reply.documents[0] end end end diff --git a/spec/support/transactions/operation.rb b/spec/support/transactions/operation.rb index 2f82b75903..68178e8422 100644 --- a/spec/support/transactions/operation.rb +++ b/spec/support/transactions/operation.rb @@ -114,12 +114,12 @@ def execute(collection, session0, session1) { 'errorCodeName' => err_doc['codeName'] || err_doc['writeConcernError']['codeName'], 'errorContains' => e.message, - 'errorLabels' => (e.instance_variable_get(:@labels) || []) + (err_doc['errorLabels'] || []) + 'errorLabels' => e.instance_variable_get(:@labels) } rescue Mongo::Error => e { 'errorContains' => e.message, - 'errorLabels' => e.instance_variable_get(:@labels) || [] + 'errorLabels' => e.instance_variable_get(:@labels) } end From ad0e112610d96c4c5d1cd6c08e88a272922670f0 Mon Sep 17 00:00:00 2001 From: Saghm Rossi Date: Fri, 16 Nov 2018 16:57:06 -0500 Subject: [PATCH 2/6] initial code review feedback --- lib/mongo/error.rb | 12 ++++++++++++ lib/mongo/error/operation_failure.rb | 10 +--------- lib/mongo/error/parser.rb | 9 +++++++++ lib/mongo/operation/result.rb | 12 ++++++++++++ spec/support/transactions/operation.rb | 4 ++-- 5 files changed, 36 insertions(+), 11 deletions(-) diff --git a/lib/mongo/error.rb b/lib/mongo/error.rb index baa07e2334..74bf4612e5 100644 --- a/lib/mongo/error.rb +++ b/lib/mongo/error.rb @@ -114,6 +114,18 @@ def label?(label) @labels.include?(label) end + # Gets the set of labels associated with the error. + # + # @example + # error.labels + # + # @return [ Array ] The set of labels. + # + # @since 2.7.0 + def labels + @labels.dup + end + private def add_label(label) diff --git a/lib/mongo/error/operation_failure.rb b/lib/mongo/error/operation_failure.rb index 5680b2eb75..f50fa7579b 100644 --- a/lib/mongo/error/operation_failure.rb +++ b/lib/mongo/error/operation_failure.rb @@ -183,15 +183,7 @@ def initialize(message = nil, result = nil, options = {}) @code_name = options[:code_name] super(message) - if result - err_doc = result.send(:first_document) - - if err_doc && err_doc['errorLabels'] - err_doc['errorLabels'].each do |label| - add_label(label) - end - end - end + @labels = (result && result.labels) || [] end end end diff --git a/lib/mongo/error/parser.rb b/lib/mongo/error/parser.rb index 90095158c1..f4fe305465 100644 --- a/lib/mongo/error/parser.rb +++ b/lib/mongo/error/parser.rb @@ -62,6 +62,10 @@ class Parser # @since 2.6.0 attr_reader :code_name + # @return [ Array ] labels The set of labels associated with the error. + # @since 2.7.0 + attr_reader :labels + # Create the new parser with the returned document. # # @example Create the new parser. @@ -88,6 +92,7 @@ def parse! document[WRITE_CONCERN_ERROR]) if document[WRITE_CONCERN_ERROR] parse_flag(@message) parse_code + parse_labels end def parse_single(message, key, doc = document) @@ -147,6 +152,10 @@ def parse_code end end end + + def parse_labels + @labels = document['errorLabels'] || [] + end end end end diff --git a/lib/mongo/operation/result.rb b/lib/mongo/operation/result.rb index 1be4058d58..0e5328ea81 100644 --- a/lib/mongo/operation/result.rb +++ b/lib/mongo/operation/result.rb @@ -312,6 +312,18 @@ def cluster_time first_document && first_document[CLUSTER_TIME] end + # Gets the set of error labels associated with the result. + # + # @example Get the labels. + # result.labels + # + # @return [ Array ] labels The set of labels. + # + # @since 2.7.0 + def labels + @labels ||= parser.labels + end + private def aggregate_returned_count diff --git a/spec/support/transactions/operation.rb b/spec/support/transactions/operation.rb index 68178e8422..9101fe452a 100644 --- a/spec/support/transactions/operation.rb +++ b/spec/support/transactions/operation.rb @@ -114,12 +114,12 @@ def execute(collection, session0, session1) { 'errorCodeName' => err_doc['codeName'] || err_doc['writeConcernError']['codeName'], 'errorContains' => e.message, - 'errorLabels' => e.instance_variable_get(:@labels) + 'errorLabels' => e.labels } rescue Mongo::Error => e { 'errorContains' => e.message, - 'errorLabels' => e.instance_variable_get(:@labels) + 'errorLabels' => e.labels } end From 1aec4520cc1b81c685adf423d199eb41d549e97b Mon Sep 17 00:00:00 2001 From: Saghm Rossi Date: Fri, 16 Nov 2018 17:35:02 -0500 Subject: [PATCH 3/6] add unit testing for label parsing --- spec/mongo/error/operation_failure_spec.rb | 67 ++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/spec/mongo/error/operation_failure_spec.rb b/spec/mongo/error/operation_failure_spec.rb index 58bbb4dfa2..fa3ea3e5e2 100644 --- a/spec/mongo/error/operation_failure_spec.rb +++ b/spec/mongo/error/operation_failure_spec.rb @@ -149,4 +149,71 @@ end end end + + describe '#labels' do + + context 'when the result is nil' do + + subject do + described_class.new('not master (10107)', nil, + :code => 10107, :code_name => 'NotMaster') + end + + it 'has no labels' do + expect(subject.labels).to eq([]) + end + end + + context 'when the result is not nil' do + + let(:reply_document) do + { + 'code' => 251, + 'codeName' => 'NoSuchTransaction', + 'errorLabels' => labels, + } + end + + let(:reply) do + Mongo::Protocol::Reply.new.tap do |r| + # Because this was not created by Mongo::Protocol::Reply::deserialize, we need to manually + # initialize the fields. + r.instance_variable_set(:@documents, [reply_document]) + r.instance_variable_set(:@flags, []) + end + end + + let(:result) do + Mongo::Operation::Result.new(reply) + end + + subject do + described_class.new('Transaction has been aborted', result, + :code => 251, :code_name => 'NoSuchTransaction') + end + + context 'when the error has no labels' do + + let(:labels) do + [] + end + + it 'has the correct labels' do + expect(subject.labels).to eq(labels) + end + end + + + context 'when the error has labels' do + + let(:labels) do + [ Mongo::Error::TRANSIENT_TRANSACTION_ERROR_LABEL ] + end + + it 'has the correct labels' do + expect(subject.labels).to eq(labels) + end + end + end + end end From af3a43f097463925682d1e2242456a29c271cbc4 Mon Sep 17 00:00:00 2001 From: Saghm Rossi Date: Mon, 26 Nov 2018 17:56:29 -0500 Subject: [PATCH 4/6] refactor label initialization --- lib/mongo/error.rb | 2 +- lib/mongo/error/operation_failure.rb | 3 +-- lib/mongo/operation/result.rb | 2 +- spec/mongo/error/operation_failure_spec.rb | 25 ++-------------------- 4 files changed, 5 insertions(+), 27 deletions(-) diff --git a/lib/mongo/error.rb b/lib/mongo/error.rb index 74bf4612e5..89585f5694 100644 --- a/lib/mongo/error.rb +++ b/lib/mongo/error.rb @@ -96,7 +96,7 @@ def change_stream_resumable? TRANSIENT_TRANSACTION_ERROR_LABEL = 'TransientTransactionError'.freeze def initialize(msg = nil) - @labels = [] + @labels ||= [] super(msg) end diff --git a/lib/mongo/error/operation_failure.rb b/lib/mongo/error/operation_failure.rb index f50fa7579b..589dcc51f9 100644 --- a/lib/mongo/error/operation_failure.rb +++ b/lib/mongo/error/operation_failure.rb @@ -181,9 +181,8 @@ def initialize(message = nil, result = nil, options = {}) @result = result @code = options[:code] @code_name = options[:code_name] + @labels = options[:labels] super(message) - - @labels = (result && result.labels) || [] end end end diff --git a/lib/mongo/operation/result.rb b/lib/mongo/operation/result.rb index 0e5328ea81..22f2085050 100644 --- a/lib/mongo/operation/result.rb +++ b/lib/mongo/operation/result.rb @@ -267,7 +267,7 @@ def raise_operation_failure raise Error::OperationFailure.new( parser.message, self, - :code => parser.code, :code_name => parser.code_name) + :code => parser.code, :code_name => parser.code_name, :labels => parser.labels) end private :raise_operation_failure diff --git a/spec/mongo/error/operation_failure_spec.rb b/spec/mongo/error/operation_failure_spec.rb index fa3ea3e5e2..0824abe518 100644 --- a/spec/mongo/error/operation_failure_spec.rb +++ b/spec/mongo/error/operation_failure_spec.rb @@ -166,30 +166,9 @@ context 'when the result is not nil' do - let(:reply_document) do - { - 'code' => 251, - 'codeName' => 'NoSuchTransaction', - 'errorLabels' => labels, - } - end - - let(:reply) do - Mongo::Protocol::Reply.new.tap do |r| - # Because this was not created by Mongo::Protocol::Reply::deserialize, we need to manually - # initialize the fields. - r.instance_variable_set(:@documents, [reply_document]) - r.instance_variable_set(:@flags, []) - end - end - - let(:result) do - Mongo::Operation::Result.new(reply) - end - subject do - described_class.new('Transaction has been aborted', result, - :code => 251, :code_name => 'NoSuchTransaction') + described_class.new('Transaction has been aborted', nil, + :code => 251, :code_name => 'NoSuchTransaction', :labels => labels) end context 'when the error has no labels' do From d58d9029d831cc5bdd7bc74facee187b53e01f79 Mon Sep 17 00:00:00 2001 From: Saghm Rossi Date: Tue, 27 Nov 2018 13:11:49 -0500 Subject: [PATCH 5/6] use Result#raise_operation_failure for testing --- lib/mongo/operation/get_more/result.rb | 2 +- spec/mongo/error/operation_failure_spec.rb | 28 ++++++++++++++++++++-- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/lib/mongo/operation/get_more/result.rb b/lib/mongo/operation/get_more/result.rb index b1a5953879..caeccbd289 100644 --- a/lib/mongo/operation/get_more/result.rb +++ b/lib/mongo/operation/get_more/result.rb @@ -52,7 +52,7 @@ def cursor_document end def first_document - @first_document ||= reply && reply.documents[0] + @first_document ||= reply.documents[0] end end end diff --git a/spec/mongo/error/operation_failure_spec.rb b/spec/mongo/error/operation_failure_spec.rb index 0824abe518..9e2cb870cb 100644 --- a/spec/mongo/error/operation_failure_spec.rb +++ b/spec/mongo/error/operation_failure_spec.rb @@ -166,9 +166,33 @@ context 'when the result is not nil' do + let(:reply_document) do + { + 'code' => 251, + 'codeName' => 'NoSuchTransaction', + 'errorLabels' => labels, + } + end + + let(:reply) do + Mongo::Protocol::Reply.new.tap do |r| + # Because this was not created by Mongo::Protocol::Reply::deserialize, we need to manually + # initialize the fields. + r.instance_variable_set(:@documents, [reply_document]) + r.instance_variable_set(:@flags, []) + end + end + + let(:result) do + Mongo::Operation::Result.new(reply) + end + subject do - described_class.new('Transaction has been aborted', nil, - :code => 251, :code_name => 'NoSuchTransaction', :labels => labels) + begin + result.send(:raise_operation_failure) + rescue => e + e + end end context 'when the error has no labels' do From 48f22dc5c5616c2796cdbf8f7c90242611316f5e Mon Sep 17 00:00:00 2001 From: Saghm Rossi Date: Tue, 27 Nov 2018 14:27:22 -0500 Subject: [PATCH 6/6] add result parser unit test --- spec/mongo/error/parser_spec.rb | 54 +++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/spec/mongo/error/parser_spec.rb b/spec/mongo/error/parser_spec.rb index 4c49e5e93f..136e1eff96 100644 --- a/spec/mongo/error/parser_spec.rb +++ b/spec/mongo/error/parser_spec.rb @@ -95,7 +95,7 @@ end end end - + describe '#code' do let(:parser) do described_class.new(document) @@ -105,7 +105,7 @@ let(:document) do { 'ok' => 0, 'errmsg' => 'not master', 'code' => 10107, 'codeName' => 'NotMaster' } end - + it 'returns the code' do expect(parser.code).to eq(10107) end @@ -115,7 +115,7 @@ let(:document) do { 'ok' => 0, 'errmsg' => 'not master' } end - + it 'returns nil' do expect(parser.code).to eq(nil) end @@ -143,7 +143,7 @@ end end end - + describe '#code_name' do let(:parser) do described_class.new(document) @@ -153,7 +153,7 @@ let(:document) do { 'ok' => 0, 'errmsg' => 'not master', 'code' => 10107, 'codeName' => 'NotMaster' } end - + it 'returns the code name' do expect(parser.code_name).to eq('NotMaster') end @@ -163,7 +163,7 @@ let(:document) do { 'ok' => 0, 'errmsg' => 'not master' } end - + it 'returns nil' do expect(parser.code_name).to eq(nil) end @@ -192,7 +192,7 @@ end end end - + describe '#document' do let(:parser) do described_class.new(document) @@ -201,12 +201,12 @@ let(:document) do { 'ok' => 0, 'errmsg' => 'not master', 'code' => 10107, 'codeName' => 'NotMaster' } end - + it 'returns the document' do expect(parser.document).to eq(document) end end - + describe '#replies' do let(:parser) do described_class.new(document) @@ -216,10 +216,44 @@ let(:document) do { 'ok' => 0, 'errmsg' => 'not master', 'code' => 10107, 'codeName' => 'NotMaster' } end - + it 'returns nil' do expect(parser.replies).to eq(nil) end end end + + describe '#labels' do + let(:parser) do + described_class.new(document) + end + + let(:document) do + { + 'code' => 251, + 'codeName' => 'NoSuchTransaction', + 'errorLabels' => labels, + } + end + + context 'when there are no labels' do + let(:labels) do + [] + end + + it 'has the correct labels' do + expect(parser.labels).to eq(labels) + end + end + + context 'when there are labels' do + let(:labels) do + [ Mongo::Error::TRANSIENT_TRANSACTION_ERROR_LABEL ] + end + + it 'has the correct labels' do + expect(parser.labels).to eq(labels) + end + end + end end