Handle conflict swith a cas qp on save and create (#14100)

* Handle conflict swith a cas qp on save and create

* Notify error and give them refresh or overwrite options

* Merge conflict missed, resolved

* Mirage fixture

* Integration test

* Bracket closed (thx jai)

* Adjust tests to account for number of variables with auto-conflicter

ui/app/adapters/variable.js

@@ -1,6 +1,7 @@
 import ApplicationAdapter from './application';
 import { pluralize } from 'ember-inflector';
 import classic from 'ember-classic-decorator';
+import { ConflictError } from '@ember-data/adapter/error';
 
 @classic
 export default class VariableAdapter extends ApplicationAdapter {
@@ -11,7 +12,8 @@ export default class VariableAdapter extends ApplicationAdapter {
   createRecord(_store, type, snapshot) {
     let data = this.serialize(snapshot);
     let baseUrl = this.buildURL(type.modelName, data.ID);
-    return this.ajax(baseUrl, 'PUT', { data });
+    const checkAndSetValue = snapshot?.attr('modifyIndex') || 0;
+    return this.ajax(`${baseUrl}&cas=${checkAndSetValue}`, 'PUT', { data });
   }
 
   urlForFindAll(modelName) {
@@ -33,7 +35,12 @@ export default class VariableAdapter extends ApplicationAdapter {
   urlForUpdateRecord(identifier, modelName, snapshot) {
     const { id } = _extractIDAndNamespace(identifier, snapshot);
     let baseUrl = this.buildURL(modelName, id);
-    return `${baseUrl}`;
+    if (snapshot?.adapterOptions?.overwrite) {
+      return `${baseUrl}`;
+    } else {
+      const checkAndSetValue = snapshot?.attr('modifyIndex') || 0;
+      return `${baseUrl}?cas=${checkAndSetValue}`;
+    }
   }
 
   urlForDeleteRecord(identifier, modelName, snapshot) {
@@ -41,6 +48,15 @@ export default class VariableAdapter extends ApplicationAdapter {
     const baseUrl = this.buildURL(modelName, id);
     return `${baseUrl}?namespace=${namespace}`;
   }
+
+  handleResponse(status, _, payload) {
+    if (status === 409) {
+      return new ConflictError([
+        { detail: _normalizeConflictErrorObject(payload), status: 409 },
+      ]);
+    }
+    return super.handleResponse(...arguments);
+  }
 }
 
 function _extractIDAndNamespace(identifier, snapshot) {
@@ -51,3 +67,10 @@ function _extractIDAndNamespace(identifier, snapshot) {
     id,
   };
 }
+
+function _normalizeConflictErrorObject(conflictingVariable) {
+  return {
+    modifyTime: Math.floor(conflictingVariable.ModifyTime / 1000000),
+    items: conflictingVariable.Items,
+  };
+}

ui/app/components/secure-variable-form.hbs

@@ -9,6 +9,27 @@
     </div>
   {{/if}}
 
+  {{#if this.hasConflict}}
+    <div class="notification conflict is-danger">
+      <h3 class="title is-4">Heads up! Your Secure Variable has a conflict.</h3>
+      <p>This might be because someone else tried saving in the time since you've had it open.</p>
+      {{#if this.conflictingVariable.modifyTime}}
+        <span class="tooltip" aria-label="{{format-ts this.conflictingVariable.modifyTime}}">
+          {{moment-from-now this.conflictingVariable.modifyTime}}
+        </span>
+      {{/if}}
+      {{#if this.conflictingVariable.items}}
+        <pre><code>{{stringify-object this.conflictingVariable.items whitespace=2}}</code></pre>
+      {{else}}
+        <p>Your ACL token limits your ability to see further details about the conflicting variable.</p>
+      {{/if}}
+      <div class="options">
+        <button data-test-refresh-button type="button" class="button" {{on "click" this.refresh}}>Refresh your browser</button>
+        <button data-test-overwrite-button type="button" class="button is-danger" {{on "click" this.saveWithOverwrite}}>Overwrite anyway</button>
+      </div>
+    </div>
+  {{/if}}
+
   <div class={{if this.namespaceOptions "path-namespace"}}>
     <label>
       <span>

ui/app/components/secure-variable-form.js

@@ -11,6 +11,7 @@ import EmberObject, { set } from '@ember/object';
 import MutableArray from '@ember/array/mutable';
 import { A } from '@ember/array';
 import { stringifyObject } from 'nomad-ui/helpers/stringify-object';
+import notifyConflict from 'nomad-ui/utils/notify-conflict';
 
 const EMPTY_KV = {
   key: '',
@@ -25,6 +26,18 @@ export default class SecureVariableFormComponent extends Component {
 
   @tracked variableNamespace = null;
   @tracked namespaceOptions = null;
+  @tracked hasConflict = false;
+
+  /**
+   * @typedef {Object} conflictingVariable
+   * @property {string} ModifyTime
+   * @property {Object} Items
+   */
+
+  /**
+   * @type {conflictingVariable}
+   */
+  @tracked conflictingVariable = null;
 
   @tracked path = '';
   constructor() {
@@ -144,8 +157,17 @@ export default class SecureVariableFormComponent extends Component {
     this.keyValues.removeObject(row);
   }
 
+  @action refresh() {
+    window.location.reload();
+  }
+
+  @action saveWithOverwrite(e) {
+    set(this, 'conflictingVariable', null);
+    this.save(e, true);
+  }
+
   @action
-  async save(e) {
+  async save(e, overwrite = false) {
     if (e.type === 'submit') {
       e.preventDefault();
     }
@@ -175,7 +197,7 @@ export default class SecureVariableFormComponent extends Component {
       this.args.model.set('keyValues', this.keyValues);
       this.args.model.set('path', this.path);
       this.args.model.setAndTrimPath();
-      await this.args.model.save();
+      await this.args.model.save({ adapterOptions: { overwrite } });
 
       this.flashMessages.add({
         title: 'Secure Variable saved',
@@ -186,13 +208,21 @@ export default class SecureVariableFormComponent extends Component {
       });
       this.router.transitionTo('variables.variable', this.args.model.id);
     } catch (error) {
-      this.flashMessages.add({
-        title: `Error saving ${this.path}`,
-        message: error,
-        type: 'error',
-        destroyOnClick: false,
-        sticky: true,
-      });
+      notifyConflict(this)(error);
+      if (!this.hasConflict) {
+        this.flashMessages.add({
+          title: `Error saving ${this.path}`,
+          message: error,
+          type: 'error',
+          destroyOnClick: false,
+          sticky: true,
+        });
+      } else {
+        if (error.errors[0]?.detail) {
+          set(this, 'conflictingVariable', error.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
+      }
     }
   }
 

ui/app/styles/components/secure-variables.scss

@@ -98,6 +98,28 @@
       }
     }
   }
+
+  .notification.conflict {
+    color: $text;
+
+    p {
+      margin-bottom: 1rem;
+    }
+
+    pre {
+      background: black;
+      color: $white;
+      max-height: 500px;
+      overflow: auto;
+      code {
+        height: 100%;
+      }
+    }
+
+    .options {
+      margin: 1rem 0;
+    }
+  }
 }
 
 table.path-tree {

ui/app/utils/notify-conflict.js

@@ -0,0 +1,14 @@
+// @ts-check
+// Catches errors with conflicts (409)
+// and allow the route to handle them.
+import { set } from '@ember/object';
+import codesForError from './codes-for-error';
+export default function notifyConflict(parent) {
+  return (error) => {
+    if (codesForError(error).includes('409')) {
+      set(parent, 'hasConflict', true);
+    } else {
+      return error;
+    }
+  };
+}

ui/mirage/config.js

@@ -6,6 +6,7 @@ import { generateDiff } from './factories/job-version';
 import { generateTaskGroupFailures } from './factories/evaluation';
 import { copy } from 'ember-copy';
 import formatHost from 'nomad-ui/utils/format-host';
+import faker from 'nomad-ui/mirage/faker';
 
 export function findLeader(schema) {
   const agent = schema.agents.first();
@@ -852,12 +853,28 @@ export default function () {
 
   this.put('/var/:id', function (schema, request) {
     const { Path, Namespace, Items } = JSON.parse(request.requestBody);
-    return server.create('variable', {
-      path: Path,
-      namespace: Namespace,
-      items: Items,
-      id: Path,
-    });
+    if (request.url.includes('cas=') && Path === 'Auto-conflicting Variable') {
+      return new Response(
+        409,
+        {},
+        {
+          CreateIndex: 65,
+          CreateTime: faker.date.recent(14) * 1000000, // in the past couple weeks
+          Items: { edited_by: 'your_remote_pal' },
+          ModifyIndex: 2118,
+          ModifyTime: faker.date.recent(0.01) * 1000000, // a few minutes ago
+          Namespace: Namespace,
+          Path: Path,
+        }
+      );
+    } else {
+      return server.create('variable', {
+        path: Path,
+        namespace: Namespace,
+        items: Items,
+        id: Path,
+      });
+    }
   });
 
   this.delete('/var/:id', function (schema, request) {

ui/mirage/scenarios/default.js

@@ -93,6 +93,11 @@ function smallCluster(server) {
     namespace: variableLinkedJob.namespace,
   });
 
+  server.create('variable', {
+    id: 'Auto-conflicting Variable',
+    namespace: 'default',
+  });
+
   // #region evaluations
 
   // Branching: a single eval that relates to N-1 mutually-unrelated evals
@@ -231,6 +236,11 @@ function variableTestCluster(server) {
     id: 'another arbitrary file again',
     namespace: 'namespace-2',
   });
+
+  server.create('variable', {
+    id: 'Auto-conflicting Variable',
+    namespace: 'default',
+  });
 }
 
 // Due to Mirage performance, large cluster scenarios will be slow

ui/tests/acceptance/secure-variables-test.js

@@ -553,6 +553,43 @@ module('Acceptance | secure variables', function (hooks) {
       // Reset Token
       window.localStorage.nomadTokenSecret = null;
     });
+    test('handles conflicts on save', async function (assert) {
+      // Arrange Test Set-up
+      allScenarios.variableTestCluster(server);
+      const variablesToken = server.db.tokens.find(SECURE_TOKEN_ID);
+      window.localStorage.nomadTokenSecret = variablesToken.secretId;
+      // End Test Set-up
+
+      await Variables.visitConflicting();
+      await click('button[type="submit"]');
+
+      assert
+        .dom('.notification.conflict')
+        .exists('Notification alerting user of conflict is present');
+
+      document.querySelector('[data-test-var-key]').value = ''; // clear current input
+      await typeIn('[data-test-var-key]', 'buddy');
+      await typeIn('[data-test-var-value]', 'pal');
+      await click('[data-test-submit-var]');
+
+      await click('button[data-test-overwrite-button]');
+      assert.equal(
+        currentURL(),
+        '/variables/var/Auto-conflicting Variable@default',
+        'Selecting overwrite forces a save and redirects'
+      );
+
+      assert
+        .dom('.flash-message.alert.alert-success')
+        .exists('Shows a success toast notification on edit.');
+
+      assert
+        .dom('[data-test-var=buddy]')
+        .exists('The edited variable key should appear in the list.');
+
+      // Reset Token
+      window.localStorage.nomadTokenSecret = null;
+    });
   });
 
   module('delete flow', function () {
@@ -626,8 +663,8 @@ module('Acceptance | secure variables', function (hooks) {
       assert
         .dom('[data-test-file-row]:not(.inaccessible)')
         .exists(
-          { count: 3 },
-          'Shows 3 variable files, none of which are inaccessible'
+          { count: 4 },
+          'Shows 4 variable files, none of which are inaccessible'
         );
 
       await click('[data-test-file-row]');
@@ -646,8 +683,8 @@ module('Acceptance | secure variables', function (hooks) {
       assert
         .dom('[data-test-file-row].inaccessible')
         .exists(
-          { count: 3 },
-          'Shows 3 variable files, all of which are inaccessible'
+          { count: 4 },
+          'Shows 4 variable files, all of which are inaccessible'
         );
 
       // Reset Token

ui/tests/pages/variables.js

@@ -3,4 +3,7 @@ import { create, visitable } from 'ember-cli-page-object';
 export default create({
   visit: visitable('/variables'),
   visitNew: visitable('/variables/new'),
+  visitConflicting: visitable(
+    '/variables/var/Auto-conflicting%20Variable@default/edit'
+  ),
 });