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 composite index testing build bug #7682

Merged
merged 12 commits into from
Oct 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test-changed-firestore-integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:
if: github.event_name == 'pull_request'
run: |
cd packages/firestore
terraform apply -var-file=../../config/project.json -auto-approve
terraform apply -var-file=../../config/project.json -auto-approve &> /dev/null
continue-on-error: true
- name: Set up Node (16)
uses: actions/setup-node@v3
Expand Down
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,4 @@ toc/
.terraform/*
.terraform.lock.hcl
*.tfstate
*.tfstate.*
*.tfstate.*
1 change: 1 addition & 0 deletions integration/firestore/gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ function copyTests() {
.src(
[
testBase + '/integration/api/*.ts',
testBase + '/integration/util/composite_index_test_helper.ts',
testBase + '/integration/util/events_accumulator.ts',
testBase + '/integration/util/helpers.ts',
testBase + '/integration/util/settings.ts',
Expand Down
1 change: 1 addition & 0 deletions packages/firestore/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ export { FieldPath as _FieldPath } from './model/path';
export type { ResourcePath as _ResourcePath } from './model/path';
export { ByteString as _ByteString } from './util/byte_string';
export { logWarn as _logWarn } from './util/log';
export { AutoId as _AutoId } from './util/misc';
export type {
AuthTokenFactory,
FirstPartyCredentialsSettings
Expand Down
6 changes: 6 additions & 0 deletions packages/firestore/src/util/misc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ export interface Indexable {
[k: string]: unknown;
}

/**
* A utility class for generating unique alphanumeric IDs of a specified length.
*
* @internal
* Exported internally for testing purposes.
*/
export class AutoId {
static newId(): string {
// Alphanumeric characters
Expand Down
18 changes: 18 additions & 0 deletions packages/firestore/test/integration/api/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,21 @@ put in test/integration/api_internal instead.
The line "import * as firebaseExport from '../util/firebase_export';" is
replaced via the gulpfile in 'integration/firestore' and should not be
modified.


## Testing composite index query against production

### Setting Up the Environment:
1. Create a `project.json` file in the `firebase-js-sdk/config` directory. This file should contain your target Firebase project's configuration.
2. If not already logged in, authenticate with your Google Cloud Platform (GCP) account using `gcloud auth application-default login`. You can check your logged-in accounts by running `gcloud auth list`.
3. Navigate to the `firebase-js-sdk/packages/firestore` directory, run:
```
terraform init
terraform apply -var-file=../../config/project.json -auto-approve
```
Note: If the index creation encounters issues, such as concurrent operations, consider running the index creation process again. Error messages indicating that indexes have already been created can be safely disregarded.


### Adding new composite index query tests
1. To create a new composite index for local development, click on the provided link in the test error message, which will direct you to the Firebase Console.
2. Add the newly created composite index to the `firestore_index_config.tf` file. The "__name__" field is not required to be explicitly added to the file, as the index creation will auto complete it on behalf.
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,10 @@ import { apiDescribe } from '../util/helpers';
* and setting test documents and running queries with ease, ensuring proper data
* isolation and query construction.
*
* Please remember to update the main index configuration file (firestore_index_config.tf)
* with any new composite indexes needed for the tests. This ensures synchronization with
* other testing environments, including CI. You can generate the required index link by
* clicking on the Firebase console link in the error message while running tests locally.
* To get started, please refer to the instructions provided in the README file. This will guide you
* through setting up your local testing environment and updating the Terraform configuration with
* any new composite indexes required for your testing scenarios.
*/

apiDescribe('Composite Index Queries', persistence => {
// OR Query tests only run when the SDK's local cache is configured to use
// LRU garbage collection (rather than eager garbage collection) because
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@
* limitations under the License.
*/

import { and } from '../../../src/lite-api/query';
import { AutoId } from '../../../src/util/misc';
import { field } from '../../util/helpers';

import {
query as internalQuery,
CollectionReference,
Expand All @@ -41,7 +37,10 @@ import {
getDocs as getDocuments,
QuerySnapshot,
deleteDoc as deleteDocument,
doc
doc,
and,
_AutoId,
_FieldPath
} from './firebase_export';
import {
batchCommitDocsToCollection,
Expand Down Expand Up @@ -72,7 +71,7 @@ export class CompositeIndexTestHelper {
// Creates a new instance of the CompositeIndexTestHelper class, with a unique test
// identifier for data isolation.
constructor() {
this.testId = 'test-id-' + AutoId.newId();
this.testId = 'test-id-' + _AutoId.newId();
}

// Runs a test with specified documents in the COMPOSITE_INDEX_TEST_COLLECTION.
Expand Down Expand Up @@ -113,8 +112,8 @@ export class CompositeIndexTestHelper {

// Remove test-specific fields from a document, including the testId and expiration date.
private removeTestSpecificFieldsFromDoc(doc: DocumentData): void {
doc._document?.data?.delete(field(this.TTL_FIELD));
doc._document?.data?.delete(field(this.TEST_ID_FIELD));
doc._document?.data?.delete(new _FieldPath([this.TEST_ID_FIELD]));
doc._document?.data?.delete(new _FieldPath([this.TEST_ID_FIELD]));
}

// Helper method to hash document keys and add test-specific fields for the provided documents.
Expand Down
7 changes: 3 additions & 4 deletions packages/firestore/test/integration/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
import { isIndexedDBAvailable } from '@firebase/util';
import { expect } from 'chai';

import { AutoId } from '../../../src/util/misc';

import {
clearIndexedDbPersistence,
collection,
Expand All @@ -45,7 +43,8 @@ import {
writeBatch,
Query,
getDocsFromServer,
getDocsFromCache
getDocsFromCache,
_AutoId
} from './firebase_export';
import {
ALT_PROJECT_ID,
Expand Down Expand Up @@ -448,7 +447,7 @@ export function withTestCollectionSettings<T>(
docs: { [key: string]: DocumentData },
fn: (collection: CollectionReference, db: Firestore) => Promise<T>
): Promise<T> {
const collectionId = AutoId.newId();
const collectionId = _AutoId.newId();
return batchCommitDocsToCollection(
persistence,
settings,
Expand Down
Loading