ref(core): Refactor options validation (#104)

Refactor the validation logic of the plugin options. Since Sentry CLI does most of the validation work out of the box, we only need to take care of a few special cases. Therefore:

* Delete the ugly and duplicated org, project, auth, etc. checks in the release pipeline steps as most of them aren't necessary at all in that form
* Add a centralized validation function that validates certain combinations of options that Sentry CLI doesn't do (these validations are also present in the webpack plugin, albeit in a little different form)
* Add special validation of the `release` value as it simply has to be present to continue with release injection and the release creation pipeline
* All validation errors will cause the plugin to throw unless the user specified an error handler. Happy to discuss this point but I think it makes sense to only continue with valid options.

Author: Lms24

packages/bundler-plugin-core/src/index.ts

@@ -12,9 +12,10 @@ import {
 import "@sentry/tracing";
 import { addSpanToTransaction, captureMinimalError, makeSentryClient } from "./sentry/telemetry";
 import { Span, Transaction } from "@sentry/types";
-import { createLogger } from "./sentry/logger";
-import { normalizeUserOptions } from "./options-mapping";
+import { createLogger, Logger } from "./sentry/logger";
+import { InternalOptions, normalizeUserOptions, validateOptions } from "./options-mapping";
 import { getSentryCli } from "./sentry/cli";
+import { Hub } from "@sentry/node";
 
 // We prefix the polyfill id with \0 to tell other plugins not to try to load or transform it.
 // This hack is taken straight from https://rollupjs.org/guide/en/#resolveid.
@@ -78,8 +79,7 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
 
   const { hub: sentryHub } = makeSentryClient(
     "https://[email protected]/6690737",
-    internalOptions.telemetry,
-    internalOptions.org
+    internalOptions.telemetry
   );
 
   const logger = createLogger({
@@ -89,6 +89,15 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
     debug: internalOptions.debug,
   });
 
+  if (!validateOptions(internalOptions, logger)) {
+    handleError(
+      new Error("Options were not set correctly. See output above for more details."),
+      logger,
+      internalOptions.errorHandler,
+      sentryHub
+    );
+  }
+
   const cli = getSentryCli(internalOptions, logger);
 
   if (internalOptions.telemetry) {
@@ -125,6 +134,18 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
         internalOptions.release = await cli.releases.proposeVersion();
       }
 
+      // At this point, we either have determined a release or we have to bail
+      if (!internalOptions.release) {
+        handleError(
+          new Error(
+            "Unable to determine a release name. Make sure to set the `release` option or use an environment that supports auto-detection https://docs.sentry.io/cli/releases/#creating-releases`"
+          ),
+          logger,
+          internalOptions.errorHandler,
+          sentryHub
+        );
+      }
+
       transaction = sentryHub.startTransaction({
         op: "function.plugin",
         name: "Sentry Bundler Plugin execution",
@@ -282,12 +303,6 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
         level: "info",
       });
 
-      //TODO:
-      //  1. validate options to see if we get a valid include property, release name, etc.
-      //  2. normalize the include property: Users can pass string | string [] | IncludeEntry[].
-      //     That's good for them but a hassle for us. Let's try to normalize this into one data type
-      //     (I vote IncludeEntry[]) and continue with that down the line
-
       const ctx: BuildContext = { hub: sentryHub, parentSpan: releasePipelineSpan, logger, cli };
 
       createNewRelease(internalOptions, ctx)
@@ -298,16 +313,8 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
         .then(() => addDeploy(internalOptions, ctx))
         .then(() => transaction?.setStatus("ok"))
         .catch((e: Error) => {
-          captureMinimalError(e, sentryHub);
           transaction?.setStatus("cancelled");
-
-          logger.error(e.message);
-
-          if (internalOptions.errorHandler) {
-            internalOptions.errorHandler(e);
-          } else {
-            throw e;
-          }
+          handleError(e, logger, internalOptions.errorHandler, sentryHub);
         })
         .finally(() => {
           sentryHub.addBreadcrumb({
@@ -321,6 +328,25 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
   };
 });
 
+function handleError(
+  error: Error,
+  logger: Logger,
+  errorHandler: InternalOptions["errorHandler"],
+  sentryHub?: Hub
+) {
+  logger.error(error.message);
+
+  if (sentryHub) {
+    captureMinimalError(error, sentryHub);
+  }
+
+  if (errorHandler) {
+    errorHandler(error);
+  } else {
+    throw error;
+  }
+}
+
 /**
  * Generates code for the "sentry-release-injector" which is responsible for setting the global `SENTRY_RELEASE`
  * variable.

packages/bundler-plugin-core/src/options-mapping.ts

@@ -1,3 +1,4 @@
+import { Logger } from "./sentry/logger";
 import { IncludeEntry as UserIncludeEntry, Options as UserOptions } from "./types";
 import { arrayify } from "./utils";
 
@@ -175,3 +176,53 @@ function normalizeIncludeEntry(
     validate: includeEntry.validate ?? userOptions.validate ?? false,
   };
 }
+
+/**
+ * Validates a few combinations of options that are not checked by Sentry CLI.
+ *
+ * For all other options, we can rely on Sentry CLI to validate them. In fact,
+ * we can't validate them in the plugin because Sentry CLI might pick up options from
+ * its config file.
+ *
+ * @param options the internal options
+ * @param logger the logger
+ *
+ * @returns `true` if the options are valid, `false` otherwise
+ */
+export function validateOptions(options: InternalOptions, logger: Logger): boolean {
+  if (options.injectReleasesMap && !options.org) {
+    logger.error(
+      "The `injectReleasesMap` option was set but it is only supported when the `org` option is also specified.",
+      "Please set the `org` option (you can also set the SENTRY_ORG environment variable) or disable the `injectReleasesMap` option."
+    );
+    return false;
+  }
+
+  const setCommits = options.setCommits;
+  if (setCommits) {
+    if (!setCommits.auto && !(setCommits.repo && setCommits.commit)) {
+      logger.error(
+        "The `setCommits` option was specified but is missing required properties.",
+        "Please set either `auto` or both, `repo` and `commit`."
+      );
+      return false;
+    }
+    if (setCommits.auto && setCommits.repo && setCommits) {
+      logger.warn(
+        "The `setCommits` options includes `auto` but also `repo` and `commit`.",
+        "Ignoring `repo` and `commit`.",
+        "Please only set either `auto` or both, `repo` and `commit`."
+      );
+    }
+  }
+
+  if (options.deploy && !options.deploy.env) {
+    logger.error(
+      "The `deploy` option was specified but is missing the required `env` property.",
+      "Please set the `env` property."
+    );
+    return false;
+  }
+
+  return true;
+}

packages/bundler-plugin-core/src/sentry/releasePipeline.ts

@@ -13,48 +13,21 @@ import { addSpanToTransaction } from "./telemetry";
 export async function createNewRelease(options: InternalOptions, ctx: BuildContext): Promise<void> {
   const span = addSpanToTransaction(ctx, "function.plugin.create_release");
 
-  // TODO: pull these checks out of here and simplify them
-  if (options.authToken === undefined) {
-    ctx.logger.warn('Missing "authToken" option. Will not create release.');
-    return;
-  } else if (options.org === undefined) {
-    ctx.logger.warn('Missing "org" option. Will not create release.');
-    return;
-  } else if (options.project === undefined) {
-    ctx.logger.warn('Missing "project" option. Will not create release.');
-    return;
-  }
-
   await ctx.cli.releases.new(options.release);
 
   ctx.logger.info("Successfully created release.");
-
   span?.finish();
 }
 
 export async function uploadSourceMaps(options: InternalOptions, ctx: BuildContext): Promise<void> {
   const span = addSpanToTransaction(ctx, "function.plugin.upload_sourcemaps");
-
-  // TODO: pull these checks out of here and simplify them
-  if (options.authToken === undefined) {
-    ctx.logger.warn('Missing "authToken" option. Will not create release.');
-    return Promise.resolve();
-  } else if (options.org === undefined) {
-    ctx.logger.warn('Missing "org" option. Will not create release.');
-    return Promise.resolve();
-  } else if (options.project === undefined) {
-    ctx.logger.warn('Missing "project" option. Will not create release.');
-    return Promise.resolve();
-  }
-
   ctx.logger.info("Uploading Sourcemaps.");
 
   // Since our internal include entries contain all top-level sourcemaps options,
   // we only need to pass the include option here.
   await ctx.cli.releases.uploadSourceMaps(options.release, { include: options.include });
 
   ctx.logger.info("Successfully uploaded Sourcemaps.");
-
   span?.finish();
 }
 
@@ -73,20 +46,7 @@ export async function cleanArtifacts(options: InternalOptions, ctx: BuildContext
   const span = addSpanToTransaction(ctx, "function.plugin.clean_artifacts");
 
   if (options.cleanArtifacts) {
-    // TODO: pull these checks out of here and simplify them
-    if (options.authToken === undefined) {
-      ctx.logger.warn('Missing "authToken" option. Will not clean existing artifacts.');
-      return;
-    } else if (options.org === undefined) {
-      ctx.logger.warn('Missing "org" option. Will not clean existing artifacts.');
-      return;
-    } else if (options.project === undefined) {
-      ctx.logger.warn('Missing "project" option. Will not clean existing artifacts.');
-      return;
-    }
-
     await ctx.cli.releases.execute(["releases", "files", options.release, "delete", "--all"], true);
-
     ctx.logger.info("Successfully cleaned previous artifacts.");
   }
 
@@ -99,22 +59,16 @@ export async function setCommits(options: InternalOptions, ctx: BuildContext): P
   if (options.setCommits) {
     const { auto, repo, commit, previousCommit, ignoreMissing, ignoreEmpty } = options.setCommits;
 
-    if (auto || (repo && commit)) {
-      await ctx.cli.releases.setCommits(options.release, {
-        commit,
-        previousCommit,
-        repo,
-        auto,
-        ignoreMissing,
-        ignoreEmpty,
-      });
-      ctx.logger.info("Successfully set commits.");
-    } else {
-      ctx.logger.error(
-        "Couldn't set commits - neither the `auto` nor the `repo` and `commit` options were specified!",
-        "Make sure to either set `auto` to `true` or to manually set `repo` and `commit`."
-      );
-    }
+    await ctx.cli.releases.setCommits(options.release, {
+      commit,
+      previousCommit,
+      repo,
+      auto,
+      ignoreMissing,
+      ignoreEmpty,
+    });
+
+    ctx.logger.info("Successfully set commits.");
   }
 
   span?.finish();
@@ -126,22 +80,16 @@ export async function addDeploy(options: InternalOptions, ctx: BuildContext): Pr
   if (options.deploy) {
     const { env, started, finished, time, name, url } = options.deploy;
 
-    if (env) {
-      await ctx.cli.releases.newDeploy(options.release, {
-        env,
-        started,
-        finished,
-        time,
-        name,
-        url,
-      });
-      ctx.logger.info("Successfully added deploy.");
-    } else {
-      ctx.logger.error(
-        "Couldn't add deploy - the `env` option was not specified!",
-        "Make sure to set `deploy.env` (e.g. to 'production')."
-      );
-    }
+    await ctx.cli.releases.newDeploy(options.release, {
+      env,
+      started,
+      finished,
+      time,
+      name,
+      url,
+    });
+
+    ctx.logger.info("Successfully added deploy.");
   }
 
   span?.finish();

packages/bundler-plugin-core/src/sentry/telemetry.ts

@@ -12,8 +12,7 @@ import { BuildContext } from "../types";
 
 export function makeSentryClient(
   dsn: string,
-  telemetryEnabled: boolean,
-  org?: string
+  telemetryEnabled: boolean
 ): { client: NodeClient; hub: Hub } {
   const client = new NodeClient({
     dsn,
@@ -34,12 +33,6 @@ export function makeSentryClient(
 
   const hub = new Hub(client);
 
-  hub.configureScope((scope) => {
-    if (org) {
-      scope.setTag("org", org);
-    }
-  });
-
   //TODO: This call is problematic because as soon as we set our hub as the current hub
   //      we might interfere with other plugins that use Sentry. However, for now, we'll
   //      leave it in because without it, we can't get distributed traces (which are pretty nice)