Running migrations with '--dryrun' modifies the db sometimes.

[Tittoh]

I noticed an inconsistency when running migration scripts with the --dryrun option. I was testing out my code to copy data from one collection to a new one. Whenever a change to the script was made, I ran the migration script which worked as expected on most occasions but made changes to the db sometimes. The new collection is created and the supplied data is added.

The expected behaviour is that the new collection should not be created when using --dryrun.

Command ran: fireway migrate --dryrun Fireway fireway@1.0.2 Node: v14.16.0 OS: macOS Catalina

[kevlened]

Hmm. Are you sure you're only using the firestore instance supplied by fireway? That's the only instance with --dryrun guarantees.

Expected to work every time

// /project/migrations/v0.0.0__change.js
module.exports.migrate = async ({firestore, FieldValue}) => {
    await firestore.collection('name').add({key: FieldValue.serverTimestamp()});
};

Could change things

// /project/migrations/v0.0.0__change.js
const util = require('../util');

module.exports.migrate = async () => {
    await util.change();
};

// /project/util.js
const { admin } = require('firebase-admin'); // this instance isn't guaranteed to be patched
const { firestore } = admin;
const { FieldValue } = firestore;

module.exports.change = async () => {
    await firestore.collection('name').add({key: FieldValue.serverTimestamp()});
};

[kevlened]

Heads up, if that is the problem, there may be a solution that could improve compatibility. Just lmk

[Tittoh]

The setup is pretty much the same. I have been testing this again and the data is written to the DB without the fireway collection.

[kiptoomm]

👀

[kiptoomm]

Heads up, if that is the problem, there may be a solution that could improve compatibility. Just lmk

hey @kevlened, we have adapted our scripts to use the fireway-provided firestore instance like you suggested, but still seeing this problem. what is the compatibility solution you had in mind?

[42ae]

Heads up, if that is the problem, there may be a solution that could improve compatibility. Just lmk

hey @kevlened, we have adapted our scripts to use the fireway-provided firestore instance like you suggested, but still seeing this problem. what is the compatibility solution you had in mind?

Hi @kiptoomm I was facing the same issue today. Make sure you don't leave any open async calls when running your migration, that was the problem for me.

[josheverett]

@42ae would you mind sharing a snippet showing the async calls you left open? Want to see what to avoid.

[42ae]

@josheverett Given the following example, not returning Promise.all to the migrate function on the last line would leave async calls opened.

import { MigrateOptions } from "fireway";
import { size } from "lodash";
import { listAllEmployees } from "../helpers/employees";
import { listAllOrganizations } from "../helpers/organizations";

export async function migrate({ firestore }: MigrateOptions) {
  const organizations = await listAllOrganizations(firestore);

  const fetchingEmployees = organizations.docs.map((organization) => {
    return listAllEmployees(organization);
  });

  const organizationsEmployees = await Promise.all(fetchingEmployees);
  const fetchingPrivateData: Promise<
    FirebaseFirestore.DocumentSnapshot<FirebaseFirestore.DocumentData>
  >[] = [];

  for (const employees of organizationsEmployees) {
    for (const employee of employees.docs) {
      fetchingPrivateData.push(
        employee.ref.collection("private").doc("data").get()
      );
    }
  }

  const employeesPrivateData = await Promise.all(fetchingPrivateData);
  const updatingCounters: Promise<FirebaseFirestore.WriteResult>[] = [];

  for (const employeePrivateData of employeesPrivateData) {
    const employeeRef = employeePrivateData.ref.parent.parent;

    const campaignsCount = employeePrivateData.exists
      ? size(employeePrivateData.get("campaigns"))
      : 0;
    const updatingCounter = employeeRef.update(
      "count.campaigns",
      campaignsCount
    );

    updatingCounters.push(updatingCounter);
  }

  return Promise.all(updatingCounters);
}

Hope this helps! Feel free to share your code if you need some insights.

[josheverett]

@42ae tried to reproduce your case but couldn't. Here is a test: https://github.com/josheverett/fireway/commit/46bfacbf79969b4e83e90f08a1690f01188f23f7

Forcing the test to wait before the deepEqual check didn't change the results. fireway does correctly detect the unhandled Promise.all(), FWIW.