fix(bigquery-firestore-export): restore install-time param validation - #2982
Draft
IzaakGough wants to merge 2 commits into
Draft
fix(bigquery-firestore-export): restore install-time param validation#2982IzaakGough wants to merge 2 commits into
IzaakGough wants to merge 2 commits into
Conversation
COLLECTION_PATH regains the upstream collection path regex, and BIGQUERY_DATASET_LOCATION becomes a select over the upstream list of regions and multi-regions. Both were free-form strings after the port, so a bad value was accepted at deploy and only failed at runtime.
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces validation and predefined options for configuration parameters in the BigQuery Firestore Export kit. Specifically, it adds a comprehensive list of supported BigQuery dataset locations to restrict the BIGQUERY_DATASET_LOCATION parameter and a regular expression to validate that the COLLECTION_PATH parameter is a valid Firestore collection path. It also exports the params object and adds comprehensive unit tests to verify the validation logic and parameter options. There are no review comments, so no further feedback is provided.
Both sides restored the COLLECTION_PATH validator. Keep the inline RegExp literal used by every other kit, plus the example added here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was broken
Two params lost their validation in the port to the kit.
COLLECTION_PATHbecame a plain string, so an even-segment path such asa/bis accepted at deploy and then fails inside Firestore at runtime.BIGQUERY_DATASET_LOCATIONbecame free-form, so a typo means the query job location does not match the dataset and the scheduled query fails.What changed
COLLECTION_PATHcarries the upstream regex^[^/]+(/[^/]+/[^/]+)*$and its error message.BIGQUERY_DATASET_LOCATIONis aselectover the same 35 options as upstream (33 regions plus the US and EU multi-regions), following the existingLOG_LEVELpattern. Resolved config is unchanged for valid input.paramsis now exported so tests can assert the declared inputs.Verification
Tests and build run in a worktree holding only this commit on top of
kits: 36 tests pass,tsc -bexits 0. The regex is exercised againsta,a/b/c,a/b/c/d/e,a/b,a/b/c/d,/a,a/and the empty string.Notes
Validation only fires on the interactive prompt. Driving firebase-tools 15.28.1's own
resolveParams, a value supplied from a.envfile is taken verbatim, with no regex check and no select-membership check, while the prompt path retries with the error message. So this catches typos during an interactive deploy, not in CI.The extension behaves the same way, so this is not a gap the kit opened.
validateCommandLineParamsstill exists in firebase-tools but has had no caller in the product path sincee16fe0a80(March 2023), and the extensions deploy planner reads.envparams without validating them. That was checked in the CLI only; whether the backend rejects a bad value at instance-create time is untested.One deviation from upstream: the label
EU Mutli-Region (EU)is spelledEU Multi-Region (EU)here.