Je/rcore 368 #4167
Je/rcore 368 #4167
Conversation
|
@tgoyne how would you suggest this should be fixed? As I can see it, |
| if (m_info->tables.count(table_key.value)) { | ||
| auto& changes = m_info->tables[table_key.value]; | ||
| for (auto& key_val : m_previous_rows) { | ||
| if (changes.get_deletions().count(key_val)) { | ||
| key_val = -1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| if (m_info->tables.count(table_key.value)) { | |
| auto& changes = m_info->tables[table_key.value]; | |
| for (auto& key_val : m_previous_rows) { | |
| if (changes.get_deletions().count(key_val)) { | |
| key_val = -1; | |
| } | |
| } | |
| } | |
| if (auto it = m_info->tables.find(table_key.value); it != m_info->tables.end()) { | |
| auto& changes = *it; | |
| for (auto& key_val : m_previous_rows) { | |
| if (changes.deletions_contains(key_val)) { | |
| key_val = -1; | |
| } | |
| } | |
| } |
Checking get_deletions() does the wrong thing for a table clear, and using find() eliminates a double lookup in the set (which probably doesn't matter much...). This does point out that we also need to test table clear followed by creating an object with one of the deleted PKs (and resolve the FIXME in deletions_contains() by changing the check on insertions to return true).
There was a problem hiding this comment.
It turns out that "clear_table" is no longer issued. We will have a deletion instruction for every object. So that code is removed.
| @@ -3525,3 +3525,88 @@ TEST_CASE("results: query helpers", "[include]") { | |||
| CHECK(includes.get_description(table) == "INCLUDE(@links.class_linking_object.link)"); | |||
| } | |||
| } | |||
|
|
|||
| TEST_CASE("notifications: objects with PK recreated") { | |||
There was a problem hiding this comment.
This is a lot of duplicated code. The test should just be another section in the existing test case rather than copying and pasting all the setup.
There was a problem hiding this comment.
Actually, the setup is different.
|
@tgoyne Any more comments? |
| r->read_group().get_table("class_string_pk")->remove_object(k3); | ||
| create("no_pk", AnyDict{{"id", INT64_C(123)}, {"value", INT64_C(200)}}); | ||
| create("int_pk", AnyDict{{"id", INT64_C(456)}, {"value", INT64_C(200)}}); | ||
| create("string_pk", AnyDict{{"id", std::string("hello")}, {"value", INT64_C(200)}}); |
There was a problem hiding this comment.
What is the expected notification changes for this event? Can you add a check that this is being observed as CollectionChangeSet::modifications and not a deletion and insertion? at least I think that is the expected behaviour, except for maybe the no_pk case where we don't guarantee reuse of auto generated pks?
There was a problem hiding this comment.
This should be reported as a delete and insert. All of these tests need to be validating the changeset and not just that a notification was sent.
There was a problem hiding this comment.
The test is now enhanced with those tests.
What, How & Why?
Related to #4010
☑️ ToDos