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

Fix bundle (re-)loading #22013

Merged
merged 13 commits into from May 7, 2024
Merged

Conversation

hanneskuettner
Copy link
Contributor

@hanneskuettner hanneskuettner commented Mar 28, 2024

Scope

What's changed:

  • Instead of skipping over already registered bundle extensions, update the directus_extensions bundle entry records when loading a bundle extension, by removing stale ones and adding new ones
  • Instead of using database.insert I'm now using the extensionsItemService.insert, was there any reason it was not used before?

Potential Risks / Drawbacks

  • This approach removes bundle extension entries that are no longer present, potentially losing the extension enabled state. In practice this should not matter, however.

Review Notes / Questions

  • I would like Pascal to review this since he initially wrote this and might have some thoughts on it.

Fixes #21942, fixes #21946, fixes #21922

Copy link

changeset-bot bot commented Mar 28, 2024

🦋 Changeset detected

Latest commit: 70bc671

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@paescuj paescuj self-assigned this Mar 28, 2024
@paescuj paescuj changed the title Fix bundle (re-)loading (fixes #21942 and #21946) Fix bundle (re-)loading Mar 31, 2024
@hanneskuettner
Copy link
Contributor Author

Note on the potential fixing of #21758:

Currently this PR only inserts missing module entries and removes DB records of entries that were removed from the bundle (matched based on the bundle ID + folder / entry name). If we want, we can extend the scope of this PR to instead upsert all entries, since we're touching on it anyways.

@br41nslug br41nslug assigned br41nslug and unassigned paescuj May 7, 2024
Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

As already discussed, I think we can consider #21758 fixed with this PR as the missing bundle entries should be added now.

.changeset/hot-dolphins-shop.md Outdated Show resolved Hide resolved
api/src/extensions/lib/get-extensions-settings.ts Outdated Show resolved Hide resolved
api/src/extensions/lib/get-extensions-settings.ts Outdated Show resolved Hide resolved
Copy link
Member

@br41nslug br41nslug left a comment

Choose a reason for hiding this comment

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

  • successfully updates the database but doesnt actually load the newly named endpoint either on auto-reload or on a manual restart (endpoint names get compiled into the bundle unlike regular endpoints, so missed a rebuild in that specific test)

@hanneskuettner
Copy link
Contributor Author

successfully updates the database but doesnt actually load the newly named endpoint either on auto-reload or on a manual restart

Interesting find. I'll have a look

Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Great work 🎉

@br41nslug br41nslug enabled auto-merge (squash) May 7, 2024 14:41
@br41nslug br41nslug merged commit 1d7e0b7 into directus:main May 7, 2024
4 checks passed
@github-actions github-actions bot added this to the Next Release milestone May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment