search

typesense / firestore-typesense-search

Firebase Extension to automatically push Firestore documents to Typesense for full-text search with typo tolerance, faceting, and more
https://extensions.dev/extensions/typesense/firestore-typesense-search
Apache License 2.0
159 stars 35 forks source link

Fix/batch backfill v18 #70

Closed DavidWeiss2 closed 2 months ago

DavidWeiss2 commented 11 months ago

Change Summary

The current batch mechanism is failing on Firestore big collections with a function crash. This PR fixing this issue.

Also on this PR - Better comments for failing batch (the current one get trimmed when multiple errors)

but for v18

PR Checklist

DavidWeiss2 commented 11 months ago

Saw the conflicts, fixing them.

DavidWeiss2 commented 11 months ago

@jasonbosco Conflict resolved

tharropoulos commented 2 months ago

Looks like tests are failing

In https://github.com/typesense/firestore-typesense-search/pull/70/commits/30105bcdb5ddd131fbd71e8f500907f1ba526306 , there was a change introduced on line 53, regarding the currentDocumentsBatch variable 

const currentDocumentsBatch = thisBatch.docs.map((doc) => utils.typesenseDocumentFromSnapshot(doc));

This implementation resulted in currentDocumentsBatch being an array of pending promises, as typesenseDocumentFromSnapshot is an asynchronous function (Although only because of the dynamic import of flat).

So, in https://github.com/typesense/firestore-typesense-search/pull/70/commits/b8d920728880ea3b6170c1445464dc2108ed088f, changing this line to 

const currentDocumentsBatch = await Promise.all(thisBatch.docs.map(async (doc) => {
  return await utils.typesenseDocumentFromSnapshot(doc);
}));

takes care of the issue, resolving to the corresponding document schema that the import endpoint expects.

tharropoulos commented 2 months ago

Rebased master into the branch, can now target master.

DavidWeiss2 commented 2 months ago

What needed to fix in this PR in order to merge it?

tharropoulos commented 2 months ago

What needed to fix in this PR in order to merge it?

Tests were failing, I've posted a comment above referencing what went wrong (returning a promise instead of unpacking it)

DavidWeiss2 commented 2 months ago

What needed to fix in this PR in order to merge it?

Tests were failing, I've posted a comment above referencing what went wrong (returning a promise instead of unpacking it)

So now this is just waiting for Jason approval?

jasonbosco commented 2 months ago

This is now available in v1.5.0-rc.1.

The installation link for this RC build is in the link above.