ui: Warning and better error handling for invalid-named variables and…

… job templates (#19989) * Warning and better error handling for invalid-named variables and job templates * Warning and better error handling for invalid-named variables and job templates * Tests for variable pathname warnings * Only show the bad-name warning if the variable is being created and path is editable
hashicorp · Feb 15, 2024 · e8db588 · e8db588
1 parent c1cbc39
commit e8db588
Show file tree

Hide file tree

Showing 7 changed files with 102 additions and 7 deletions.
diff --git a/.changelog/19989.txt b/.changelog/19989.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+ui: Improve error and warning messages for invalid variable and job template paths/names
+```
diff --git a/ui/app/adapters/variable.js b/ui/app/adapters/variable.js
@@ -6,6 +6,7 @@
 // @ts-check
 import ApplicationAdapter from './application';
 import AdapterError from '@ember-data/adapter/error';
+import InvalidError from '@ember-data/adapter/error';
 import { pluralize } from 'ember-inflector';
 import classic from 'ember-classic-decorator';
 import { ConflictError } from '@ember-data/adapter/error';
@@ -141,6 +142,15 @@ export default class VariableAdapter extends ApplicationAdapter {
         { detail: _normalizeConflictErrorObject(payload), status: 409 },
       ]);
     }
+    if (status === 400) {
+      return new InvalidError([
+        {
+          detail:
+            'Invalid name. Name must contain only alphanumeric or "-", "_", "~", or "/" characters, and be fewer than 128 characters in length.',
+          status: 400,
+        },
+      ]);
+    }
     return super.handleResponse(...arguments);
   }
 }

diff --git a/ui/app/components/variable-form.hbs b/ui/app/components/variable-form.hbs
@@ -55,7 +55,7 @@
 			@isRequired={{true}}
 			@value={{this.path}}
 			placeholder="nomad/jobs/my-job/my-group/my-task"
-      @isInvalid={{this.duplicatePathWarning}}
+      @isInvalid={{or this.duplicatePathWarning (and @model.isNew this.hasInvalidPath)}}
       {{on "input" (action this.setModelPath)}}
       disabled={{not @model.isNew}}
 			{{autofocus}}
@@ -79,6 +79,13 @@
           .
         </F.Error>
       {{/if}}
+      {{#if @model.isNew}}
+        {{#if this.hasInvalidPath}}
+          <F.Error data-test-invalid-path-error>
+            Path must contain only alphanumeric or "-", "_", "~", or "/" characters, and be fewer than 128 characters in length.
+          </F.Error>
+        {{/if}}
+      {{/if}}
       {{#if this.isJobTemplateVariable}}
         <F.HelperText data-test-job-template-hint>
           Use this variable to generate job templates with <Hds::Copy::Snippet @textToCopy="nomad job init -template={{this.jobTemplateName}}" />

diff --git a/ui/app/components/variable-form.js b/ui/app/components/variable-form.js
@@ -154,6 +154,11 @@ export default class VariableFormComponent extends Component {
     }
   }
 
+  get hasInvalidPath() {
+    let pathNameRegex = new RegExp('^[a-zA-Z0-9-_~/]{1,128}$');
+    return !pathNameRegex.test(trimPath([this.path]));
+  }
+
   @action
   validateKey(entry, e) {
     const value = e.target.value;
@@ -269,18 +274,27 @@ export default class VariableFormComponent extends Component {
 
       this.removeExitHandler();
       this.router.transitionTo('variables.variable', this.args.model.id);
-    } catch (error) {
-      notifyConflict(this)(error);
+    } catch (e) {
+      notifyConflict(this)(e);
       if (!this.hasConflict) {
+        let errorMessage = e;
+        if (e.errors && e.errors.length > 0) {
+          const nameInvalidError = e.errors.find((err) => err.status === 400);
+          if (nameInvalidError) {
+            errorMessage = nameInvalidError.detail;
+          }
+        }
+
+        console.log('caught an error', e);
         this.notifications.add({
           title: `Error saving ${this.path}`,
-          message: error,
+          message: errorMessage,
           color: 'critical',
           sticky: true,
         });
       } else {
-        if (error.errors[0]?.detail) {
-          set(this, 'conflictingVariable', error.errors[0].detail);
+        if (e.errors[0]?.detail) {
+          set(this, 'conflictingVariable', e.errors[0].detail);
         }
         window.scrollTo(0, 0); // because the k/v list may be long, ensure the user is snapped to top to read error
       }

diff --git a/ui/app/controllers/jobs/run/templates/new.js b/ui/app/controllers/jobs/run/templates/new.js
@@ -36,6 +36,11 @@ export default class JobsRunTemplatesNewController extends Controller {
       );
   }
 
+  get hasInvalidName() {
+    let pathNameRegex = new RegExp('^[a-zA-Z0-9-_~/]{1,128}$');
+    return !pathNameRegex.test(this.templateName);
+  }
+
   @action
   updateKeyValue(key, value) {
     if (this.model.keyValues.find((kv) => kv.key === key)) {
@@ -74,9 +79,19 @@ export default class JobsRunTemplatesNewController extends Controller {
 
       this.router.transitionTo('jobs.run.templates');
     } catch (e) {
+      let errorMessage =
+        'An unexpected error occurred when saving your Job template.';
+      console.log('caught', e);
+      if (e.errors && e.errors.length > 0) {
+        const nameInvalidError = e.errors.find((err) => err.status === 400);
+        if (nameInvalidError) {
+          errorMessage = nameInvalidError.detail;
+        }
+      }
+
       this.notifications.add({
         title: 'Job template cannot be saved.',
-        message: e,
+        message: errorMessage,
         color: 'critical',
       });
     }

diff --git a/ui/app/templates/jobs/run/templates/new.hbs b/ui/app/templates/jobs/run/templates/new.hbs
@@ -28,6 +28,11 @@
                         There is already a templated named {{this.templateName}}.
                     </p>
                 {{/if}}
+                {{#if this.hasInvalidName}}
+                  <p class="help is-danger" data-test-invalid-name-error>
+                    Template name must contain only alphanumeric or "-", "_", "~", or "/" characters, and be fewer than 128 characters in length.
+                  </p>
+                {{/if}}
             </label>
             {{#if this.system.shouldShowNamespaces}}
                 <label>

diff --git a/ui/tests/integration/components/variable-form-test.js b/ui/tests/integration/components/variable-form-test.js
@@ -279,6 +279,47 @@ module('Integration | Component | variable-form', function (hooks) {
         .hasClass('hds-form-text-input--is-invalid');
     });
 
+    test('warns when you try to create a path with invalid characters', async function (assert) {
+      this.server.createList('namespace', 3);
+
+      this.set(
+        'mockedModel',
+        server.create('variable', {
+          path: '',
+          keyValues: [{ key: '', value: '' }],
+        })
+      );
+
+      await render(hbs`<VariableForm @model={{this.mockedModel}} />`);
+
+      await typeIn('[data-test-path-input]', 'foo-bar');
+      assert.dom('[data-test-invalid-path-error]').doesNotExist();
+      assert
+        .dom('[data-test-path-input]')
+        .doesNotHaveClass('hds-form-text-input--is-invalid');
+
+      document.querySelector('[data-test-path-input]').value = ''; // clear current input
+      await typeIn('[data-test-path-input]', 'foo bar');
+
+      assert
+        .dom('[data-test-invalid-path-error]')
+        .exists('Space makes path invalid');
+      assert
+        .dom('[data-test-path-input]')
+        .hasClass('hds-form-text-input--is-invalid');
+
+      document.querySelector('[data-test-path-input]').value = ''; // clear current input
+      await typeIn('[data-test-path-input]', '_');
+      assert.dom('[data-test-invalid-path-error]').doesNotExist();
+
+      // Try 129 characters
+      let longString = 'a'.repeat(129);
+      await typeIn('[data-test-path-input]', longString);
+      assert
+        .dom('[data-test-invalid-path-error]')
+        .exists('Long name makes path invalid');
+    });
+
     test('warns you when you set a key with . in it', async function (assert) {
       this.set(
         'mockedModel',