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

Fix backfill for subcollections #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guenth39
Copy link

Change Summary

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.

Closes #17

PR Checklist

@guenth39 guenth39 mentioned this pull request Apr 21, 2023
1 task
@bokzor
Copy link

bokzor commented May 18, 2023

Hi,
Any chance to merge that MR ?

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.

Could you also add test cases?

@@ -40,8 +40,16 @@ module.exports = functions.handler.firestore.document
return false;
}

const querySnapshot =
await admin.firestore().collection(config.firestoreCollectionPath).get();
const isSubcollection = config.firestoreCollectionPath.includes("/");
Copy link
Member

Choose a reason for hiding this comment

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

Is there a case where a collection path can include / and it's not a sub-collection?

For eg, let's say a set of admin users are nested under users/admin, and regular users are nested under users/regular?

Choose a reason for hiding this comment

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

This cannot be the case. users/admin and users/regular are not valid subcollections. In that example there would be a document with the id admin under the users collection and a document with the id regular under the users collection. The correct syntax for subcollections would be users/{userId}/admin and users/{userId}/regular

let query;
if (isSubcollection) {
const collectionGroup = config.firestoreCollectionPath.split("/").pop();
query = admin.firestore().collectionGroup(collectionGroup);
Copy link
Member

Choose a reason for hiding this comment

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

Does this support collections with dynamic path names? Eg: /users/{user_id}/companies

Choose a reason for hiding this comment

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

Yes it will work like this

@marcelweigle
Copy link

marcelweigle commented Jul 28, 2023

Hey, any Chance to get this merged soon? This feature is really needed and the PR looks good for me. This is blocking my migration from Algolia to typesense.

@LincolnRychecky
Copy link

I am also being blocked by this. The sooner this can get merged the better.

@jasonbosco
Copy link
Member

@guenth39 Could you add test cases for this PR?

@bokzor
Copy link

bokzor commented Sep 14, 2023

In case you don't want to wait this MR to be merged. You can directly modify the cloud function via the google console.
https://console.cloud.google.com/functions/list

And replace the backfill function with : https://github.com/Bobbele-Ideas/firestore-typesense-search/blob/Fix-backfill-for-subcollections/functions/src/backfillToTypesenseFromFirestore.js

@bokzor
Copy link

bokzor commented Feb 8, 2024

@jasonbosco

Any update on this issue ? In multi tenants application this features is mandatory.

@Ihatetomatoes
Copy link

+1 on this one, would be great to be able to backfill sub-collections.

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.

Support backfill documents in subcollections
6 participants