Skip to content

Add multitenant support to allow keeping tenant specific data separated for the cached models. #81

New issue

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

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

Already on GitHub? # to your account

Closed
wants to merge 5 commits into from

Conversation

lucian-dragomir
Copy link

This is my proposal of adding multi-tenant support to the cached models by implementing at model level (in a base model class or using a trait) a new method getCachePrefix that can be used to differentiate cache tags and keys by prefixing them with the tenant context (in the provided example the method is returning the connection name concatenated with the tenant database name).

	$eagerLoad = $this->eagerLoad ?? [];
	$model = $this->model ?? $this;
	$query = $this->query ?? app(Builder::class);
  with
	$eagerLoad = $this->retrieveEagerLoad();
	$model = $this->retrieveCacheModel();
	$query = $this->retrieveCacheQuery();
  to avoid name collisions in case the model contains one of the 'eagerLoad', 'mode' or 'query' attributes

- added makeCachePrefix and used it to prefix cache tags and keyDifferentiator and class name tag with the value returned by the 'getCachePrefix' method of the model
  This method can be used in a multitenant implementation where an object with the same key and tags is cached for each tenant database connection.

- added CacheGlobal class to be a placeholder for the global static cache disable flag. The cached value using $cache might generate unpredictable behaviour in case multiple requests are made to the server in the same time.
@coveralls
Copy link

coveralls commented Feb 18, 2018

Coverage Status

Coverage decreased (-5.2%) to 94.34% when pulling 50c542d on lucian-dragomir:master into 403dc41 on GeneaLabs:master.

@mikebronner
Copy link
Owner

mikebronner commented Feb 18, 2018

I would recommend creating individual redis connections for each tenant, and handle it via configuration, instead of implementing it in the code. I might consider addressing this when I refactor the way keys are generated. Keep working on this by all means, and I will take it into consideration when adding that functionality.

@mikebronner
Copy link
Owner

Sorry, didn't mean to close it.

@mikebronner
Copy link
Owner

I've been reading through you proposed changes some more, and feel this PR contains too many different changes (this that are not necessarily related to just implementing cache prefixing, but other refactoring as well). Would you mind stripping this PR down to only cache prefixing without any of the other refactoring, if possible? I don't necessarily want to merge all those changes.
If that's a problem, no worry, I'll implement cache prefixing soon.

@mikebronner
Copy link
Owner

Actually, sorry to flip-flop on this so much. I will be implementing cache key prefixes now, but not using this PR, as I don't want to incorporate everything it has. Thanks for submitting this, though. Sorry for not accepting your PR, hopefully next time! :)

@mikebronner
Copy link
Owner

Hi @lucian-dragomir this should be fixed in version 0.3.31. Please let me know how that works for you.

@lucian-dragomir
Copy link
Author

Hi @mikebronner,
I read your recommendation to use different cache connection for each tenant and I think it is the correct and safe approach.
Thank you also for the cache prefix feature!

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

Successfully merging this pull request may close these issues.

3 participants