fix: missing conductor_url in aws code and improving handling of erro…

…r/warn conditions (#1130)

# fix: missing conductor_url in aws code and improving handling of
error/warn conditions

## JIRA Ticket

[BSS-224](https://jira.csiro.au/browse/BSS-224)

## Description

Fixes missing environment variable in conductor API configuration for
AWS deployment.

Refactors default conductor URLs to constants in the app and api code. 

Adds doc strings for conductor URL split method and handles error
conditions more durably including filtering out empty values which could
result from trailing commas, and warning under either empty string or
empty list conditions. Falls through to the default value if the length
of the list after splitting and filtering out empty values is zero, such
as in the case where the environment variable is an empty string.

## How to Test

1. Testing changes to conductor URL splitting logic: I added unit tests
for this function. See `cd app && npm i && npx vitest buildconfig`.

2. Testing AWS deployment - not on critical path right now but once
merged I'll update the downstream fork and redeploy and ensure it works.

## Checklist

- [x] I have confirmed all commits have been signed.
- [x] I have added JSDoc style comments to any new functions or classes.
- [x] Relevant documentation such as READMEs, guides, and class comments
are updated.

api/src/buildconfig.ts

@@ -30,6 +30,11 @@ const FALSEY_STRINGS = ['false', '0', 'off', 'no'];
 export const CLUSTER_ADMIN_GROUP_NAME = 'cluster-admin';
 export const NOTEBOOK_CREATOR_GROUP_NAME = 'notebook-creator';
 
+// Constants
+
+// If a URL for the conductor instance is not provided this will be used as a fall-through
+const DEFAULT_CONDUCTOR_URL = 'http://localhost:8080';
+
 /*
  * This is designed to get useful commit information data from
  * environment variables for the testing server. While more sophisticated
@@ -56,9 +61,13 @@ function commit_version(): string {
 function conductor_url(): string {
   const url = process.env.CONDUCTOR_PUBLIC_URL;
   if (url === '' || url === undefined) {
-    return 'http://localhost:8080';
+    console.warn(
+      `No value for CONDUCTOR_PUBLIC_URL was provided in the environment. Defaulting to ${DEFAULT_CONDUCTOR_URL}.`
+    );
+    return DEFAULT_CONDUCTOR_URL;
+  } else {
+    return url;
   }
-  return url;
 }
 
 function app_url(): string {

app/src/buildconfig.test.ts

@@ -0,0 +1,44 @@
+/*
+ * Copyright 2021, 2022 Macquarie University
+ *
+ * Licensed under the Apache License Version 2.0 (the, "License");
+ * you may not use, this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing software
+ * distributed under the License is distributed on an "AS IS" BASIS
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND either express or implied.
+ * See, the License, for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * Filename: buildconfig.test.ts
+ * Description:
+ *   This test file checks that the parsing of conductor URLs meets specifications
+ */
+
+// eslint-disable-next-line n/no-unpublished-import
+import {expect, it, describe} from 'vitest';
+import {DEFAULT_CONDUCTOR_URL, parseConductorUrls} from './buildconfig';
+
+describe('parse conductor URLs', () => {
+  it('defaults to the DEFAULT_CONDUCTOR_URL if empty string provided', () => {
+    const res = parseConductorUrls('');
+    expect(res).toEqual([DEFAULT_CONDUCTOR_URL]);
+  });
+  it('trims excess whitespace', () => {
+    const res = parseConductorUrls('value1,value2 , value 3 , value4');
+    expect(res).toEqual(['value1', 'value2', 'value 3', 'value4']);
+  });
+  it('removes trailing values properly', () => {
+    const res1 = parseConductorUrls('value1,');
+    expect(res1).toEqual(['value1']);
+    const res2 = parseConductorUrls('value1,   ');
+    expect(res2).toEqual(['value1']);
+  });
+  it('handles malformed inputs such as just commas with spaces by falling through to default', () => {
+    const res1 = parseConductorUrls(', ,,  , , ,,   ,   ');
+    expect(res1).toEqual([DEFAULT_CONDUCTOR_URL]);
+  });
+});

app/src/buildconfig.ts

@@ -13,7 +13,7 @@
  * See, the License, for the specific language governing permissions and
  * limitations under the License.
  *
- * Filename: reportWebVitals.ts
+ * Filename: buildconfig.ts
  * Description:
  *   This module exports the configuration of the build, including things like
  *   which server to use and whether to include test data
@@ -24,6 +24,11 @@ import {BUILD_VERSION, BUILD_VERSION_DEFAULT} from './version';
 // need to define a local logError here since logging.tsx imports this file
 const logError = (err: any) => console.error(err);
 
+// Constants
+
+// Default conductor URL
+export const DEFAULT_CONDUCTOR_URL = 'http://localhost:8154';
+
 const TRUTHY_STRINGS = ['true', '1', 'on', 'yes'];
 const FALSEY_STRINGS = ['false', '0', 'off', 'no'];
 
@@ -246,16 +251,50 @@ function get_bugsnag_key(): string | false {
   return bugsnag_key;
 }
 
+/**
+ * Splits the input, trimming for whitespace and filtering empty strings.
+ * Handles cases including an empty resulting list, and warns on empty strings
+ * being contained. Falls through to the DEFAULT_CONDUCTOR_URL where needed.
+ * @returns A list of conductor URLs which can act as servers.
+ */
+export function parseConductorUrls(conductorUrls: string): string[] {
+  const urls = conductorUrls.split(',');
+  // Provide a warning if the split results in any empty strings
+  if (urls.some((url: string) => url.length === 0)) {
+    console.warn(
+      `CONDUCTOR_URL value was provided, but when split, contained entries which were empty. Value: ${conductorUrls}. After split: ${urls}.`
+    );
+  }
+  // return URLs without trailing whitespace and excluding any values which are empty e.g. due to a trailing comma
+
+  const filteredUrls = urls
+    .map((url: string) => url.trim())
+    .filter((url: string) => url.length > 0);
+
+  // Provide a warning if the split results in an empty list
+  if (!(filteredUrls.length > 0)) {
+    console.error(
+      `CONDUCTOR_URL value was provided, but when split, trimmed, and empty strings removed, had a length of zero. Value: ${conductorUrls}. After split: ${urls}. After filter: ${filteredUrls}. Returning default [${DEFAULT_CONDUCTOR_URL}]`
+    );
+    return [DEFAULT_CONDUCTOR_URL];
+  }
+
+  return filteredUrls;
+}
+/**
+ * Hands the VITE_CONDUCTOR_URL input to the parse conductor urls method,
+ * returning a safely handled list of conductor URLs.
+ * @returns A list of conductor URLs which can act as servers.
+ */
 function get_conductor_urls(): string[] {
-  const config = import.meta.env.VITE_CONDUCTOR_URL;
-  if (config) {
-    const urls = config.split(',');
-    return urls.map((url: string) => url.trim());
+  const conductorUrls: string = import.meta.env.VITE_CONDUCTOR_URL;
+  if (conductorUrls) {
+    return parseConductorUrls(conductorUrls);
   } else {
-    console.error(
-      'No CONDUCTOR URL configured, using default development server'
+    console.warn(
+      `No CONDUCTOR URL configured, using default development server at ${DEFAULT_CONDUCTOR_URL}.`
     );
-    return ['http://localhost:8154'];
+    return [DEFAULT_CONDUCTOR_URL];
   }
 }
 

app/src/gui/layout/appBar.tsx

@@ -56,8 +56,12 @@ import {TokenContents} from '@faims3/data-model';
 import {checkToken} from '../../utils/helpers';
 import SyncStatus from '../components/sync';
 import {NOTEBOOK_NAME, NOTEBOOK_NAME_CAPITALIZED} from '../../buildconfig';
+
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
 import HelpIcon from '@mui/icons-material/Help';
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
 import SwapHorizIcon from '@mui/icons-material/SwapHoriz';
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
 import {now} from 'lodash';
 
 /**

infrastructure/aws-cdk/lib/components/conductor.ts

@@ -140,7 +140,9 @@ export class FaimsConductor extends Construct {
           COUCHDB_EXTERNAL_PORT: `${props.couchDBPort}`,
           COUCHDB_PUBLIC_URL: props.couchDBEndpoint,
           COUCHDB_INTERNAL_URL: props.couchDBEndpoint,
+          // Conductor API URLs
           CONDUCTOR_PUBLIC_URL: this.conductorEndpoint,
+          CONDUCTOR_URL: this.conductorEndpoint,
           // TODO Setup Google auth
           CONDUCTOR_AUTH_PROVIDERS: 'google',
           GOOGLE_CLIENT_ID: 'replace-me',