Encode links in a way the server can understand #5835

jedelbo · 2022-09-08T09:57:30Z

What, How & Why?

Fixes #5409

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

src/realm/query_engine.hpp

test/test_parser.cpp

src/realm/query_expression.hpp

test/test_parser.cpp

mpobrien

One other follow-up; on the original ticket description we were talking about two forms of link, one which has the table explicitly set (for a "typed link" or for use in a mixed column) and one which just holds the primary key of the target object, without specifying the target table.
AFAICT this only covers the former case; do we need to also implement the latter as well, or is that a separate task?

ironage · 2022-09-09T18:03:21Z

src/realm/util/serializer.hpp

@@ -109,6 +112,8 @@ struct SerialisationState {
    std::string get_variable_name(ConstTableRef table);
    std::vector<std::string> subquery_prefix_list;
    std::string class_prefix;
+    Group* group;
+    ConstTableRef taget_table;


Suggested change

ConstTableRef taget_table;

ConstTableRef target_table;

ironage · 2022-09-09T18:06:42Z

src/realm/query_expression.hpp

            if constexpr (std::is_same_v<T, TypeOfValue>) {
                return util::serializer::print_value(val.get_type_of_value());
            }
+            else if constexpr (std::is_same_v<T, ObjKey>) {
+                ObjLink link(state.taget_table->get_key(), val.template get<ObjKey>());


I'd prefer to have a defensive check in case the target table is null before dereferencing.

ironage · 2022-09-09T18:09:41Z

src/realm/util/serializer.cpp

    }
    else {
-        ss << "L" << link.get_table_key().value << ":" << link.get_obj_key().value;
+        TableRef target_table = g->get_table(link.get_table_key());


Let's add a null check on the group and fallback to the "Lx:y" syntax if not found.

ironage · 2022-09-09T18:22:35Z

src/realm/util/serializer.cpp

-        ss << "L" << link.get_table_key().value << ":" << link.get_obj_key().value;
+        TableRef target_table = g->get_table(link.get_table_key());
+        if (ColKey pk_col = target_table ? target_table->get_primary_key_column() : ColKey{}) {
+            if (auto obj = target_table->try_get_object(link.get_obj_key())) {


I notice a problem here which is not trivial to solve:

User A makes a subscription which queries to a link that exists locally.

User B deletes the target object.

User A adds another unrelated query to the subscription set.

Realm serializes User A's subscription queries again, but not finding the original target object locally here uses the Lx:y syntax.

Server cannot parse this and sends a subscription error.

We could refuse to serialize this query.

What do you think about giving ObjLink an util::Optional<Mixed> m_pk member field? Then the serializer would use that info to print the primary key directly without looking up the object key which will work even if the object is deleted.

That would be a hack, I think. But are you sure the scenario will play out as described above? As far as I can tell, the query string is stored as text in the file. Will the query ever be serialized again?

Ok, you are correct. A subscription query is serialized once and the text is stored and reused. So maybe this is not a problem after all. @jbreams can you confirm this?

jedelbo · 2022-09-12T09:47:00Z

@mpobrien I suggest that we only support the form where we supply both table and object. I don't see any downsides and it makes implementation much easier.

mpobrien · 2022-09-12T22:06:26Z

@mpobrien I suggest that we only support the form where we supply both table and object. I don't see any downsides and it makes implementation much easier.

Sounds good to me. I think there is one additional consideration related to this for when we get to implementing support for this on the server side. The client's query might try to be querying on a link identity like foo = obj('TableX', '...') but the server schema actually defines that field foo has a relationship with TableY, not TableX. I believe we'd need the server to detect this condition and reject it as an invalid query.

…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)

jedelbo added 2 commits September 2, 2022 11:00

Possible solution to RCORE-1046

b9909f8

Add test

fe77138

jedelbo requested a review from ironage September 8, 2022 09:57

cla-bot bot added the cla: yes label Sep 8, 2022

jedelbo added 2 commits September 8, 2022 11:57

Merge remote-tracking branch 'origin/master' into je/parser-primary-key

f5f7fdb

Update release note

b79e166

jedelbo requested a review from mpobrien September 8, 2022 10:27

ironage reviewed Sep 8, 2022

View reviewed changes

src/realm/query_engine.hpp Outdated Show resolved Hide resolved

src/realm/query_engine.hpp Show resolved Hide resolved

test/test_parser.cpp Outdated Show resolved Hide resolved

test/test_parser.cpp Show resolved Hide resolved

src/realm/query_expression.hpp Outdated Show resolved Hide resolved

jedelbo force-pushed the je/parser-primary-key branch from 111c1b7 to af5ae16 Compare September 9, 2022 11:38

Update after review

8e028eb

jedelbo force-pushed the je/parser-primary-key branch from af5ae16 to 8e028eb Compare September 9, 2022 12:35

mpobrien reviewed Sep 9, 2022

View reviewed changes

test/test_parser.cpp Show resolved Hide resolved

Fix tests

e2a46ad

mpobrien reviewed Sep 9, 2022

View reviewed changes

ironage reviewed Sep 9, 2022

View reviewed changes

jedelbo added 2 commits September 12, 2022 11:47

Update after review (2)

9d61506

Merge branch 'master' into je/parser-primary-key

b4fc1bc

Merge branch 'master' into je/parser-primary-key

b570749

ironage approved these changes Sep 14, 2022

View reviewed changes

jedelbo merged commit 8f49440 into master Sep 15, 2022

jedelbo deleted the je/parser-primary-key branch September 15, 2022 13:59

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

Encode links in a way the server can understand #5835

Encode links in a way the server can understand #5835

jedelbo commented Sep 8, 2022

mpobrien left a comment

ironage Sep 9, 2022

ironage Sep 9, 2022

jedelbo Sep 12, 2022

ironage Sep 9, 2022

jedelbo Sep 12, 2022

ironage Sep 9, 2022

jedelbo Sep 12, 2022

ironage Sep 12, 2022

jedelbo Sep 13, 2022

ironage Sep 13, 2022

jbreams Sep 14, 2022

jedelbo commented Sep 12, 2022

mpobrien commented Sep 12, 2022

Encode links in a way the server can understand #5835

Encode links in a way the server can understand #5835

Conversation

jedelbo commented Sep 8, 2022

What, How & Why?

☑️ ToDos

mpobrien 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

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

jedelbo commented Sep 12, 2022

mpobrien commented Sep 12, 2022