UI: Fix empty item on kv list #22838

hashishaw · 2023-09-06T23:04:13Z

There is a bug in our version of ember-data (4.11.x) where after editing a record, going to the list page would result in a ghost record because unloadAll wasn't unloading everything properly. As part of the investigation I found a couple other issues that I cleaned up as well.

Replication steps for initial issue:

Have a few secrets created in a KV v2 engine
Go to a secret detail
Click "Metadata" tab
Edit metadata, add custom metadata
Save
Click the breadcrumb to go back to the secrets list
Ghost item exists on the list

We decided not to upgrade to ember-data@4.12 at this time (the version which has a fix), because after upgrade other tests were failing with expected stable identifier issues, and the upgrade requires store configuration which we didn't want to rush to get in before code freeze without adequate testing.

Monkeychip · 2023-09-07T21:40:32Z

ui/app/services/store.js

@@ -138,13 +138,23 @@ export default class StoreService extends Store {
    return resp;
  }

+  forceUnload(modelName) {


interesting find!

Monkeychip · 2023-09-07T21:40:47Z

ui/lib/kv/addon/routes/list-directory.js

@@ -46,9 +46,15 @@ export default class KvSecretsListRoute extends Route {
      });
  }

+  getPathToSecret(pathParam) {


much cleaner, thank you!

ui/app/services/store.js

hashishaw · 2023-09-08T20:58:13Z

ui/app/components/secret-delete-menu.js

@@ -24,92 +22,21 @@ export default class SecretDeleteMenu extends Component {

  @tracked showDeleteModal = false;

-  @maybeQueryRecord(


When navigating from the details page back to the list in kv v1, the console was erroring with
Cannot create a new tag for `<model::capabilities:undefined>` after it has been destroyed
Finally tracked it down to these not tearing down gracefully, so I moved the capabilities to the model instead which is our latest pattern

It looks like the secret-edit component also uses maybeQueryRecord so something to be aware of if we notice that error further. Interesting that it's also used in lazy-capabilities which is used a bunch but maybe it's ok when used on a model since they aren't torn down as frequently as components.

hashishaw · 2023-09-08T20:58:46Z

ui/app/components/secret-delete-menu.js

-  )
-  secretSoftDataPath;
-  @alias('secretSoftDataPath.canUpdate') canSoftDeleteSecretData;
+  get canUndeleteVersion() {


This component should actually go away soon, so to simplify the changeset I just made these all getters of the same name

github-actions · 2023-09-08T21:16:58Z

Build Results:
All builds succeeded! ✅

zofskeez

Great working grinding this one out and finding a work around 👏. I left a few comments and questions, nothing blocking.

zofskeez · 2023-09-12T15:46:25Z

ui/app/components/secret-delete-menu.js

@@ -24,92 +22,21 @@ export default class SecretDeleteMenu extends Component {

  @tracked showDeleteModal = false;

-  @maybeQueryRecord(


It looks like the secret-edit component also uses maybeQueryRecord so something to be aware of if we notice that error further. Interesting that it's also used in lazy-capabilities which is used a bunch but maybe it's ok when used on a model since they aren't torn down as frequently as components.

zofskeez · 2023-09-12T15:48:24Z

ui/app/services/store.js

-    this.unloadAll(modelName);
+    this.forceUnload(modelName);
+    // Hack to ensure the pushed records below all get in the store
+    this.peekAll(modelName).length;


I'm guessing this is necessary but since there is already a peek in the forceUnload method it has to be done again after unloadAll?

Yeah, I just sprinkled them around because I saw lots of test failures when I tried to minimize these. Hoping we can remove after upgrade.

zofskeez · 2023-09-12T15:52:00Z

ui/app/services/store.js

Just a thought but I wonder if someone else ends up doing the next upgrade if it will be clear what can be removed as part of these workarounds? Maybe some extra annotations like remove after upgrade to Ember Data 4.12 would be helpful so we don't have extra peeks etc. hanging around.

Good idea! Added comments about removing once we upgrade to 4.12

zofskeez · 2023-09-12T15:52:52Z

ui/tests/acceptance/secrets/backend/kv/kv-v2-workflow-edge-cases-test.js

+  test('no ghost item after editing metadata', async function (assert) {
+    await visit(`/vault/secrets/${this.backend}/kv/edge/directory`);
+    assert.dom(PAGE.list.item()).exists({ count: 2 }, 'two secrets are listed');
+    await click(PAGE.list.item('two'));
+    await click(PAGE.secretTab('Metadata'));
+    await click(PAGE.metadata.editBtn);
+    await fillIn(FORM.keyInput(), 'foo');
+    await fillIn(FORM.valueInput(), 'bar');
+    await click(FORM.saveBtn);
+    await click(PAGE.breadcrumbAtIdx(2));
+    assert.dom(PAGE.list.item()).exists({ count: 2 }, 'two secrets are listed');
+  });


ui/tests/unit/unload-test.js

Monkeychip

This looks great to me! This PR touches some of the KV removal work that's still hanging in the wind. When this is merged I'll go ahead and update that sidebranch.

hashishaw added 2 commits September 6, 2023 17:49

Fix ghost item in kv list

d71c075

Safeguard against non-trailing slash on list-directory param

d9e7289

hashishaw added ui backport/1.15.x labels Sep 6, 2023

hashishaw added this to the 1.15 milestone Sep 6, 2023

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Sep 6, 2023

weird hack, works on build

07ef805

hashishaw added the pr/no-changelog label Sep 7, 2023

Monkeychip reviewed Sep 7, 2023

View reviewed changes

zofskeez reviewed Sep 8, 2023

View reviewed changes

ui/app/services/store.js Outdated Show resolved Hide resolved

ui/app/services/store.js Show resolved Hide resolved

hashishaw added 5 commits September 8, 2023 09:14

address comments

6b43990

Add test coverage for edge case

1cf87b0

Fix list sometimes not updating from latest API response

5f2d8d0

Move capabilities from component to model

66e2036

remove logs

d2dd7c1

hashishaw commented Sep 8, 2023

View reviewed changes

hashishaw marked this pull request as ready for review September 8, 2023 20:59

Merge branch 'main' into ui/fix-empty-kv-list-item

25d52f0

vercel bot deployed to Preview September 8, 2023 21:06 View deployment

Merge branch 'main' into ui/fix-empty-kv-list-item

da90d34

vercel bot deployed to Preview September 11, 2023 15:03 View deployment

hashishaw modified the milestones: 1.15.0-rc1, 1.15.0 Sep 11, 2023

hashishaw and others added 4 commits September 12, 2023 09:11

Add peekAll to store

f437ba9

Add unloadAll test

4f57e9d

Fix delete capability check for v1

436733f

Merge branch 'main' into ui/fix-empty-kv-list-item

2672f22

vercel bot deployed to Preview September 12, 2023 15:20 View deployment

Merge branch 'main' into ui/fix-empty-kv-list-item

3ba7eb0

vercel bot deployed to Preview September 12, 2023 15:45 View deployment

zofskeez approved these changes Sep 12, 2023

View reviewed changes

Monkeychip approved these changes Sep 12, 2023

View reviewed changes

hashishaw and others added 2 commits September 12, 2023 11:44

Address pr comments

66834b4

Merge branch 'main' into ui/fix-empty-kv-list-item

45727a5

hashishaw enabled auto-merge (squash) September 12, 2023 16:45

hashishaw merged commit de1382e into main Sep 12, 2023
69 checks passed

hashishaw deleted the ui/fix-empty-kv-list-item branch September 12, 2023 17:01

hc-github-team-secure-vault-core mentioned this pull request Sep 12, 2023

Backport of UI: Fix empty item on kv list into release/1.15.x #23014

Merged

hellobontempo mentioned this pull request Oct 14, 2024

UI: Ember-data upgrade 5.3.2 prep: use custom service instead of extending ember-data store #28695

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Fix empty item on kv list #22838

UI: Fix empty item on kv list #22838

hashishaw commented Sep 6, 2023 •

edited

Loading

Monkeychip Sep 7, 2023

Monkeychip Sep 7, 2023

hashishaw Sep 8, 2023 •

edited

Loading

zofskeez Sep 12, 2023

hashishaw Sep 8, 2023

github-actions bot commented Sep 8, 2023

zofskeez left a comment

zofskeez Sep 12, 2023

zofskeez Sep 12, 2023

hashishaw Sep 12, 2023

zofskeez Sep 12, 2023

hashishaw Sep 12, 2023

zofskeez Sep 12, 2023

Monkeychip left a comment

		@@ -24,92 +22,21 @@ export default class SecretDeleteMenu extends Component {

		@tracked showDeleteModal = false;

		@maybeQueryRecord(

UI: Fix empty item on kv list #22838

UI: Fix empty item on kv list #22838

Conversation

hashishaw commented Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashishaw Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 8, 2023

zofskeez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monkeychip left a comment

Choose a reason for hiding this comment

hashishaw commented Sep 6, 2023 •

edited

Loading

hashishaw Sep 8, 2023 •

edited

Loading