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

hellobontempo · 2024-10-12T00:04:16Z

Description

This PR un-extends the store service and moves lazyPagintedQuery methods to Pagination service to prepare for upgrading to "ember-data": "~5.3.2"

When initially upgrading ember-data and @ember-data/legacy-compat to 5.3.2 we received this error:

router.js:1129 Error while processing route: vault.cluster.index this.store.adapterFor is not a function TypeError: this.store.adapterFor is not a function
    at VersionService.getType (http://localhost:4200/ui/assets/vault.js:64936:41)
    at getType.next (<anonymous>)
  
ember-environment.js:21 Uncaught TypeError: this.store.adapterFor is not a function
    at VersionService.getType (version.js:74:1)
    at getType.next (<anonymous>)
    at GeneratorState.step (generator-state.js:19:53)
    at TaskInstanceExecutor.generatorStep (executor.js:284:42)
    at TaskInstanceExecutor.handleResolvedContinueValue (executor.js:152:27)
    at TaskInstanceExecutor.proceedSync (executor.js:116:12)
    at TaskInstanceExecutor.start (executor.js:54:10)
    at TaskInstance.start (base.js:57:19)
    at refresh.js:27:67
    at Array.forEach (<anonymous>)

Though it's unclear what exactly was causing this error, it was clear the store was failing to compile properly and seemed to trace back to the createCache hook.

Deleting our store.js file fixed the build failure and so after some discussion we decided maintaining our custom store extension was become too costly -- every upgrade cycle it required more and more bandaids (#25272, #22838)

We were just extending the store for lazyPaginatedQuery which manages client-side pagination so we opted to move these methods to a custom Pagination service. Although this functionality would probably be better as a decorator (thank you @Monkeychip for the research and suggestion), that refactor is out of scope for this work. (It requires updating all outdated class syntax to use native javascript classes). For now, moving the methods to a separate service was the most feasible, elegant solution. In #28690, addressed some minor cleanup to the store service so updates here would be strictly un-extending the store service and upgrading ember data.

TODO only if you're a HashiCorp employee

Backport Labels: If this PR is in the ENT repo and needs to be backported, backport
to N, N-1, and N-2, using the backport/ent/x.x.x+ent labels. If this PR is in the CE repo, you should only backport to N, using the backport/x.x.x label, not the enterprise labels.
- If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
Jira: If this change has an associated Jira, it's referenced either
in the PR description, commit message, or branch name.
RFC: If this change has an associated RFC, please link it in the description.
ENT PR: If this change has an associated ENT PR, please link it in the
description. Also, make sure the changelog is in this PR, not in your ENT PR.

github-actions · 2024-10-12T00:07:45Z

Build Results:
All builds succeeded! ✅

github-actions · 2024-10-12T00:27:04Z

CI Results:
All Go tests succeeded! ✅

hellobontempo · 2024-10-14T17:24:12Z

✅ enterprise tests

hellobontempo · 2024-10-14T18:09:30Z

ui/app/services/pagination.js

-    this.requestManager.use([LegacyNetworkHandler]);
-    this.requestManager.useCache(CacheHandler);
-  }
+export default class Pagination extends Service {


Name here seemed intuitive, but open to alternatives!

hellobontempo · 2024-10-14T18:12:23Z

ui/app/services/pagination.js

@@ -83,7 +73,7 @@ export default class StoreService extends Store {
    const skipCache = query.skipCache;
    // We don't want skipCache to be part of the actual query key, so remove it
    delete query.skipCache;
-    const adapter = this.adapterFor(modelType);
+    const adapter = this.store.adapterFor(modelType);


I tried to leave this file alone as much as possible because lazyPaginatedQuery is so widely used and really just updated this to this.store. I'm sure there's lots of cleanup that could be done here but I didn't want to scope creep too much

hellobontempo · 2024-10-14T18:41:38Z

ui/app/routes/vault/cluster/access/identity/aliases/index.js

@@ -38,12 +38,12 @@ export default Route.extend(ListRoute, {
    willTransition(transition) {
      window.scrollTo(0, 0);
      if (!transition || transition.targetName !== this.routeName) {
-        this.store.clearDataset();
+        this.pagination.clearDataset();


I feel like this reads nicely because it's clear we're manipulating the store with a custom method here (and not calling a built-in store method)

hellobontempo · 2024-10-14T18:42:36Z

ui/lib/kmip/addon/engine.js

-  modulePrefix,
-  Resolver,
-  dependencies: {
+export default class KmipEngine extends Engine {


Not necessary, but I thought updating this syntax was low lift and quick win removing leaky state potential

hellobontempo · 2024-10-14T18:46:26Z

ui/types/vault/services/store.d.ts

@@ -3,16 +3,11 @@
 * SPDX-License-Identifier: BUSL-1.1
 */

-import Store, { RecordArray } from '@ember-data/store';
+import Store from '@ember-data/store';


At some point we should be able to remove this declaration file and use https://www.npmjs.com/package/@types/ember-data instead, but when I attempted in this PR it didn't work out out of the box so I abandoned for now. (Especially since EmberData 5.4.0-alpha.35 is published with alpha-level support for typescript, so we may be able to avoid installing another dependency all together)

hellobontempo · 2024-10-14T18:50:10Z

ui/lib/config-ui/addon/components/messages/page/details.js


  @action
  async deleteMessage() {
    try {
-      this.store.clearDataset('config-ui/message');


typically we call clearDataset after deleting so I don't think this was previously working as expected - went ahead and moved method

hellobontempo · 2024-10-14T18:50:17Z

ui/lib/config-ui/addon/components/messages/page/list.js

@@ -90,8 +90,8 @@ export default class MessagesList extends Component {
  @task
  *deleteMessage(message) {
    try {
-      this.store.clearDataset('config-ui/message');


same thing here

Monkeychip · 2024-10-14T20:11:40Z

ui/app/services/pagination.js


  lazyCaches = new Map();

  setLazyCacheForModel(modelName, key, value) {
    const cacheKey = keyForCache(key);
    const cache = this.lazyCacheForModel(modelName) || new Map();
    cache.set(cacheKey, value);
-    const modelKey = normalizeModelName(modelName);


we did some funny things. Nice clean up

Monkeychip · 2024-10-14T20:17:23Z

ui/lib/pki/addon/routes/certificates/index.js

  @service secretMountPath;
+  @service store; // used by @withConfig decorator


thanks for the comment

I wonder if the withConfig decorator should inject the store instead if it needs it?

yep - I almost made that change and can't remember why I didn't 🤔 I think to keep the scope small...because I wanted to change both config decorators and cleanup unneeded store injections

Monkeychip

Smoke tested (I created 1000) secrets and ran through various areas that code was changed. I really like the name change if anything—much easier to read.

zofskeez

Nice work! Since everything required to make lazyPaginatedQuery function can be achieved by using the public methods on the store service, this seems like a great update to me. 🎉

zofskeez · 2024-10-15T19:28:20Z

ui/lib/pki/addon/routes/certificates/index.js

  @service secretMountPath;
+  @service store; // used by @withConfig decorator


I wonder if the withConfig decorator should inject the store instead if it needs it?

hellobontempo added 12 commits October 11, 2024 14:26

rename store to pagination, remove store extension

baacdc7

initial update of service test

6536edb

remove superfluous helper

0ca7a8d

replace store with pagination service in main app

21b825b

update kmip engine syntax

2ce7386

add pagination to kmip engine

34de9ab

update to pagination in config-ui engine

6298a9e

update sync engine to use pagination service

4e40fd2

use pagination service in kv engine

cdefe0a

use pagination service in ldap engine

ed70218

use pagination in pki engine

bae50ec

update renaming clearDataset functions

2eee35f

hellobontempo added ui pr/no-changelog labels Oct 12, 2024

hellobontempo added this to the 1.19.0-rc milestone Oct 12, 2024

hellobontempo requested a review from a team as a code owner October 12, 2024 00:04

hellobontempo mentioned this pull request Oct 12, 2024

UI/attempt removing manual store #28662

Closed

6 tasks

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Oct 12, 2024

hellobontempo added 2 commits October 14, 2024 10:32

link to jira VAULT-31721

f2df545

remove comment

e14b1d2

hellobontempo commented Oct 14, 2024

View reviewed changes

hellobontempo changed the title ~~UI: Use custom service instead of extending ember-data store~~ UI: Ember-data upgrade 5.3.2: use custom service instead of extending ember-data store Oct 14, 2024

hellobontempo commented Oct 14, 2024

View reviewed changes

hellobontempo changed the title ~~UI: Ember-data upgrade 5.3.2: use custom service instead of extending ember-data store~~ UI: Ember-data upgrade 5.3.2 prep: use custom service instead of extending ember-data store Oct 14, 2024

hellobontempo commented Oct 14, 2024

View reviewed changes

Monkeychip reviewed Oct 14, 2024

View reviewed changes

Monkeychip approved these changes Oct 14, 2024

View reviewed changes

zofskeez reviewed Oct 15, 2024

View reviewed changes

Merge branch 'main' into ui/remove-manual-store-extension

95c880a

vercel bot deployed to Preview October 17, 2024 16:43 View deployment

hellobontempo merged commit 1fbbf9d into main Oct 17, 2024
32 checks passed

hellobontempo deleted the ui/remove-manual-store-extension branch October 17, 2024 17:01

hellobontempo mentioned this pull request Nov 6, 2024

Backport 1.18.x: UI LDAP Hierarchical roles #28842

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hellobontempo commented Oct 12, 2024 •

edited

Loading

github-actions bot commented Oct 12, 2024

github-actions bot commented Oct 12, 2024 •

edited

Loading

hellobontempo commented Oct 14, 2024

hellobontempo Oct 14, 2024

Monkeychip Oct 14, 2024

hellobontempo Oct 14, 2024

hellobontempo Oct 14, 2024

hellobontempo Oct 14, 2024

hellobontempo Oct 14, 2024

hellobontempo Oct 14, 2024

hellobontempo Oct 14, 2024

Monkeychip Oct 14, 2024

Monkeychip Oct 14, 2024

zofskeez Oct 15, 2024

hellobontempo Oct 15, 2024

Monkeychip left a comment

zofskeez left a comment

zofskeez Oct 15, 2024

		@service secretMountPath;
		@service store; // used by @withConfig decorator

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

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

Conversation

hellobontempo commented Oct 12, 2024 • edited Loading

Description

TODO only if you're a HashiCorp employee

github-actions bot commented Oct 12, 2024

github-actions bot commented Oct 12, 2024 • edited Loading

hellobontempo commented Oct 14, 2024

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

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

zofskeez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellobontempo commented Oct 12, 2024 •

edited

Loading

github-actions bot commented Oct 12, 2024 •

edited

Loading