search

firebase / extensions

Source code for official Firebase extensions
https://firebase.google.com/products/extensions
Apache License 2.0
882 stars 373 forks source link

Fix BigQuery Table creation with partitioning enabled #2028

Closed alexregier closed 2 months ago

alexregier commented 3 months ago

Fixes an issue when installing the extension with partition enabled on BigQuery.

When the extension gets initialized for the first time, the BigQuery table does not yet exist and needs to be created. However, if partition is configured it breaks in the current implementation:

  1. https://github.com/firebase/extensions/blob/ea8fa836a9d98754c4705a61733a8d1e1aa3a132/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/index.ts#L401
  2. The function attempts to check if partition is already set up for the table: https://github.com/firebase/extensions/blob/ea8fa836a9d98754c4705a61733a8d1e1aa3a132/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/partitioning.ts#L253
  3. The check attempts to read metadata for the table https://github.com/firebase/extensions/blob/ea8fa836a9d98754c4705a61733a8d1e1aa3a132/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/partitioning.ts#L139
  4. However, if the table does not yet exist, this will crash, causing the whole cloud function to timeout because the issue is never caught.

This issue was introduced in version 0.1.45 by this PR: https://github.com/firebase/extensions/pull/1923 The intention of that PR was to fix a bug where exists() was called synchronous, although it returns a Promise, thus will always evaluate as being truthy back then: https://github.com/firebase/extensions/blob/41d9a3b5e46487d725c6bdd8c2f6ad1f0d438d7a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/partitioning.ts#L132-L137

However, that PR also removes the actual handling in case the table does not exist, causing this path not to be checked at all: https://github.com/firebase/extensions/pull/1923/commits/d7c329fac67995cf3093f78ffc74d675b31ed53a#diff-0dc03457cedb2c2f90cf2d8b9673e91443a0fb93ff0b43613f9d8e1f56285a30L143-L145

My PR brings back this check.

This issue does not occur if the table already exists, but on initial setup (while partition is already configured), it breaks.

alexregier commented 3 months ago

I also tried to add a test for this case... But given how the tests are set up right now, it's really complicated to test this specific case: Setting up partitioning should still work even if the table does not exist yet, so the expected behavior is the same to the existing tests: https://github.com/firebase/extensions/blob/ea8fa836a9d98754c4705a61733a8d1e1aa3a132/firestore-bigquery-export/firestore-bigquery-change-tracker/src/__tests__/bigquery/partitioning.test.ts#L42-L56

So I don't really know how to write a test for this case to make sure this does not break again...

cabljac commented 2 months ago

Apologies, I was given this issue this week and did not see your fix here. I've merged a PR to fix this (and a couple of other bugs i saw in the code)

Thanks for your contribution, and sorry again that I missed this PR.

I will close this PR as the issue should be resolved in the release today.