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

Improve support for subcollections by propagating dynamic vars and backfilling collectionGroup #99

Merged
merged 5 commits into from
Dec 18, 2024

Conversation

jsasitorn
Copy link
Contributor

Change Summary

Currently users can provide dynamic collection paths in typesense config, "users/{userId}/books" even though this isn't properly supported. This improves Firebase plugin support for subcollections in indexOnWrite and backfill by the following:

For onWrite, pass context.params so that placeholders in a dynamic collection path, e.g. users/{userId}/books/{bookId}/chapters, are also indexed in typesense, thus allowing querying not only for a chapter with title="Intro" or id=789, but also userId=123 and bookId=456.

For backfill, query using .collectionGroup(collectionGroup), to backfill following a dynamic path: "users/{userId}/books/{bookId}/title".

This draws inspiration from #93 to improve backfills. However, this version adds guards to prevent group backfills for static paths: e.g.,"users/123/books/456/chapters" and unrelated paths, e.g. using .collectionGroup("books") could end up backfilling both "books" and "users/{userId}/books".

Only change to the typesense collection is the inclusion of ids in the dynamic path.

PR Checklist

Copy link
Collaborator

@tharropoulos tharropoulos left a comment

Choose a reason for hiding this comment

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

Just some small nitpicks I noticed

functions/src/utils.js Show resolved Hide resolved
functions/src/utils.js Outdated Show resolved Hide resolved
test/utilsWithFlattening.spec.js Outdated Show resolved Hide resolved
@jsasitorn
Copy link
Contributor Author

updated. let me know any other comments or feedback

@tharropoulos
Copy link
Collaborator

lgtm!

@jasonbosco jasonbosco merged commit 8fcb94e into typesense:master Dec 18, 2024
2 checks passed
@jasonbosco
Copy link
Member

Thank you for the PR @jsasitorn.

This is now available in this RC version: https://github.com/typesense/firestore-typesense-search/releases/tag/v2.0.0-rc.0

Could you install the RC version of the extension using the installation link above and let me know how it goes?

@jsasitorn
Copy link
Contributor Author

jsasitorn commented Dec 19, 2024

@jasonbosco tested both indexOnWrite and backfill on my data set (~600 entries), data appears to be all there. Noticed some innocuous errors addressed in #100.

Also, couple of things I noticed testing:

  1. Might be useful to note which permissions are required: indexOnWrite uses "documents:create","documents:update"," while backfill will also need "documents:import". Error from typesense is the generic invalid key, so wasn't clear.

  2. This debug is pretty useful for debugging, but think its dumping more than it should in a steady production setup:

    console.log("typesenseDocument", typesenseDocument);

@jasonbosco
Copy link
Member

Thanks @jsasitorn. Published PR #100 in v2.0.0-rc.1

Might be useful to note which permissions are required: indexOnWrite uses "documents:create","documents:update"," while backfill will also need "documents:import". Error from typesense is the generic invalid key, so wasn't clear.

@tharropoulos could you add this to the README?

This debug is pretty useful for debugging, but think its dumping more than it should in a steady production setup:

Yeah, may be we should add a debug: true key to the typesense_sync collection and only log that particular line when it's set to true. Do you want to do a PR for this?

@jsasitorn
Copy link
Contributor Author

@jasonbosco adding a firestore call to typesense_sync is a little heavy since typesenseDocumentFromSnapshot is also called in .onWrite for every object update. What do you think of adding debug:true/false to extension.yaml and piping it in through config.js? Takes a couple min to update after toggling, but would this action is infrequent.

@jsasitorn jsasitorn deleted the subcollection branch December 20, 2024 02:01
@vazome
Copy link

vazome commented Dec 20, 2024

Thank you all for this!

@jasonbosco
Copy link
Member

@jsasitorn Good point. Adding a debug mode to extensions.yml sounds good to me.

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