Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 🎸 Add migration check to delete index DB when updating #2241

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ZedLi
Copy link
Collaborator

@ZedLi ZedLi commented Apr 12, 2024

βœ… Closes: https://hashicorp.atlassian.net/browse/ICU-13223

🎟️ Jira ticket

Description

This PR aims to prevent any migration that might be needed in the future for indexed DB by destroying the previous DB if a version is detected.

There seems to some weird async behavior happening in the setup when we have to await some dexie methods.

Screenshots (if appropriate):

How to Test

Increment the version of index DB and confirm it correctly deletes previous tables and sets up a fresh instance.

Checklist:

  • [ ] I have added before and after screenshots for UI changes
  • [ ] I have added JSON response output for API changes
  • [ ] I have added steps to reproduce and test for bug fixes in the description
  • I have commented on my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • [ ] I have added tests that prove my fix is effective or that my feature works

@ZedLi ZedLi self-assigned this Apr 12, 2024
Copy link

vercel bot commented Apr 12, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
boundary-ui βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Sep 25, 2024 4:02pm
boundary-ui-desktop βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Sep 25, 2024 4:02pm

get db() {
return this.#db;
}

setup(dbName) {
async setup(dbName) {
Copy link
Collaborator Author

@ZedLi ZedLi Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before:
image

After:
image

It seems on initial login (not from an authentication restoration) some requests are firing off before index db is properly setup.

I was suspicious that the session service I override here can't actually be async since it seems the source code doesn't take into account if the overridable method needs to be async. The setup call in the service should be awaited but it didn't seem to make a difference.

I tried setting up indexed DB in our authenticate method before any transition but it seems like for some reason ember still fires off requests for scopes before index DB is ready.

I'm not sure what the solution is as it seems the moment the setup method is async, this scope request isn't properly awaiting now.

@hashicc
Copy link
Collaborator

hashicc commented Jan 24, 2025

Some notes related to this comment in the pr where there's a race condition between getting an indexeddb setup and then making calls that are use the db for caches.

The goal is to deterministically await an indexeddb setup before making calls with IndexedDbHandler#request. From reading the code and @ZedLi's comment, I think the issue specifically arises here since as @ZedLi already mentioned even if async was added to handleAuthentication and the setup was awaited, there's nothing in ember-simple-auth awaiting the handleAuthentication hook itself to finish. Although, their docs do use an async handleAuthentication example (I think they just leave that promise hanging? πŸ€”).

Here are some possible solutions toe ensure that the setup is awaited by the time IndexedDbHandler#request is called. I think the first example is probably the way to go.

1. Instead of routing ourselves, route within handleAuthentication

Described in this comment on the ember-simple-auth repo. Also removing this.router.transitionTo('index'); here and letting that be taken care of in the hook.

Within the hook we could either not call super and transition ourselves or call super with the route we want to go to, it does some extra work with routeAfterAuthentication here. It seems like the intention of routeAfterAuthentication is to default to using the value from configuration but I can't find current documentation on the way that should be done so using the super call explicitly might be more clear.

2. Setup promise state stashed on IndexedDb

One way this could be solved is having the IndexedDb service have some sort of promise stashed on it that is awaited before making calls in the handler
// in IndexedDbHandler#request
await this.indexedDb.isSetup // is a promise that resolves when setup is finished
const { db: indexedDb } = this.indexedDb ?? {};
That's kind of ugly since it forces the await in the request.

3. Chase all root transitionTo and add await indexeddb.setup() before

The other thing is that the setup could be pushed to right after this.session.authenticate succeeds but before this.router.transitionTo('index');. I'm not sure about the OIDC case though, when it just gets redirected, is the session authenticated before the indexeddb would setup is called in the application route? Sprinkling the setup calls (maybe only in a few spots) before the transitions is not great because it's a bit hard to track.

4. Chase all root model hooks and include a beforeModel with await indexeddb.setup()

Find all routes that would make a call to IndexedDbHandler#request in routes and making sure that await indexeddb.setup() is called first. This ends up being boilerplate that will probably be forgotten about.

5. authenticated or protected "invisible" route

  • "Invisible" because it is not part of the "url" but is part of the route call order
  • This acts as route guard to ensure setup for any child routes in a beforeModel, similar to what is above but is enforced by ember
// router.js
// top-level route layer that "hosts" all authenticated child routes
this.route('authenticated', { resetNamespace: true }, function () {
  // any routes that need "authenticated" `beforeModel` checks and are expected to be authenticated
  // move all existing child routes into here
});

From a URL level nothing changes but from a route name change this can be annoying, all migrated routes that are referenced by route name now need to be added with an authenticated: transitionTo("account.change-password") becomes transitionTo("authenticated.account-change-password")

class AuthenticatedRoute {
	async beforeModel() {
		// Using the `AuthenticatedRoute` removes the need to have this
		// sprinkled across the application since it's a pre-req for
		// entering this layer of routes and its child routes
		if (!this.session.isAuthenticated) this.router.transitionTo('index');

		// enforce that this is setup before entering child routes
		await indexeddb.setup();
	}
}

I believe once you're in this layer it's hooks don't get called again unless something like ApplicationRoute.refresh() gets called and recalls its hooks and all child route's hooks recursively.

@hashicc
Copy link
Collaborator

hashicc commented Jan 24, 2025

Here's a branch based on suggestion 1 above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants