Update use of schema API for the pull command (#499)

* Update use of schema API for pull command

Author: ptpaterson

src/commands/schema/diff.mjs

@@ -12,7 +12,7 @@ import { reformatFSL } from "../../lib/schema.mjs";
 import { localSchemaOptions } from "./schema.mjs";
 
 /**
- * @returns string[]
+ * @returns {[string, string]} An tuple containing the source and target schema
  */
 function parseTarget(argv) {
   if (!argv.active && !argv.staged) {

src/commands/schema/pull.mjs

@@ -1,10 +1,7 @@
 //@ts-check
 
 import { container } from "../../cli.mjs";
-import {
-  CommandError,
-  yargsWithCommonQueryOptions,
-} from "../../lib/command-helpers.mjs";
+import { yargsWithCommonQueryOptions } from "../../lib/command-helpers.mjs";
 import { getSecret } from "../../lib/fauna-client.mjs";
 import { localSchemaOptions } from "./schema.mjs";
 
@@ -36,9 +33,9 @@ async function determineFileState(argv, filenames) {
   return { adds, deletes, existing, overwrites };
 }
 
-function logDiff({ argv, adds, overwrites, deletes }) {
+function logDiff({ argv, adds, overwrites, deletes, source }) {
   const logger = container.resolve("logger");
-  logger.stdout("Pull will make the following changes:");
+  logger.stdout(`Pulling ${source} schema will make the following changes:`);
   if (argv.delete) {
     for (const deleteme of deletes) {
       logger.stdout(`delete:    ${deleteme}`);
@@ -58,10 +55,29 @@ async function doPull(argv) {
   const makeFaunaRequest = container.resolve("makeFaunaRequest");
   const secret = await getSecret();
 
+  // Get the staged schema status
+  /** @type {{ status: "none" | "pending" | "ready" | "failed", version: string }} */
+  const statusResponse = await makeFaunaRequest({
+    argv,
+    path: "/schema/1/staged/status",
+    method: "GET",
+    secret,
+  });
+
+  const version = statusResponse.version;
+  const source =
+    argv.active || statusResponse.status === "none" ? "active" : "staged";
+
+  const filesParams = new URLSearchParams({
+    version,
+    staged: source === "staged" ? "true" : "false",
+  });
+
   // fetch the list of remote FSL files
   const filesResponse = await makeFaunaRequest({
     argv,
     path: "/schema/1/files",
+    params: filesParams,
     method: "GET",
     secret,
   });
@@ -72,31 +88,16 @@ async function doPull(argv) {
     .filter((name) => name.endsWith(".fsl"))
     .sort();
 
-  // check if there's a staged schema
-  const statusResponse = await makeFaunaRequest({
-    argv,
-    path: "/schema/1/staged/status",
-    params: new URLSearchParams({ version: filesResponse.version }),
-    method: "GET",
-    secret,
-  });
-
-  // if there's a staged schema, cannot use the --active flag.
-  // getting active FSL while staged FSL exists is not yet
-  // implemented at the service level.
-  if (statusResponse.status !== "none" && argv.active) {
-    throw new CommandError(
-      "There is a staged schema change. Remove the --active flag to pull it.",
-    );
-  } else if (statusResponse.status === "none" && !argv.active) {
-    throw new CommandError("There are no staged schema changes to pull.");
-  }
+  logger.debug(
+    `Pulling remote ${source} schema, version '${version}'.`,
+    "schema-pull",
+  );
 
   const { adds, deletes, overwrites } = await determineFileState(
     argv,
     filenames,
   );
-  logDiff({ argv, adds, deletes, overwrites });
+  logDiff({ argv, adds, deletes, overwrites, source });
 
   const confirmed = await confirm({
     message: "Accept the changes?",
@@ -108,7 +109,12 @@ async function doPull(argv) {
     const getAllSchemaFileContents = container.resolve(
       "getAllSchemaFileContents",
     );
-    const contents = await getAllSchemaFileContents(filenames, argv);
+    const contents = await getAllSchemaFileContents(
+      filenames,
+      source,
+      version,
+      argv,
+    );
 
     // don't start writing or deleting files until we've successfully fetched all
     // the remote schema files
@@ -138,8 +144,7 @@ function buildPullCommand(yargs) {
         default: false,
       },
       active: {
-        description:
-          "Pull the database's active schema files. If omitted, pulls the database's staged schema, if available.",
+        description: "Pull the database's active schema files.",
         type: "boolean",
         default: false,
       },

src/lib/schema.mjs

@@ -167,19 +167,33 @@ export async function writeSchemaFiles(dir, filenameToContentsDict) {
 
 /**
  * @param {string[]} filenames - A list of schema file names to fetch
+ * @param {"active" | "staged"} source - The source to pull from
+ * @param {string} version - The schema version for optimistic concurrency control
  * @param {object} argv
  * @returns {Promise<Record<string, string>>} A map of schema file names to their contents.
  */
-export async function getAllSchemaFileContents(filenames, argv) {
+export async function getAllSchemaFileContents(
+  filenames,
+  source,
+  version,
+  argv,
+) {
   const promises = [];
   /** @type Record<string, string> */
   const fileContentCollection = {};
   const secret = await getSecret();
+
+  const params = new URLSearchParams({
+    version: version,
+    staged: source === "staged" ? "true" : "false",
+  });
+
   for (const filename of filenames) {
     promises.push(
       makeFaunaRequest({
         argv,
         path: `/schema/1/files/${encodeURIComponent(filename)}`,
+        params,
         method: "GET",
         secret,
       }).then(({ content }) => {

test/schema/pull.mjs

@@ -3,7 +3,6 @@
 import * as awilix from "awilix";
 import { expect } from "chai";
 import sinon from "sinon";
-import tryToCatch from "try-to-catch";
 
 import { run } from "../../src/cli.mjs";
 import { setupTestContainer as setupContainer } from "../../src/config/setup-test-container.mjs";
@@ -39,7 +38,7 @@ describe("schema pull", function () {
     gatherFSL = container.resolve("gatherFSL");
   });
 
-  it("can pull schema, adding new files and overwriting existing files", async function () {
+  it("can pull schema, adding new files and overwriting existing files", async function () {
     gatherFSL.resolves([
       {
         name: "coll.fsl",
@@ -59,17 +58,18 @@ describe("schema pull", function () {
     fetch.onCall(0).resolves(
       f({
         version: "194838274939473",
-        files: [
-          { filename: "main.fsl" },
-          { filename: "second.fsl" },
-          { filename: "third.fsl" },
-        ],
+        // assume no staged schema for this test
+        status: "none",
       }),
     );
     fetch.onCall(1).resolves(
       f({
         version: "194838274939473",
-        status: "ready",
+        files: [
+          { filename: "main.fsl" },
+          { filename: "second.fsl" },
+          { filename: "third.fsl" },
+        ],
       }),
     );
     fetch.onCall(2).resolves(
@@ -98,35 +98,44 @@ describe("schema pull", function () {
     expect(gatherFSL).to.have.been.calledWith(".");
 
     expect(fetch).to.have.been.calledWith(
-      buildUrl("/schema/1/files"),
+      buildUrl("/schema/1/staged/status"),
       commonFetchParams,
     );
     // the version param in the URL is important - we use it for optimistic locking
     expect(fetch).to.have.been.calledWith(
-      buildUrl("/schema/1/staged/status", {
+      buildUrl("/schema/1/files", {
         version: "194838274939473",
-        color: "ansi",
+        staged: "false",
       }),
       commonFetchParams,
     );
     expect(fetch).to.have.been.calledWith(
-      buildUrl("/schema/1/files/main.fsl"),
+      buildUrl("/schema/1/files/main.fsl", {
+        version: "194838274939473",
+        staged: "false",
+      }),
       commonFetchParams,
     );
     expect(fetch).to.have.been.calledWith(
-      buildUrl("/schema/1/files/second.fsl"),
+      buildUrl("/schema/1/files/second.fsl", {
+        version: "194838274939473",
+        staged: "false",
+      }),
       commonFetchParams,
     );
     expect(fetch).to.have.been.calledWith(
-      buildUrl("/schema/1/files/third.fsl"),
+      buildUrl("/schema/1/files/third.fsl", {
+        version: "194838274939473",
+        staged: "false",
+      }),
       commonFetchParams,
     );
 
     expect(logger.stdout).to.have.been.calledWith("overwrite: main.fsl");
     expect(logger.stdout).to.have.been.calledWith("add:       second.fsl");
     expect(logger.stdout).to.have.been.calledWith("add:       third.fsl");
     expect(logger.stdout).to.have.been.calledWith(
-      "Pull will make the following changes:",
+      "Pulling active schema will make the following changes:",
     );
 
     expect(fs.mkdirSync).to.have.been.calledWith(".", { recursive: true });
@@ -168,17 +177,18 @@ describe("schema pull", function () {
     fetch.onCall(0).resolves(
       f({
         version: "194838274939473",
-        files: [
-          { filename: "main.fsl" },
-          { filename: "second.fsl" },
-          { filename: "third.fsl" },
-        ],
+        // assume no staged schema for this test
+        status: "none",
       }),
     );
     fetch.onCall(1).resolves(
       f({
         version: "194838274939473",
-        status: "ready",
+        files: [
+          { filename: "main.fsl" },
+          { filename: "second.fsl" },
+          { filename: "third.fsl" },
+        ],
       }),
     );
 
@@ -189,7 +199,7 @@ describe("schema pull", function () {
     expect(logger.stdout).to.have.been.calledWith("add:       third.fsl");
     expect(logger.stdout).to.have.been.calledWith("delete:    coll.fsl");
     expect(logger.stdout).to.have.been.calledWith(
-      "Pull will make the following changes:",
+      "Pulling active schema will make the following changes:",
     );
     expect(logger.stdout).to.have.been.calledWith("Change cancelled.");
     expect(fs.writeFile).to.have.not.been.called;
@@ -215,13 +225,14 @@ describe("schema pull", function () {
     fetch.onCall(0).resolves(
       f({
         version: "194838274939473",
-        files: [{ filename: "main.fsl" }],
+        // assume no staged schema for this test
+        status: "none",
       }),
     );
     fetch.onCall(1).resolves(
       f({
         version: "194838274939473",
-        status: "ready",
+        files: [{ filename: "main.fsl" }],
       }),
     );
     fetch.onCall(2).resolves(
@@ -241,7 +252,7 @@ describe("schema pull", function () {
     expect(logger.stdout).to.have.been.calledWith("overwrite: main.fsl");
     expect(logger.stdout).to.have.been.calledWith("delete:    coll.fsl");
     expect(logger.stdout).to.have.been.calledWith(
-      "Pull will make the following changes:",
+      "Pulling active schema will make the following changes:",
     );
 
     expect(fs.mkdirSync).to.have.been.calledWith(".", { recursive: true });
@@ -255,30 +266,60 @@ describe("schema pull", function () {
 
   it.skip("does not modify the filesystem if it fails to read file contents", async function () {});
 
-  it("errors if called with the --active flag while a schema change is staged", async function () {
-    fetch.onCall(0).resolves(
-      f({
-        version: "194838274939473",
-        files: [
-          { filename: "main.fsl" },
-          { filename: "second.fsl" },
-          { filename: "third.fsl" },
-        ],
-      }),
-    );
-    fetch.onCall(1).resolves(
-      f({
-        version: "194838274939473",
-        status: "ready",
-      }),
-    );
+  [
+    { argvActive: true, remoteStaged: true, expectedStagedParam: "false" },
+    { argvActive: false, remoteStaged: false, expectedStagedParam: "false" },
+    { argvActive: false, remoteStaged: true, expectedStagedParam: "true" },
+  ].forEach(({ argvActive, remoteStaged, expectedStagedParam }) => {
+    it(`can pull ${expectedStagedParam === "true" ? "staged" : "active"} schema while schema is ${remoteStaged ? "staged" : "not staged"}, and --active is ${argvActive ? "specified" : "not specified"}`, async function () {
+      fetch.onCall(0).resolves(
+        f({
+          version: "194838274939473",
+          status: remoteStaged ? "ready" : "none",
+        }),
+      );
+      fetch.onCall(1).resolves(
+        f({
+          version: "194838274939473",
+          files: [{ filename: "main.fsl" }],
+        }),
+      );
+      fetch.onCall(2).resolves(
+        f({
+          content:
+            "collection Main {\\n  name: String\\n  index byName {\\n    terms [.name]\\n  }\\n}\\n",
+        }),
+      );
 
-    const [error] = await tryToCatch(() =>
-      run(`schema pull --secret "secret" --active`, container),
-    );
-    expect(error).to.have.property("code", 1);
-    expect(container.resolve("gatherFSL")).to.not.have.been.called;
-  });
+      // user accepts the changes in the interactive prompt
+      confirm.resolves(true);
+
+      await run(
+        `schema pull --secret "secret" ${argvActive ? "--active" : ""}`,
+        container,
+      );
 
-  it.skip("errors if there are no staged schema changes to pull");
+      expect(fetch).to.have.been.calledWith(
+        buildUrl("/schema/1/staged/status"),
+        commonFetchParams,
+      );
+      // the version param in the URL is important - we use it for optimistic locking
+      expect(fetch).to.have.been.calledWith(
+        buildUrl("/schema/1/files", {
+          version: "194838274939473",
+          staged: expectedStagedParam,
+        }),
+        commonFetchParams,
+      );
+      expect(fetch).to.have.been.calledWith(
+        buildUrl("/schema/1/files/main.fsl", {
+          version: "194838274939473",
+          staged: expectedStagedParam,
+        }),
+        commonFetchParams,
+      );
+
+      expect(logger.stderr).to.not.have.been.called;
+    });
+  });
 });