Skip to content

Conversation

joepio
Copy link
Member

@joepio joepio commented Jan 15, 2022

TODO:

  • Build the cache
  • Get rid of .unwrap() in update_member closures
  • Load from cache when possible
  • Update CollectionWatcher dynamically

PR Checklist:

  • Link to related issue Collection caching #114
  • Add changelog entry linking to issue
  • Add tests
  • Add tests for authorization
  • Add tests for changing data, updating index (e.g. remove stuff)
  • More tests?

@joepio
Copy link
Member Author

joepio commented Jan 15, 2022

So the thing I'm currently struggling with, is performing the cached query. Currently, Collection does not depend onDb, which means that I can't access Db specific methods from Collection. So should I add collection building methods to Storelike?

Edit: ended up making the Query abstraction!

@joepio
Copy link
Member Author

joepio commented Jan 17, 2022

Having some trouble with conditionally reversing the sled iterator, depending on the sort_desc setting of the user.

    let iter = if q.sort_desc {
        store.members_index.range(start_key..end_key)
    } else {
        store.members_index.range(start_key..end_key).rev()
    };

Fails, because

`if` and `else` have incompatible types
expected type `sled::Iter`
 found struct `std::iter::Rev<sled::Iter>`

So I read about using Box to prevent this, but that also doesn't work:

    let iter = if q.sort_desc {
        Box::new(store.members_index.range(start_key..end_key))
    } else {
        Box::new(store.members_index.range(start_key..end_key).rev())
    };

Fails:

`if` and `else` have incompatible types
expected type `std::boxed::Box<sled::Iter>`
 found struct `std::boxed::Box<std::iter::Rev<sled::Iter>>`rustcE0308

@joepio
Copy link
Member Author

joepio commented Jan 19, 2022

The changing data test is failing, because the cache is not properly invalidated.

We have this logic, that checks an update atom according to query filters that are being watched:

            let should_update = match (&q_filter.property, &q_filter.value) {
                (Some(prop), Some(val)) => prop == &atom.property && val == &atom.value,
                (Some(prop), None) => prop == &atom.property,
                (None, Some(val)) => val == &atom.value,
                // We should not create indexes for Collections that iterate over _all_ resources.
                _ => false,
            };

But... That goes wrong when the sorted value differs from the filtered value. So let's say we have a QueryFilter that filters by is_a human, and sorts by firstname. Let's say we update the firstname. In this case we want the old Atom to be removed from the index. So in this case the should_update function is called for the new firstname atom. This atom does not contain the is_a property or the human value. We want to update the existing indexed value, but we can't, because we can't perform the is_a human check. We need all the properties of the Resource, or we need a pre-confirmed check which collections the resource matches.

We should first check if the sort_property matches, then we should check if the resource has the query, I think. This check should be memoized, probably.

Maybe the problem is more fundamental. The should_update function cannot decide if a value needs to be removed, knowing only the Atom and the FilterQuery.

Flip it

Say we want to check if an atom will match things. We could first check if the resource matches the query filter. If that is the case, we can continue, and perform the existing checks.

@joepio
Copy link
Member Author

joepio commented Jan 22, 2022

So the previous cache invalidation issue has now been solved by passing the old resource to the cache invalidation logic. It is needed to match the QueryFilters in query_index.

Things are working fine if a commit is only applied once, but for the /commit endpoint, I'm actually applying a Commit twice.

@joepio
Copy link
Member Author

joepio commented Jan 24, 2022

I think atoms for the QueryIndex and the ReferenceIndex might need to be a bit different.
The difficult part is ResourceArrays.

Let's consider I want add a Paragraph to a Document.
This means I need to add one Atom to the Index.

ReferenceIndex

  • Needs to update that the Paragraph now has an incoming link from the Document
  • The old paragraph also still needs a reference
  • It is entirely possible that in this Atom, both paragraphs are modified (and not just one new appended item). Just to make sure, we will first have to delete all references from the old atom, and then create the new references from the new atom.
  • This will be an unnecessarily costly operation if we only append an item to an array

QueryIndex

  • If someone has a Query that sorts document by their number of Paragraphs, their query might need to be updated.
  • Note that in this case, the IndexAtom will not have any references - it should have a count

Solution

I think we need to have two value fields in a QueryAtom: both a Filtervalue and a Sortvalue. Or we should accept that we can't sort by count. Which is also acceptable.

@joepio
Copy link
Member Author

joepio commented Jan 24, 2022

I think I finally know what is causing my last failing test: remove_atom_from_index and add_atom_to_index should get passed a different resource, not the same one. Should update this in the apply method of commits

@joepio
Copy link
Member Author

joepio commented Jan 24, 2022

Tests are green! I think I'll merge this soon. But first, I'll look for some extra things to break, test and fix.

@joepio joepio marked this pull request as ready for review January 24, 2022 21:27
@joepio joepio merged commit d9b9fb6 into master Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant