From 2ceda25637935753fe0404d79ef7bf79d65e556d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Luismi=20Ram=C3=ADrez?= <luismi@appsignal.com>
Date: Thu, 4 Nov 2021 12:38:50 +0100
Subject: [PATCH] Implement config validation (#482)

Config was always considered to be valid as `Config.isValid()` just had
a `return true` inside. Now we check the presence of an apiKey in the
configuration data to validate the config.
---
 package-lock.json                             | 32 +++++++++----------
 ...ed-valid-anymore-when-apikey-is-missing.md |  5 +++
 packages/nodejs/src/__tests__/client.test.ts  |  6 ++++
 packages/nodejs/src/__tests__/config.test.ts  | 22 +++++++++++++
 packages/nodejs/src/client.ts                 |  2 +-
 packages/nodejs/src/config.ts                 |  4 +--
 6 files changed, 51 insertions(+), 20 deletions(-)
 create mode 100644 packages/nodejs/.changesets/config-is-not-considered-valid-anymore-when-apikey-is-missing.md

diff --git a/package-lock.json b/package-lock.json
index eac7d1dd..de6cc34a 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -10630,10 +10630,10 @@
     },
     "packages/apollo-server": {
       "name": "@appsignal/apollo-server",
-      "version": "1.0.13",
+      "version": "1.0.16",
       "license": "MIT",
       "dependencies": {
-        "@appsignal/nodejs": "=2.2.1",
+        "@appsignal/nodejs": "=2.2.4",
         "apollo-server-plugin-base": "^0.10.3",
         "tslib": "^2.0.3"
       },
@@ -10648,10 +10648,10 @@
     },
     "packages/express": {
       "name": "@appsignal/express",
-      "version": "1.0.14",
+      "version": "1.0.17",
       "license": "MIT",
       "dependencies": {
-        "@appsignal/nodejs": "=2.2.1",
+        "@appsignal/nodejs": "=2.2.4",
         "tslib": "^2.0.3"
       },
       "devDependencies": {
@@ -10669,10 +10669,10 @@
     },
     "packages/koa": {
       "name": "@appsignal/koa",
-      "version": "1.0.4",
+      "version": "1.0.7",
       "license": "MIT",
       "dependencies": {
-        "@appsignal/nodejs": "=2.2.1",
+        "@appsignal/nodejs": "=2.2.4",
         "@appsignal/types": "^2.1.5",
         "shimmer": "^1.2.1",
         "tslib": "^2.0.3"
@@ -10694,10 +10694,10 @@
     },
     "packages/nextjs": {
       "name": "@appsignal/nextjs",
-      "version": "2.0.2",
+      "version": "2.0.5",
       "license": "MIT",
       "dependencies": {
-        "@appsignal/nodejs": "=2.2.1",
+        "@appsignal/nodejs": "=2.2.4",
         "tslib": "^2.0.3"
       },
       "devDependencies": {
@@ -10714,7 +10714,7 @@
     },
     "packages/nodejs": {
       "name": "@appsignal/nodejs",
-      "version": "2.2.1",
+      "version": "2.2.4",
       "license": "MIT",
       "dependencies": {
         "@appsignal/core": "^1.1.4",
@@ -10738,12 +10738,12 @@
         "node": ">= 12"
       },
       "optionalDependencies": {
-        "@appsignal/nodejs-ext": "=2.0.2"
+        "@appsignal/nodejs-ext": "=2.0.3"
       }
     },
     "packages/nodejs-ext": {
       "name": "@appsignal/nodejs-ext",
-      "version": "2.0.2",
+      "version": "2.0.3",
       "cpu": [
         "x64",
         "x86"
@@ -10816,7 +10816,7 @@
     "@appsignal/apollo-server": {
       "version": "file:packages/apollo-server",
       "requires": {
-        "@appsignal/nodejs": "=2.2.1",
+        "@appsignal/nodejs": "=2.2.4",
         "apollo-server-plugin-base": "*",
         "tslib": "^2.0.3"
       },
@@ -10848,7 +10848,7 @@
     "@appsignal/express": {
       "version": "file:packages/express",
       "requires": {
-        "@appsignal/nodejs": "=2.2.1",
+        "@appsignal/nodejs": "=2.2.4",
         "@types/express": "*",
         "express": "*",
         "tslib": "^2.0.3"
@@ -10864,7 +10864,7 @@
     "@appsignal/koa": {
       "version": "file:packages/koa",
       "requires": {
-        "@appsignal/nodejs": "=2.2.1",
+        "@appsignal/nodejs": "=2.2.4",
         "@appsignal/types": "^2.1.5",
         "@types/koa": "*",
         "@types/koa__router": "*",
@@ -10884,7 +10884,7 @@
     "@appsignal/nextjs": {
       "version": "file:packages/nextjs",
       "requires": {
-        "@appsignal/nodejs": "=2.2.1",
+        "@appsignal/nodejs": "=2.2.4",
         "next": "*",
         "tslib": "^2.0.3"
       },
@@ -10900,7 +10900,7 @@
       "version": "file:packages/nodejs",
       "requires": {
         "@appsignal/core": "^1.1.4",
-        "@appsignal/nodejs-ext": "=2.0.2",
+        "@appsignal/nodejs-ext": "=2.0.3",
         "@appsignal/types": "^2.1.5",
         "@types/pg": "*",
         "@types/redis": "*",
diff --git a/packages/nodejs/.changesets/config-is-not-considered-valid-anymore-when-apikey-is-missing.md b/packages/nodejs/.changesets/config-is-not-considered-valid-anymore-when-apikey-is-missing.md
new file mode 100644
index 00000000..38ffd2fa
--- /dev/null
+++ b/packages/nodejs/.changesets/config-is-not-considered-valid-anymore-when-apikey-is-missing.md
@@ -0,0 +1,5 @@
+---
+bump: "patch"
+---
+
+The AppSignal config is not considered valid anymore when the apiKey config option is not set.
diff --git a/packages/nodejs/src/__tests__/client.test.ts b/packages/nodejs/src/__tests__/client.test.ts
index c451ff52..0d1ff138 100644
--- a/packages/nodejs/src/__tests__/client.test.ts
+++ b/packages/nodejs/src/__tests__/client.test.ts
@@ -28,6 +28,12 @@ describe("BaseClient", () => {
     expect(client.isActive).toBeFalsy()
   })
 
+  it("does not start the client if config is not valid", () => {
+    process.env["APPSIGNAL_PUSH_API_KEY"] = undefined
+    client = new BaseClient({ name, enableMinutelyProbes: false })
+    expect(client.isActive).toBeFalsy()
+  })
+
   it("starts the client when the active option is true", () => {
     client = new BaseClient({ ...DEFAULT_OPTS, active: true })
     expect(client.isActive).toBeTruthy()
diff --git a/packages/nodejs/src/__tests__/config.test.ts b/packages/nodejs/src/__tests__/config.test.ts
index 73cddb4c..b887d78f 100644
--- a/packages/nodejs/src/__tests__/config.test.ts
+++ b/packages/nodejs/src/__tests__/config.test.ts
@@ -86,6 +86,28 @@ describe("Configuration", () => {
     })
   })
 
+  describe(".isValid", () => {
+    it("is valid if apiKey is present", () => {
+      expect(config.isValid).toBeTruthy()
+    })
+
+    it("is invalid if apiKey is not present", () => {
+      process.env["APPSIGNAL_PUSH_API_KEY"] = undefined
+      config = new Configuration({ name })
+      expect(config.isValid).toBeFalsy()
+    })
+
+    it("is invalid if apiKey is an empty string", () => {
+      config = new Configuration({ name, apiKey: "" })
+      expect(config.isValid).toBeFalsy()
+    })
+
+    it("is invalid if apiKey is a string with only whitespaces", () => {
+      config = new Configuration({ name, apiKey: "  " })
+      expect(config.isValid).toBeFalsy()
+    })
+  })
+
   describe("Overriden log path with file specified", () => {
     beforeEach(() => {
       process.env["APPSIGNAL_LOG_PATH"] = "/other_path/appsignal.log"
diff --git a/packages/nodejs/src/client.ts b/packages/nodejs/src/client.ts
index 9cd59676..369913a2 100644
--- a/packages/nodejs/src/client.ts
+++ b/packages/nodejs/src/client.ts
@@ -68,7 +68,7 @@ export class BaseClient implements Client {
     if (this.config.isValid) {
       this.extension.start()
     } else {
-      console.error("Not starting, no valid config for this environment")
+      console.error("Not starting, no valid AppSignal configuration found")
     }
   }
 
diff --git a/packages/nodejs/src/config.ts b/packages/nodejs/src/config.ts
index b9f4f852..be4cd818 100644
--- a/packages/nodejs/src/config.ts
+++ b/packages/nodejs/src/config.ts
@@ -36,11 +36,9 @@ export class Configuration {
 
   /**
    * Returns `true` if the current configuration is valid.
-   *
-   * @todo
    */
   public get isValid(): boolean {
-    return true
+    return (this.data.apiKey || "").trim() !== ""
   }
 
   /**