Skip to content

Commit

Permalink
Fix querying with a path into nested collections with wildcards (#7404)
Browse files Browse the repository at this point in the history 
Comparing a collection with a list could fail if there was wildcards
in the path and therefore multiple collections to compare with right
hand list.

Linklist is implicitly having wildcard in the path, so if linklists is
in the path there will be a similar problem.  Do not merge values
from different objects into a common list in queries.
  • Loading branch information
@jedelbo
jedelbo authored Mar 7, 2024
1 parent 2761d3a commit edf7064
Show file tree
Hide file tree
Showing 8 changed files with 414 additions and 206 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@
### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* Fixed an issue when removing items from a LnkLst that could result in invalidated links becoming visable which could cause crashes or exceptions when accessing those list items later on. This affects sync Realms where another client had previously removed a link in a linklist that has over 1000 links in it, and then further local removals from the same list caused the list to have fewer than 1000 items. ([#7414](https://github.com/realm/realm-core/pull/7414), since v10.0.0)
* Query lists vs lists if the property to check is a path with wildcards would not give correct result. This has for a long time also been a problem for queries with linklist properties in the path ([#7393](https://github.com/realm/realm-core/issues/7393), since v14.0.0)

### Breaking changes
* None.
* The fix of querying involving multiple lists may cause tests that depended on the broken beharior to fail.

### Compatibility
* Fileformat: Generates files with format v24. Reads and automatically upgrade from fileformat v10. If you want to upgrade from an earlier file format version you will have to use RealmCore v13.x.y or earlier.
Expand Down
41 changes: 24 additions & 17 deletions src/realm/collection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,20 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
{
auto path_size = ctrl.path.size() - index;
PathElement& pe = ctrl.path[index];
bool end_of_path = path_size == 1;

if (end_of_path) {
ctrl.matches.emplace_back();
}

if (val.is_type(type_Dictionary) && (pe.is_key() || pe.is_all())) {
auto ref = val.get_ref();
if (!ref)
return;
Array top(ctrl.alloc);
Array top(*ctrl.alloc);
top.init_from_ref(ref);

BPlusTree<StringData> keys(ctrl.alloc);
BPlusTree<StringData> keys(*ctrl.alloc);
keys.set_parent(&top, 0);
keys.init_from_parent();
size_t start = 0;
Expand All @@ -156,28 +162,29 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
start = keys.find_first(StringData(pe.get_key()));
if (start == realm::not_found) {
if (pe.get_key() == "@keys") {
ctrl.from_list = true;
ctrl.path_only_unary_keys = false;
REALM_ASSERT(end_of_path);
keys.for_all([&](const auto& k) {
ctrl.matches.insert(k);
ctrl.matches.back().push_back(k);
});
}
else {
ctrl.matches.insert(Mixed());
ctrl.matches.back().push_back(Mixed());
}
return;
}
finish = start + 1;
}
BPlusTree<Mixed> values(ctrl.alloc);
BPlusTree<Mixed> values(*ctrl.alloc);
values.set_parent(&top, 1);
values.init_from_parent();
for (; start < finish; start++) {
val = values.get(start);
if (path_size > 1) {
Collection::get_any(ctrl, val, index + 1);
if (end_of_path) {
ctrl.matches.back().push_back(val);
}
else {
ctrl.matches.insert(val);
Collection::get_any(ctrl, val, index + 1);
}
}
}
Expand All @@ -186,7 +193,7 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
auto ref = val.get_ref();
if (!ref)
return;
BPlusTree<Mixed> list(ctrl.alloc);
BPlusTree<Mixed> list(*ctrl.alloc);
list.init_from_ref(ref);
if (size_t sz = list.size()) {
size_t start = 0;
Expand All @@ -202,11 +209,11 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
}
for (; start < finish; start++) {
val = list.get(start);
if (path_size > 1) {
Collection::get_any(ctrl, val, index + 1);
if (end_of_path) {
ctrl.matches.back().push_back(val);
}
else {
ctrl.matches.insert(val);
Collection::get_any(ctrl, val, index + 1);
}
}
}
Expand All @@ -217,15 +224,15 @@ void Collection::get_any(QueryCtrlBlock& ctrl, Mixed val, size_t index)
auto col = obj.get_table()->get_column_key(pe.get_key());
if (col) {
val = obj.get_any(col);
if (path_size > 1) {
if (end_of_path) {
ctrl.matches.back().push_back(val);
}
else {
if (val.is_type(type_Link)) {
val = ObjLink(obj.get_target_table(col)->get_key(), val.get<ObjKey>());
}
Collection::get_any(ctrl, val, index + 1);
}
else {
ctrl.matches.insert(val);
}
}
}
}
Expand Down
17 changes: 5 additions & 12 deletions src/realm/collection.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,18 +105,11 @@ class Collection {
virtual StablePath get_stable_path() const = 0;

struct QueryCtrlBlock {
QueryCtrlBlock(Path& p, const Table& table, bool is_from_list)
: path(p)
, from_list(is_from_list)
, alloc(table.get_alloc())
, group(table.get_parent_group())
{
}
Path& path;
std::set<Mixed> matches;
bool from_list;
Allocator& alloc;
Group* group;
Path path;
std::vector<std::vector<Mixed>> matches;
bool path_only_unary_keys = false; // Not from list
Allocator* alloc = nullptr;
Group* group = nullptr;
};
static void get_any(QueryCtrlBlock&, Mixed, size_t);
};
Expand Down
4 changes: 4 additions & 0 deletions src/realm/obj.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1979,6 +1979,10 @@ Dictionary Obj::get_dictionary(ColKey col_key) const
Obj& Obj::set_collection(ColKey col_key, CollectionType type)
{
REALM_ASSERT(col_key.get_type() == col_type_Mixed);
if ((col_key.is_dictionary() && type == CollectionType::Dictionary) ||
(col_key.is_list() && type == CollectionType::List)) {
return *this;
}
update_if_needed();
Mixed new_val(0, type);

Expand Down
125 changes: 62 additions & 63 deletions src/realm/query_expression.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,19 +270,19 @@ ColumnDictionaryKeys Columns<Dictionary>::keys()

void Columns<Dictionary>::init_path(const PathElement* begin, const PathElement* end)
{
m_path.clear();
m_path_only_unary_keys = true;
m_ctrl.path.clear();
m_ctrl.path_only_unary_keys = true;
while (begin != end) {
if (begin->is_all()) {
m_path_only_unary_keys = false;
m_ctrl.path_only_unary_keys = false;
}
m_path.emplace_back(std::move(*begin));
m_ctrl.path.emplace_back(std::move(*begin));
++begin;
}
std::move(begin, end, std::back_inserter(m_path));
if (m_path.empty()) {
m_path_only_unary_keys = false;
m_path.push_back(PathElement::AllTag());
std::move(begin, end, std::back_inserter(m_ctrl.path));
if (m_ctrl.path.empty()) {
m_ctrl.path_only_unary_keys = false;
m_ctrl.path.push_back(PathElement::AllTag());
}
}

Expand All @@ -297,28 +297,27 @@ void ColumnDictionaryKeys::set_cluster(const Cluster* cluster)
}
}


void ColumnDictionaryKeys::evaluate(size_t index, ValueBase& destination)
void ColumnDictionaryKeys::evaluate(Subexpr::Index& index, ValueBase& destination)
{
if (m_link_map.has_links()) {
REALM_ASSERT(!m_leaf);
std::vector<ObjKey> links = m_link_map.get_links(index);
auto sz = links.size();

// Here we don't really know how many values to expect
std::vector<Mixed> values;
for (size_t t = 0; t < sz; t++) {
const Obj obj = m_link_map.get_target_table()->get_object(links[t]);
auto dict = obj.get_dictionary(m_column_key);
// Insert all values
dict.for_all_keys<StringData>([&values](const Mixed& value) {
values.emplace_back(value);
});
if (index.initialize()) {
m_links.clear();
REALM_ASSERT(!m_leaf);
m_links = m_link_map.get_links(index);
if (!index.set_size(m_links.size())) {
destination.init(true, 0);
return;
}
}

// Copy values over
destination.init(true, values.size());
destination.set(values.begin(), values.end());
const Obj obj = m_link_map.get_target_table()->get_object(m_links[index.get_and_incr_sub_index()]);
auto dict = obj.get_dictionary(m_column_key);
destination.init(true, dict.size());
// Insert all values
size_t n = 0;
dict.for_all_keys<StringData>([&](const Mixed& value) {
destination.set(n, value);
n++;
});
}
else {
// Not a link column
Expand Down Expand Up @@ -350,11 +349,11 @@ class DictionarySize : public Columns<Dictionary> {
: Columns<Dictionary>(other)
{
}
void evaluate(size_t index, ValueBase& destination) override
void evaluate(Subexpr::Index& index, ValueBase& destination) override
{
Allocator& alloc = this->m_link_map.get_target_table()->get_alloc();
Value<int64_t> list_refs;
this->get_lists(index, list_refs, 1);
this->get_lists(index, list_refs);
destination.init(list_refs.m_from_list, list_refs.size());
for (size_t i = 0; i < list_refs.size(); i++) {
ref_type ref = to_ref(list_refs[i].get_int());
Expand All @@ -375,41 +374,46 @@ SizeOperator<int64_t> Columns<Dictionary>::size()
return SizeOperator<int64_t>(std::move(ptr));
}

void Columns<Dictionary>::evaluate(size_t index, ValueBase& destination)
void Columns<Dictionary>::evaluate(Subexpr::Index& index, ValueBase& destination)
{
Collection::QueryCtrlBlock ctrl(m_path, *m_link_map.get_target_table(), !m_path_only_unary_keys);

if (links_exist()) {
REALM_ASSERT(!m_leaf);
std::vector<ObjKey> links = m_link_map.get_links(index);
auto sz = links.size();
if (!m_link_map.only_unary_links())
ctrl.from_list = true;
if (index.initialize()) {
m_ctrl.matches.clear();
if (links_exist()) {
REALM_ASSERT(!m_leaf);
std::vector<ObjKey> links = m_link_map.get_links(index);
auto sz = links.size();
if (!m_link_map.only_unary_links())
m_ctrl.path_only_unary_keys = false;

for (size_t t = 0; t < sz; t++) {
const Obj obj = m_link_map.get_target_table()->get_object(links[t]);
auto val = obj.get_any(m_column_key);
if (!val.is_null()) {
Collection::get_any(ctrl, val, 0);
for (size_t t = 0; t < sz; t++) {
const Obj obj = m_link_map.get_target_table()->get_object(links[t]);
auto val = obj.get_any(m_column_key);
if (!val.is_null()) {
Collection::get_any(m_ctrl, val, 0);
}
}
}
}
else {
// Not a link column
REALM_ASSERT(m_leaf);
if (ref_type ref = to_ref(m_leaf->get(index))) {
Collection::get_any(ctrl, {ref, CollectionType::Dictionary}, 0);
else {
// Not a link column
REALM_ASSERT(m_leaf);
if (ref_type ref = to_ref(m_leaf->get(index))) {
Collection::get_any(m_ctrl, {ref, CollectionType::Dictionary}, 0);
}
}
if (!index.set_size(m_ctrl.matches.size())) {
destination.init(true, 0);
return;
}
}

// Copy values over
auto sz = ctrl.matches.size();
destination.init(ctrl.from_list || sz == 0, sz);
destination.set(ctrl.matches.begin(), ctrl.matches.end());
auto& matches = m_ctrl.matches[index.get_and_incr_sub_index()];
auto sz = matches.size();
destination.init(!m_ctrl.path_only_unary_keys || sz == 0, sz);
destination.set(matches.begin(), matches.end());
}


void Columns<Link>::evaluate(size_t index, ValueBase& destination)
void Columns<Link>::evaluate(Subexpr::Index& index, ValueBase& destination)
{
// Destination must be of Key type. It only makes sense to
// compare keys with keys
Expand Down Expand Up @@ -440,7 +444,7 @@ void ColumnListBase::set_cluster(const Cluster* cluster)
}
}

void ColumnListBase::get_lists(size_t index, Value<int64_t>& destination, size_t nb_elements)
void ColumnListBase::get_lists(size_t index, Value<int64_t>& destination)
{
if (m_link_map.has_links()) {
std::vector<ObjKey> links = m_link_map.get_links(index);
Expand All @@ -465,17 +469,12 @@ void ColumnListBase::get_lists(size_t index, Value<int64_t>& destination, size_t
}
}
else {
size_t rows = std::min(m_leaf->size() - index, nb_elements);

destination.init(false, rows);

for (size_t t = 0; t < rows; t++) {
destination.set(t, m_leaf->get(index + t));
}
destination.init(false, 1);
destination.set(0, m_leaf->get(index));
}
}

void LinkCount::evaluate(size_t index, ValueBase& destination)
void LinkCount::evaluate(Subexpr::Index& index, ValueBase& destination)
{
if (m_column_key) {
REALM_ASSERT(m_link_map.has_links());
Expand Down
Loading

0 comments on commit edf7064

Please sign in to comment.