Refactor link tracing code #5796

ironage · 2022-08-30T22:47:17Z

It is a common pattern to iterate through all backlinks of an object to do something with the associated forward links. But it is far too easy to forget about handling a link type and we have had several bugs due to this in the past. Using the new LinkTranslator type ensures that all link types are explicitly handled by overriding the appropriate method. This also makes it clear when reading the code, that if a certain type of link was not handled, it was intentional and not an error.

There is one functional change, which is to disallow migrating to an embedded table if we have links from a Mixed property. We do not handle this in many places yet so we should not allow these types of objects to sneak in until we add official support.

nicola-cab

This is wonderful. Just, I believe we should move all the utilities you wrote within new header/source files in order to avoid treating them as "additions" to Obj/Table. Essentially, you have introduced a standard way to deal with links.

nicola-cab · 2022-08-31T08:28:31Z

src/realm/obj.cpp

@@ -696,6 +696,61 @@ size_t Obj::get_backlink_cnt(ColKey backlink_col) const

 void Obj::traverse_path(Visitor v, PathSizer ps, size_t path_length) const
 {
+    struct BacklinkTraverser : public LinkTranslator {


Personally, I think this belongs to some other compilation unit (new header + source file). This applies to all the other classes you defined within a method.

nicola-cab · 2022-08-31T08:29:11Z

src/realm/obj.cpp

-    if (attr.test(col_attr_List)) {
-        if (origin_col_key.get_type() == col_type_LinkList) {
-            nullify_linklist(*this, origin_col_key, target_link.get_obj_key());
+    struct LinkNullifier : public LinkTranslator {


Same as above.

nicola-cab · 2022-08-31T08:29:43Z

src/realm/obj.cpp

@@ -1934,6 +1971,80 @@ bool Obj::remove_backlink(ColKey col_key, ObjLink old_link, CascadeState& state)
    return false;
 }

+struct EmbeddedObjectLinkMigrator : public LinkTranslator {


needs to be moved into a separate header/source file IMO.

nicola-cab · 2022-08-31T08:30:17Z

src/realm/obj.cpp

@@ -2049,6 +2121,83 @@ LinkCollectionPtr Obj::get_linkcollection_ptr(ColKey col_key) const

 void Obj::assign_pk_and_backlinks(const Obj& other)
 {
+    struct LinkReplacer : LinkTranslator {


same as per above comments.

nicola-cab · 2022-08-31T08:31:32Z

src/realm/table.cpp

@@ -333,6 +333,72 @@ void LinkChain::add(ColKey ck)
    m_link_cols.push_back(ck);
 }

+LinkTranslator::LinkTranslator(Obj origin, ColKey origin_col_key)


I would have this defined into a separate header/source file, this is basically a basic class to deal with links from now on..

ironage · 2022-08-31T17:37:17Z

@nicola-cab I've moved the base LinkTranslator into separate files. But I would prefer to keep the specialized classes close to where they are used in the code for readability; those classes are not meant to be reused by anything else.

nicola-cab

LGTM

ironage · 2022-09-06T19:27:13Z

I'd like to get another review on this one, pinging @finnschiermer or @jedelbo

jedelbo · 2022-09-12T15:00:51Z

src/realm/link_translator.hpp

+
+namespace realm {
+
+struct LinkTranslator {


Why not class?

jedelbo · 2022-09-12T15:01:37Z

src/realm/link_translator.hpp

+
+namespace realm {
+
+struct LinkTranslator {


Could you add a comment on how and where this class is supposed to be used

jedelbo · 2022-09-12T15:13:44Z

src/realm/obj.cpp

@@ -1798,92 +1840,88 @@ inline void nullify_set(Obj& obj, ColKey origin_col_key, T target)
 }
 } // namespace

+template <class ArrayType, class ValueType>


ArrayType can be deducted from ColumnTypeTraits<ValueType>::cluster_leaf_type

jedelbo · 2022-09-12T15:17:09Z

src/realm/link_translator.hpp

+struct LinkTranslator {
+    LinkTranslator(Obj origin, ColKey origin_col_key);
+    void run();
+    virtual void on_list_of_links(LnkLst list) = 0;


I think we should use references for the collection types

jedelbo · 2022-09-12T15:34:27Z

src/realm/list.cpp

@@ -280,6 +280,8 @@ void Lst<ObjKey>::do_clear()
        target_obj.remove_one_backlink(backlink_col, m_obj.get_key()); // Throws
        size_t num_remaining = target_obj.get_backlink_count(*origin_table, m_col_key);
        if (num_remaining == 0) {


Perhaps the above check can be removed?

…nification * origin/master: Stop forcing enums to be 64 bits unnecessarily clean up documentation of internal fields in config structs SyncConfig should be default constructible Traversal functions use IteratorControl values rather than true/false which is more expressive (#5857) Fix handling of 4-byte UTF8 values on Windows (#5803) Encode links in a way the server can understand (#5835) expose `Group::remove_table` in the C API (#5860) Disable auto refresh for old realm during migration (#5856) Expose `list_find` in the c api (#5848) Do not allow asymmetric tables in pbs (#5853) Refactor link tracing code (#5796) Expose `Obj::get_parent_object` in the C API (#5851) Update app.hpp (#5854) Fix appending to list ignores existing query (#5850)

ironage requested review from jedelbo, nicola-cab and finnschiermer August 30, 2022 22:47

ironage self-assigned this Aug 30, 2022

cla-bot bot added the cla: yes label Aug 30, 2022

ironage force-pushed the js/backlink-types branch from 4a66261 to 728503e Compare August 30, 2022 22:50

refactor link tracing code

0634e94

ironage force-pushed the js/backlink-types branch from 728503e to 0634e94 Compare August 31, 2022 00:12

nicola-cab requested changes Aug 31, 2022

View reviewed changes

ironage added 3 commits August 31, 2022 10:02

Merge branch 'master' into js/backlink-types

e30c1dc

LinkTranslator gets a separate file

be7f753

update changelog

eeb942d

nicola-cab approved these changes Sep 1, 2022

View reviewed changes

jedelbo reviewed Sep 12, 2022

View reviewed changes

nicola-cab mentioned this pull request Sep 12, 2022

Je/dictionary reform #5780

Merged

3 tasks

ironage added 2 commits September 12, 2022 11:02

Merge branch 'master' into js/backlink-types

98201ca

review feedback

b8cac8f

ironage requested a review from jedelbo September 12, 2022 19:04

jedelbo approved these changes Sep 14, 2022

View reviewed changes

ironage merged commit 267fa66 into master Sep 14, 2022

ironage deleted the js/backlink-types branch September 14, 2022 17:11

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor link tracing code #5796

Refactor link tracing code #5796

ironage commented Aug 30, 2022

nicola-cab left a comment

nicola-cab Aug 31, 2022

nicola-cab Aug 31, 2022

nicola-cab Aug 31, 2022

nicola-cab Aug 31, 2022

nicola-cab Aug 31, 2022

ironage commented Aug 31, 2022

nicola-cab left a comment

ironage commented Sep 6, 2022

jedelbo Sep 12, 2022

jedelbo Sep 12, 2022

jedelbo Sep 12, 2022

jedelbo Sep 12, 2022

jedelbo Sep 12, 2022

Refactor link tracing code #5796

Refactor link tracing code #5796

Conversation

ironage commented Aug 30, 2022

nicola-cab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironage commented Aug 31, 2022

nicola-cab left a comment

Choose a reason for hiding this comment

ironage commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment