From cde5fb25e932a0b103fa09343b365ebcf2bf2928 Mon Sep 17 00:00:00 2001 From: Dimitris Christodoulou <36637689+DemetrisChr@users.noreply.github.com> Date: Thu, 17 Aug 2023 12:30:35 +0100 Subject: [PATCH] RCBC-451 & RCBC-452: Expose any specific lookup_in spec errors (#118) Motivation ======== Any spec-specific errors were ignored which resulted in any error that happens being exposed PathNotFound (because of the absence of content) Changes ======= Add an error attribute to the SubDocumentField class that stores the Ruby error that is equivalent to the spec-specific error code from the C++ response. This error is raised then content or exists? for that spec are called --------- Co-authored-by: Sergey Avseyev --- ext/couchbase | 2 +- ext/couchbase.cxx | 22 ++++++ lib/couchbase/collection.rb | 2 + lib/couchbase/collection_options.rb | 7 ++ test/subdoc_test.rb | 118 ++++++++++++++++++++++++++++ 5 files changed, 150 insertions(+), 1 deletion(-) diff --git a/ext/couchbase b/ext/couchbase index 77fa45b6..c6be14e8 160000 --- a/ext/couchbase +++ b/ext/couchbase @@ -1 +1 @@ -Subproject commit 77fa45b615170a658440e6ce3828b9b74a1cd050 +Subproject commit c6be14e85eb3d816638708fe0b29681b3dee1bf1 diff --git a/ext/couchbase.cxx b/ext/couchbase.cxx index 5db96f1e..baceab88 100644 --- a/ext/couchbase.cxx +++ b/ext/couchbase.cxx @@ -3501,6 +3501,7 @@ cb_Backend_document_lookup_in(VALUE self, VALUE bucket, VALUE scope, VALUE colle static VALUE exists_property = rb_id2sym(rb_intern("exists")); static VALUE cas_property = rb_id2sym(rb_intern("cas")); static VALUE value_property = rb_id2sym(rb_intern("value")); + static VALUE error_property = rb_id2sym(rb_intern("error")); VALUE res = rb_hash_new(); rb_hash_aset(res, cas_property, cb_cas_to_num(resp.cas)); @@ -3516,6 +3517,12 @@ cb_Backend_document_lookup_in(VALUE self, VALUE bucket, VALUE scope, VALUE colle if (!resp_entry.value.empty()) { rb_hash_aset(entry, value_property, cb_str_new(resp_entry.value)); } + if (resp_entry.ec && resp_entry.ec != couchbase::errc::key_value::path_not_found) { + rb_hash_aset(entry, + error_property, + cb_map_error_code(resp_entry.ec, + fmt::format("error getting result for spec at index {}, path \"{}\"", i, resp_entry.path))); + } rb_ary_store(fields, static_cast(i), entry); } return res; @@ -3602,6 +3609,7 @@ cb_Backend_document_lookup_in_any_replica(VALUE self, VALUE bucket, VALUE scope, static VALUE exists_property = rb_id2sym(rb_intern("exists")); static VALUE cas_property = rb_id2sym(rb_intern("cas")); static VALUE value_property = rb_id2sym(rb_intern("value")); + static VALUE error_property = rb_id2sym(rb_intern("error")); static VALUE is_replica_property = rb_id2sym(rb_intern("is_replica")); VALUE res = rb_hash_new(); @@ -3620,6 +3628,12 @@ cb_Backend_document_lookup_in_any_replica(VALUE self, VALUE bucket, VALUE scope, if (!resp_entry.value.empty()) { rb_hash_aset(entry, value_property, cb_str_new(resp_entry.value)); } + if (resp_entry.ec && resp_entry.ec != couchbase::errc::key_value::path_not_found) { + rb_hash_aset(entry, + error_property, + cb_map_error_code(resp_entry.ec, + fmt::format("error getting result for spec at index {}, path \"{}\"", i, resp_entry.path))); + } rb_ary_store(fields, static_cast(i), entry); } @@ -3707,6 +3721,7 @@ cb_Backend_document_lookup_in_all_replicas(VALUE self, VALUE bucket, VALUE scope static VALUE exists_property = rb_id2sym(rb_intern("exists")); static VALUE cas_property = rb_id2sym(rb_intern("cas")); static VALUE value_property = rb_id2sym(rb_intern("value")); + static VALUE error_property = rb_id2sym(rb_intern("error")); static VALUE is_replica_property = rb_id2sym(rb_intern("is_replica")); auto lookup_in_entries_size = resp.entries.size(); @@ -3729,6 +3744,13 @@ cb_Backend_document_lookup_in_all_replicas(VALUE self, VALUE bucket, VALUE scope if (!field_entry.value.empty()) { rb_hash_aset(entry, value_property, cb_str_new(field_entry.value)); } + if (field_entry.ec && field_entry.ec != couchbase::errc::key_value::path_not_found) { + rb_hash_aset( + entry, + error_property, + cb_map_error_code(field_entry.ec, + fmt::format("error getting result for spec at index {}, path \"{}\"", i, field_entry.path))); + } rb_ary_store(fields, static_cast(i), entry); } rb_ary_store(res, static_cast(j), lookup_in_entry_res); diff --git a/lib/couchbase/collection.rb b/lib/couchbase/collection.rb index ef00aeb7..0371f2dc 100644 --- a/lib/couchbase/collection.rb +++ b/lib/couchbase/collection.rb @@ -482,6 +482,7 @@ def lookup_in(id, specs, options = Options::LookupIn::DEFAULT) f.index = field[:index] f.path = field[:path] f.value = field[:value] + f.error = field[:error] end end end @@ -649,6 +650,7 @@ def extract_lookup_in_replica_result(resp, options) f.index = field[:index] f.path = field[:path] f.value = field[:value] + f.error = field[:error] end end end diff --git a/lib/couchbase/collection_options.rb b/lib/couchbase/collection_options.rb index 3702d1fa..6cfc70a1 100644 --- a/lib/couchbase/collection_options.rb +++ b/lib/couchbase/collection_options.rb @@ -167,6 +167,8 @@ class LookupInResult # @return [Object] the decoded def content(path_or_index, transcoder = self.transcoder) field = get_field_at_index(path_or_index) + + raise field.error unless field.error.nil? raise Error::PathNotFound, "Path is not found: #{path_or_index}" unless field.exists transcoder.decode(field.value, :json) @@ -189,6 +191,8 @@ def exists?(path_or_index) end return false unless field + raise field.error unless field.error.nil? + field.exists end @@ -306,6 +310,9 @@ class SubDocumentField # @return [String] path attr_accessor :path + # @return [CouchbaseError] error + attr_accessor :error + # @yieldparam [SubDocumentField] self def initialize yield self if block_given? diff --git a/test/subdoc_test.rb b/test/subdoc_test.rb index 4c02eb60..438d5ea9 100644 --- a/test/subdoc_test.rb +++ b/test/subdoc_test.rb @@ -1448,5 +1448,123 @@ def test_lookup_in_all_replicas_bad_key ]) end end + + def test_lookup_in_path_invalid + doc_id = uniq_id(:foo) + document = {"value" => 42} + @collection.upsert(doc_id, document) + + res = @collection.lookup_in(doc_id, [ + LookupInSpec.get("value.."), + ]) + + assert_raises(Error::PathInvalid) do + res.exists?(0) + end + assert_raises(Error::PathInvalid) do + res.content(0) + end + end + + def test_lookup_in_path_mismatch + doc_id = uniq_id(:foo) + document = {"value" => 42} + @collection.upsert(doc_id, document) + + res = @collection.lookup_in(doc_id, [ + LookupInSpec.count("value"), + ]) + + assert_raises(Error::PathMismatch) do + res.exists?(0) + end + assert_raises(Error::PathMismatch) do + res.content(0) + end + end + + def test_lookup_in_any_replica_path_invalid + skip("#{name}: CAVES does not support subdoc read from replica yet") if use_caves? + skip("#{name}: Server does not support subdoc read from replica") unless env.server_version.supports_subdoc_read_from_replica? + + doc_id = uniq_id(:foo) + document = {"value" => 42} + @collection.upsert(doc_id, document, Options::Upsert.new(durability_level: :majority_and_persist_to_active)) + + res = @collection.lookup_in_any_replica(doc_id, [ + LookupInSpec.count("value.."), + ]) + + assert_raises(Error::PathInvalid) do + res.exists?(0) + end + assert_raises(Error::PathInvalid) do + res.content(0) + end + end + + def test_lookup_in_any_replica_path_mismatch + skip("#{name}: CAVES does not support subdoc read from replica yet") if use_caves? + skip("#{name}: Server does not support subdoc read from replica") unless env.server_version.supports_subdoc_read_from_replica? + + doc_id = uniq_id(:foo) + document = {"value" => 42} + @collection.upsert(doc_id, document, Options::Upsert.new(durability_level: :majority_and_persist_to_active)) + + res = @collection.lookup_in_any_replica(doc_id, [ + LookupInSpec.count("value"), + ]) + + assert_raises(Error::PathMismatch) do + res.exists?(0) + end + assert_raises(Error::PathMismatch) do + res.content(0) + end + end + + def test_lookup_in_all_replicas_path_invalid + skip("#{name}: CAVES does not support subdoc read from replica yet") if use_caves? + skip("#{name}: Server does not support subdoc read from replica") unless env.server_version.supports_subdoc_read_from_replica? + + doc_id = uniq_id(:foo) + document = {"value" => 42} + @collection.upsert(doc_id, document, Options::Upsert.new(durability_level: :majority_and_persist_to_active)) + + res = @collection.lookup_in_all_replicas(doc_id, [ + LookupInSpec.count("value.."), + ]) + + res.each do |entry| + assert_raises(Error::PathInvalid) do + entry.exists?(0) + end + assert_raises(Error::PathInvalid) do + entry.content(0) + end + end + end + + def test_lookup_in_all_replicas_path_mismatch + skip("#{name}: CAVES does not support subdoc read from replica yet") if use_caves? + skip("#{name}: Server does not support subdoc read from replica") unless env.server_version.supports_subdoc_read_from_replica? + + doc_id = uniq_id(:foo) + document = {"value" => 42} + @collection.upsert(doc_id, document, Options::Upsert.new(durability_level: :majority_and_persist_to_active)) + + res = @collection.lookup_in_all_replicas(doc_id, [ + LookupInSpec.count("value"), + ]) + + res.each do |entry| + assert_raises(Error::PathMismatch) do + entry.exists?(0) + end + assert_raises(Error::PathMismatch) do + entry.content(0) + end + end + end end end