Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for subcategory backfill (redo) #93

Closed
wants to merge 6 commits into from

Conversation

vazome
Copy link

@vazome vazome commented Nov 3, 2024

Change Summary

@vazome – Improves upon #72. I took into account that orginal file was renamed and put subcategory backfill logic explanation into REAME.
Tested this change in Cloud Functions, it works.

@guenth39 – With these changes, the backfill will now work also for subcollections, not only for "root" collection. If the collection path is something like users/{userId}/items the sync has already worked, but the backfill did not as the documents were queried with this collection path, but that did not work. Instead, we use the collectionGroup feature now with only the last part of the path.

PR Checklist

@vazome
Copy link
Author

vazome commented Nov 3, 2024

It closes: #17

@vazome
Copy link
Author

vazome commented Nov 3, 2024

FYI @guenth39 @jasonbosco

I have fixed previous issues

Copy link
Member

@jasonbosco jasonbosco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vazome Thank you for the PR! Could you add an integration test with a sub-collection?

@vazome
Copy link
Author

vazome commented Nov 4, 2024

@jasonbosco could you please elaborate, I should make a test in the code itself or something different?

UPD: okay let's see

@jasonbosco
Copy link
Member

You want to create a new test file like this one that reads the collection name configuration from new test params file like this, but has a nested collection name specified in it.

And then update the test runner here and add a test part 3 may be, to run the test in CI

@vazome
Copy link
Author

vazome commented Nov 17, 2024

@jasonbosco tests included, please verify

UPD: fixed syntax error

@vazome
Copy link
Author

vazome commented Nov 17, 2024

Okay, I see the errors, forgot to do ESlint

@vazome
Copy link
Author

vazome commented Nov 17, 2024

Corrected the wrong indent and missing new line

@tharropoulos
Copy link
Collaborator

Hey, I saw that the tests were failing, so I took a closer look and tried to debug it myself. Turns out, if you log the value:

      const typesenseDocsStr = await typesense.collections(encodeURIComponent(typesenseSubcollectionName)).retrieve();
      console.dir(typesenseDocsStr);

The collection has no documents present:

    {
      created_at: 1731922232,
      default_sorting_field: '',
      enable_nested_fields: true,
      fields: [
        {
          facet: false,
          index: true,
          infix: false,
          locale: '',
          name: '.*',
          optional: true,
          sort: false,
          stem: false,
          store: true,
          type: 'auto'
        }
      ],
      name: 'books_firestore/1/subcollection',
      num_documents: 0,
      symbols_to_index: [],
      token_separators: []
    }

What's also interesting is one in two runs, the test fails to delete the existing collection, resulting in a 409 from the server

@jsasitorn
Copy link
Contributor

#99 was merged, and latest builds should support subcollections now

@vazome vazome closed this Dec 20, 2024
@vazome vazome deleted the master branch December 20, 2024 16:49
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.

4 participants