Skip to content

fix!: Remove caching of services from container #2

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Bl00D4NGEL
Copy link

@Bl00D4NGEL Bl00D4NGEL commented Dec 22, 2023

While using the container facade in the HasPolicies trait of the user-policy-bundle I noticed that the facade would return a cached instance of the PolicyRegistry which included a stale(?) reference to a ManagerRegistry while running our integration tests. This seems to have been caused by the cached dependencies of the Facade class.

In order to fix this there were two approaches to this:

  1. Remove the caching of dependencies
  2. Add a function to check whether a facade class is cachable

Option 1 could have backwards compatibility breaking and implicit while Option 2 is more explicit and definitely keeps BC.

After talking and fiddling around with @janvt we came to the conclusion that it makes sense to get rid of the caching and instead only check for "swapped instances".

This change should result in a major version bump as a few methods are removed and the caching behaviour could've been interpreted as intended behaviour instead.

@Bl00D4NGEL Bl00D4NGEL requested a review from b00gizm as a code owner December 22, 2023 07:48
Bl00D4NGEL added 2 commits December 22, 2023 11:02

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
@Bl00D4NGEL Bl00D4NGEL changed the title feat: add option to disable caching Change: Remove caching of services from container Dec 22, 2023
@janvt janvt changed the title Change: Remove caching of services from container fix!: Remove caching of services from container Dec 22, 2023
@janvt
Copy link

janvt commented Dec 22, 2023

Updated title so rls-pls knows to do a v2.

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

Successfully merging this pull request may close these issues.

None yet

2 participants