Skip to content

Commit

Permalink
RCBC-451 & RCBC-452: Expose any specific lookup_in spec errors (#118)
Browse files Browse the repository at this point in the history
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 <sergey.avseyev@gmail.com>
  • Loading branch information
DemetrisChr and avsej committed Aug 17, 2023
1 parent 4424bcd commit cde5fb2
Show file tree
Hide file tree
Showing 5 changed files with 150 additions and 1 deletion.
22 changes: 22 additions & 0 deletions ext/couchbase.cxx
Expand Up @@ -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));
Expand All @@ -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<long>(i), entry);
}
return res;
Expand Down Expand Up @@ -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();
Expand All @@ -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<long>(i), entry);
}

Expand Down Expand Up @@ -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();
Expand All @@ -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<long>(i), entry);
}
rb_ary_store(res, static_cast<long>(j), lookup_in_entry_res);
Expand Down
2 changes: 2 additions & 0 deletions lib/couchbase/collection.rb
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions lib/couchbase/collection_options.rb
Expand Up @@ -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)
Expand All @@ -189,6 +191,8 @@ def exists?(path_or_index)
end
return false unless field

raise field.error unless field.error.nil?

field.exists
end

Expand Down Expand Up @@ -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?
Expand Down
118 changes: 118 additions & 0 deletions test/subdoc_test.rb
Expand Up @@ -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

0 comments on commit cde5fb2

Please sign in to comment.