orchestral / tenanti

[Package] Multi-tenant Database Schema Manager for Laravel
MIT License
588 stars 52 forks source link

Support uuid as tenant database id #50

Closed nguyenhaiphan closed 7 years ago

nguyenhaiphan commented 7 years ago

I see that your implementation bind the database with Entity Id, which easily be conflicted after a reset migration. I propose to implement the uuid method instead, which can be randomize uniquely, or an option to connection methods (asConnection) on Orchestra\Tenanti\Migrator\Operation trait so that developer can by pass their own database name, instead of restrictedly following the '_{id}' pattern.

crynobone commented 7 years ago

The second parameter is the model instance, so you can handle this on your own.

nguyenhaiphan commented 7 years ago

Thank @crynobone for your quick response.

Sorry for not clearly describe my situation. I'm using the multi database mode. The tenant table is the company that my clients (user table) belong to. Each user belongTo only one company.

As far as I know, I intend to handle the database connection via middleware, such as:

    public function handle($request, Closure $next)
    {
        $user = Auth::user();
        Tenanti::driver('company')->asDefaultConnection($user->company,'tenants_{id}'));
...

I can't see your mentioned model instance in that implementation.

crynobone commented 7 years ago

Dig into the code, bindWithKey already handle everything

crynobone commented 7 years ago

https://github.com/orchestral/tenanti/blob/master/src/Migrator/Operation.php#L199

nguyenhaiphan commented 7 years ago

https://github.com/orchestral/tenanti/blob/2cac5009f03d2685c2f21b62be6546f9dfea55c4/src/Migrator/Operation.php#L195

When I try to implement my own string, such as tenants_<some-uuid>, the 195th line captured it and transform to something like tenants_<some-uuid>_{id}. (shared config is set to false for multi dabase mode).

nguyenhaiphan commented 7 years ago

So I supposed the database name can have the format like : prefix_<uu-id>_{id} in the mean time, right ?

crynobone commented 7 years ago

I feel lost here, so you telling me that the database connection name need to support other than model key? the same database connection name that Laravel provide by default as sqlite, mysql?

https://github.com/orchestral/tenanti/blob/master/README.md#database-connection-resolver you can already customize the database name here. Connection name should be irrelevant.

nguyenhaiphan commented 7 years ago

Sorry,

Here is my wanting setup: I have a master database, using 'mysql' connection setting. In master database I implement Companies and Users tables, in which companies is tenants reference table. So for each company, I'd like to have a tenants{uuid} database, instead of tenants{id}, because when I migrate:refresh the master database and re seed it, the old databases could be lost. Maybe this thing can not be done now. So I decide to go back to your initial solution, which is tenants_{id} format for auto generation database name. (Following your proposal code at https://github.com/crynobone/todoist/blob/multi-database/app/Observers/User.php)

crynobone commented 7 years ago

I don't see any reason why you can't do anything such as https://github.com/crynobone/todoist/blob/multi-database/app/Observers/User.php#L43-L61

/**
 * Create database for entity.
 *
 * @param  \Illuminate\Database\Eloquent\Model  $entity
 *
 * @return mixed
 */
protected function createTenantDatabase(Model $entity)
{
    $connection = $entity->getConnection();
    $driver = $connection->getDriverName();

    switch ($driver) {
        case 'mysql':
            $query = "CREATE DATABASE `todoist_{$entity->uuid}`";
            break;
        case 'pgsql':
            $query = "CREATE DATABASE todoist_{$entity->uuid}";
            break;
        default:
            throw new InvalidArgumentException("Database Driver [{$driver}] not supported");
    }

    return $connection->unprepared($query);
}

And resolve the Database connection based on https://github.com/crynobone/todoist/blob/multi-database/app/Providers/EventServiceProvider.php#L37-L41

Tenanti::connection('tenants', function (User $entity, array $config) {
    $config['database'] = "todoist_{$entity->uuid}";

    return $config;
});
nguyenhaiphan commented 7 years ago

Thank you @crynobone , I'm using your 3.4.x@dev version. I did put the connection logic to AppServiceProvider as mentioned in the guide, and the log do triggered. But only when I manage to intercept a customized middleware as mentioned in https://github.com/orchestral/tenanti#setting-default-database-connection, the connection then goes well.

Below is my implementation:

//for AppServiceProvider.php
$CI=$this;
Tenanti::connection('tenants', function (Model $entity, array $config) use ($CI) {
  $id=($entity instanceof User)?$entity->company->id:(($entity instanceof Company)?$entity->id:1);
  $config['database'] = 'tenants_'.$id;
  Log::info($entity); 
  return $config;
});
//for TenantResolveMiddleware.php
//Company $entity;
Tenanti::driver('company')->asDefaultConnection($entity,'tenants_{id}'));

I figured that only the asDefaultConnection method can set tenants' models to automatically retrieve from tenants' tables. And the resolver Tenanti::connection only store the database configuration. If the resolver from AppServiceProvider is missing, the app will return the error that database {...} is not configured. So is this the right way? It is supposed that Tenanti::connection automatically set the database connection to the tenant config, isn't it?

nguyenhaiphan commented 7 years ago

I figured another workaround.

To change between different tenants databases and master DB (but in same connection : host, user, password), I can use the follow snippet

    \DB::disconnect(config('database.default'));
    config(['database.connections.mysql.database'=>$database_name]);

Thank for great package.