Action builders and path utils fixes (dataform-co#1606)

* Tidy up path separator

* Tidy action target constructors

* Minor comment nit

* Lint fix

* Fix empty schema and database

* Move path to a separate file

* fix import order

core/BUILD

@@ -30,6 +30,7 @@ ts_library(
         "compilers.ts",
         "index.ts",
         "main.ts",
+        "path.ts",
         "session.ts",
         "targets.ts",
         "utils.ts",

core/actions/assertion.ts

@@ -165,10 +165,16 @@ export class Assertion implements IActionBuilder<dataform.Assertion> {
     return this;
   }
 
+  /**
+   * @hidden
+   */
   public getFileName() {
     return this.proto.fileName;
   }
 
+  /**
+   * @hidden
+   */
   public getTarget() {
     return dataform.Target.create(this.proto.target);
   }

core/actions/declaration.ts

@@ -69,10 +69,16 @@ export class Declaration implements IActionBuilder<dataform.Declaration> {
     return this;
   }
 
+  /**
+   * @hidden
+   */
   public getFileName() {
     return this.proto.fileName;
   }
 
+  /**
+   * @hidden
+   */
   public getTarget() {
     return dataform.Target.create(this.proto.target);
   }

core/actions/notebook.ts

@@ -15,16 +15,35 @@ export class Notebook implements IActionBuilder<dataform.Notebook> {
   constructor(session: Session, config: dataform.ActionConfig) {
     this.session = session;
     this.proto.config = config;
+
+    // TODO(ekrekr): move this to an Action Builder utility method, once configs are on all protos.
+    const canonicalTarget = this.proto.config.target;
+    this.proto.config.target = dataform.Target.create({
+      name: canonicalTarget.name,
+      schema: canonicalTarget.schema || session.canonicalConfig.defaultSchema || undefined,
+      database: canonicalTarget.database || session.canonicalConfig.defaultDatabase || undefined
+    });
+    this.proto.target = dataform.Target.create({
+      name: canonicalTarget.name,
+      schema: canonicalTarget.schema || session.config.defaultSchema || undefined,
+      database: canonicalTarget.database || session.config.defaultDatabase || undefined
+    });
   }
 
-  public setNotebookContents(contents: string) {
-    this.proto.notebookContents = contents;
+  public notebookContents(notebookContents: string) {
+    this.proto.notebookContents = notebookContents;
   }
 
+  /**
+   * @hidden
+   */
   public getFileName() {
     return this.proto.config.fileName;
   }
 
+  /**
+   * @hidden
+   */
   public getTarget() {
     return dataform.Target.create(this.proto.target);
   }

core/actions/operation.ts

@@ -188,10 +188,16 @@ export class Operation implements IActionBuilder<dataform.Operation> {
     return this;
   }
 
+  /**
+   * @hidden
+   */
   public getFileName() {
     return this.proto.fileName;
   }
 
+  /**
+   * @hidden
+   */
   public getTarget() {
     return dataform.Target.create(this.proto.target);
   }

core/actions/table.ts

@@ -512,10 +512,16 @@ export class Table implements IActionBuilder<dataform.Table> {
     return this;
   }
 
+  /**
+   * @hidden
+   */
   public getFileName() {
     return this.proto.fileName;
   }
 
+  /**
+   * @hidden
+   */
   public getTarget() {
     return dataform.Target.create(this.proto.target);
   }

core/actions/test.ts

@@ -72,10 +72,16 @@ export class Test implements IActionBuilder<dataform.Test> {
     return this;
   }
 
+  /**
+   * @hidden
+   */
   public getFileName() {
     return this.proto.fileName;
   }
 
+  /**
+   * @hidden
+   */
   public getTarget(): undefined {
     // The test action type has no target because it is not processed during regular execution.
     return undefined;

core/compilers.ts

@@ -1,6 +1,6 @@
 import { load as loadYaml, YAMLException } from "js-yaml";
 
-import * as utils from "df/core/utils";
+import * as Path from "df/core/path";
 import { SyntaxTreeNode, SyntaxTreeNodeType } from "df/sqlx/lexer";
 
 export function compile(code: string, path: string): string {
@@ -70,7 +70,7 @@ function compileSqlx(rootNode: SyntaxTreeNode, path: string): string {
 
   return `dataform.sqlxAction({
   sqlxConfig: {
-    name: "${utils.getEscapedFileName(path)}",
+    name: "${Path.escapedFileName(path)}",
     type: "operations",
     ...${config || "{}"}
   },

core/main.ts

@@ -1,4 +1,5 @@
 import { decode64, encode64, verifyObjectMatchesProto } from "df/common/protos";
+import * as Path from "df/core/path";
 import { Session } from "df/core/session";
 import * as utils from "df/core/utils";
 import { readWorkflowSettings } from "df/core/workflow_settings";
@@ -56,13 +57,13 @@ export function main(coreExecutionRequest: Uint8Array | string): Uint8Array | st
   // "includes" files from implicitly depending on other "includes" files.
   const topLevelIncludes: { [key: string]: any } = {};
   compileRequest.compileConfig.filePaths
-    .filter(path => path.startsWith(`includes${utils.pathSeperator}`))
-    .filter(path => path.split(utils.pathSeperator).length === 2) // Only include top-level "includes" files.
+    .filter(path => path.startsWith(`includes${Path.separator}`))
+    .filter(path => path.split(Path.separator).length === 2) // Only include top-level "includes" files.
     .filter(path => path.endsWith(".js"))
     .forEach(includePath => {
       try {
         // tslint:disable-next-line: tsr-detect-non-literal-require
-        topLevelIncludes[utils.baseFilename(includePath)] = nativeRequire(includePath);
+        topLevelIncludes[Path.fileName(includePath)] = nativeRequire(includePath);
       } catch (e) {
         session.compileError(e, includePath);
       }
@@ -80,7 +81,7 @@ export function main(coreExecutionRequest: Uint8Array | string): Uint8Array | st
 
   // Require all "definitions" files (attaching them to the session).
   compileRequest.compileConfig.filePaths
-    .filter(path => path.startsWith(`definitions${utils.pathSeperator}`))
+    .filter(path => path.startsWith(`definitions${Path.separator}`))
     .filter(path => path.endsWith(".js") || path.endsWith(".sqlx"))
     .sort()
     .forEach(definitionPath => {
@@ -108,7 +109,7 @@ export function main(coreExecutionRequest: Uint8Array | string): Uint8Array | st
 function loadActionConfigs(session: Session, filePaths: string[]) {
   filePaths
     .filter(
-      path => path.startsWith(`definitions${utils.pathSeperator}`) && path.endsWith("actions.yaml")
+      path => path.startsWith(`definitions${Path.separator}`) && path.endsWith("/actions.yaml")
     )
     .sort()
     .forEach(actionConfigsPath => {
@@ -124,34 +125,22 @@ function loadActionConfigs(session: Session, filePaths: string[]) {
       const actionConfigs = dataform.ActionConfigs.fromObject(actionConfigsAsJson);
 
       actionConfigs.actions.forEach(actionConfig => {
-        const { fileExtension, fileNameAsTargetName } = extractActionDetailsFromFileName(
+        const { fileExtension, fileNameAsTargetName } = utils.extractActionDetailsFromFileName(
           actionConfig.fileName
         );
-        if (!actionConfig.target?.name) {
-          if (!actionConfig.target) {
-            actionConfig.target = {};
-          }
+        if (!actionConfig.target) {
+          actionConfig.target = {};
+        }
+        if (!actionConfig.target.name) {
           actionConfig.target.name = fileNameAsTargetName;
         }
 
         if (fileExtension === "ipynb") {
           // TODO(ekrekr): throw an error if any non-notebook configs are given.
           // TODO(ekrekr): add test for nice errors if file not found.
           const notebookContents = nativeRequire(actionConfig.fileName).asBase64String();
-          session.notebookAction(dataform.ActionConfig.create(actionConfig), notebookContents);
+          session.notebook(dataform.ActionConfig.create(actionConfig), notebookContents);
         }
       });
     });
 }
-
-function extractActionDetailsFromFileName(
-  path: string
-): { fileExtension: string; fileNameAsTargetName: string } {
-  // TODO(ekrekr): make filename in actions.yaml files not need the `definitions` prefix. In
-  // addition, actions.yaml in nested directories should prefix file imports with their path.
-  const fileName = path.split("/").slice(-1)[0];
-  const fileExtension = fileName.split(".").slice(-1)[0];
-  // TODO(ekrekr): validate and test weird characters in filenames.
-  const fileNameAsTargetName = fileName.slice(0, fileExtension.length - 1);
-  return { fileExtension, fileNameAsTargetName };
-}

core/path.ts

@@ -0,0 +1,33 @@
+export const separator = (() => {
+  if (typeof process !== "undefined") {
+    return process.platform === "win32" ? "\\" : "/";
+  }
+  return "/";
+})();
+
+export function relativePath(fullPath: string, base: string) {
+  if (base.length === 0) {
+    return fullPath;
+  }
+  const stripped = fullPath.substr(base.length);
+  if (stripped.startsWith(separator)) {
+    return stripped.substr(1);
+  } else {
+    return stripped;
+  }
+}
+
+export function fileName(fullPath: string) {
+  return fullPath
+    .split(separator)
+    .slice(-1)[0]
+    .split(".")[0];
+}
+
+export function escapedFileName(path: string) {
+  return fileName(path).replace(/\\/g, "\\\\");
+}
+
+export function fileExtension(fullPath: string) {
+  return fullPath.split(".").slice(-1)[0];
+}

core/session.ts

@@ -195,10 +195,9 @@ export class Session {
     }
   }
 
-  public notebookAction(notebookConfig: dataform.ActionConfig, notebookContents: string): Notebook {
+  public notebook(notebookConfig: dataform.ActionConfig, notebookContents: string): Notebook {
     const notebook = new Notebook(this, notebookConfig);
-    utils.setNameAndTarget(this, notebook.proto as IActionProto, notebookConfig.target.name);
-    notebook.setNotebookContents(notebookContents);
+    notebook.notebookContents(notebookContents);
     this.actions.push(notebook);
     return notebook;
   }

core/utils.ts

@@ -3,39 +3,10 @@ import { Assertion } from "df/core/actions/assertion";
 import { Operation } from "df/core/actions/operation";
 import { Table } from "df/core/actions/table";
 import { Resolvable } from "df/core/common";
+import * as Path from "df/core/path";
 import { IActionProto, Session } from "df/core/session";
 import { dataform } from "df/protos/ts";
 
-export const pathSeperator = (() => {
-  if (typeof process !== "undefined") {
-    return process.platform === "win32" ? "\\" : "/";
-  }
-  return "/";
-})();
-
-function relativePath(fullPath: string, base: string) {
-  if (base.length === 0) {
-    return fullPath;
-  }
-  const stripped = fullPath.substr(base.length);
-  if (stripped.startsWith(pathSeperator)) {
-    return stripped.substr(1);
-  } else {
-    return stripped;
-  }
-}
-
-export function baseFilename(fullPath: string) {
-  return fullPath
-    .split(pathSeperator)
-    .slice(-1)[0]
-    .split(".")[0];
-}
-
-export function getEscapedFileName(path: string) {
-  return baseFilename(path).replace(/\\/g, "\\\\");
-}
-
 export function matchPatterns(patterns: string[], values: string[]) {
   const fullyQualifiedActions: string[] = [];
   patterns.forEach(pattern => {
@@ -76,8 +47,8 @@ export function getCallerFile(rootDir: string) {
     lastfile = nextLastfile;
     if (
       !(
-        nextLastfile.includes(`definitions${pathSeperator}`) ||
-        nextLastfile.includes(`models${pathSeperator}`)
+        nextLastfile.includes(`definitions${Path.separator}`) ||
+        nextLastfile.includes(`models${Path.separator}`)
       )
     ) {
       continue;
@@ -89,7 +60,7 @@ export function getCallerFile(rootDir: string) {
     // If so, explicitly pass the filename to Session.compileError().
     throw new Error("Unable to find valid caller file; please report this issue.");
   }
-  return relativePath(lastfile, rootDir);
+  return Path.relativePath(lastfile, rootDir);
 }
 
 function getCurrentStack(): NodeJS.CallSite[] {
@@ -169,12 +140,10 @@ export function target(
   schema?: string,
   database?: string
 ): dataform.ITarget {
-  schema = schema || config.defaultSchema;
-  database = database || config.defaultDatabase;
   return dataform.Target.create({
     name,
-    schema: !!schema ? schema || config.defaultSchema : undefined,
-    database: !!database ? database : undefined
+    schema: schema || config.defaultSchema || undefined,
+    database: database || config.defaultDatabase || undefined
   });
 }
 
@@ -186,10 +155,6 @@ export function setNameAndTarget(
   overrideDatabase?: string
 ) {
   action.target = target(session.config, name, overrideSchema, overrideDatabase);
-  if (action.config) {
-    action.config.target = target(session.canonicalConfig, name, overrideSchema, overrideDatabase);
-    return;
-  }
   action.canonicalTarget = target(session.canonicalConfig, name, overrideSchema, overrideDatabase);
 }
 
@@ -275,3 +240,15 @@ export function setOrValidateTableEnumType(table: dataform.ITable) {
     );
   }
 }
+
+export function extractActionDetailsFromFileName(
+  path: string
+): { fileExtension: string; fileNameAsTargetName: string } {
+  // TODO(ekrekr): make filename in actions.yaml files not need the `definitions` prefix. In
+  // addition, actions.yaml in nested directories should prefix file imports with their path.
+  const fileName = Path.fileName(path);
+  const fileExtension = Path.fileExtension(path);
+  // TODO(ekrekr): validate and test weird characters in filenames.
+  const fileNameAsTargetName = fileName.slice(0, fileExtension.length - 1);
+  return { fileExtension, fileNameAsTargetName };
+}