Skip to content

support multiple connections #13

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 1 commit into
base: master
Choose a base branch
from

Conversation

IvanSlyadnev
Copy link

No description provided.

@antonkomarev
Copy link
Member

@IvanSlyadnev thank you for the contribution! Could you add an example in the description?

@IvanSlyadnev
Copy link
Author

@IvanSlyadnev thank you for the contribution! Could you add an example in the description?

Good afternoon! I have a non-trivial situation in my project. There is a so-called "main database," which contains a table of companies. When each company is created, a separate database is created for it. That is, the structure of the databases is identical, but the data is different for each company.

A need arose to add ClickHouse to the project, and I decided to use your package for ClickHouse migrations. However, the problem was that when using $this->app->singleton in src/ClickhouseServiceProvider.php, the ClickHouse client, which establishes a connection to the database, did so only once—which is obvious from the method name singleton. As a result, migrations were only executed for the first database and not for the others.

After I rewrote it and replaced $this->app->singleton with $this->app->bind, the client successfully established connections for each database within the same runtime and executed migrations for all databases accordingly.

@antonkomarev
Copy link
Member

antonkomarev commented Jun 15, 2025

@IvanSlyadnev are you sure it will be enough to make changes for Migrator only? It resolves ClickHouseDB\Client from the container, which is singletone too.

How are you switching between tenants?

@antonkomarev
Copy link
Member

Maybe you need a new console command in your application, which will instantiate Migrator manually and iterate over the tenants. And you will be able to define exact one tenant to migrate. Then you will have much more flexibility.

@antonkomarev
Copy link
Member

antonkomarev commented Jun 15, 2025

Something like this:

<?php

/*
 * This file is part of Laravel ClickHouse.
 *
 * (c) Anton Komarev <anton@komarev.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

declare(strict_types=1);

namespace Cog\Laravel\Clickhouse\ConsoleCommand;

use ClickHouseDB\Client as ClickhouseClient;
use Cog\Laravel\Clickhouse\Migration\MigrationRepository;
use Cog\Laravel\Clickhouse\Migration\Migrator;
use Illuminate\Console\Command;
use Illuminate\Console\ConfirmableTrait;
use Illuminate\Contracts\Config\Repository as AppConfigRepositoryInterface;
use Illuminate\Filesystem\Filesystem;
use Symfony\Component\Console\Attribute\AsCommand;

#[AsCommand(
    name: 'clickhouse:migrate',
    description: 'Run the ClickHouse database migrations',
)]
final class ClickhouseMigrateCommand extends Command
{
    use ConfirmableTrait;

    protected static $defaultName = 'clickhouse:migrate';

    /**
     * {@inheritdoc}
     */
    protected $signature = 'clickhouse:migrate
                {--force : Force the operation to run when in production}
                {--path= : Path to Clickhouse directory with migrations}
                {--realpath : Indicate any provided migration file paths are pre-resolved absolute paths}
                {--step= : Number of migrations to run}
                {--tenant-id= : Tenant ID}';

    /**
     * {@inheritdoc}
     */
    protected $description = 'Run the ClickHouse database migrations';

    public function handle(
        ClickhouseClient $clickhouseClient,
        AppConfigRepositoryInterface $appConfigRepository,
        Filesystem $filesystem
    ): int {
        if (!$this->confirmToProceed()) {
            return self::FAILURE;
        }

        $tenantId = $this->option('tenant-id');

        $tenants = Tenant::query()
            ->when($tenantId !== null, function ($query) use ($tenantId) {
                $query->where('id', $tenantId);
            })
            ->get();

        foreach ($tenants as $tenant) {
            $clickhouseClient->database($tenant->getDatabaseName());

            $migrator = $this->resolveMigrator(
                $clickhouseClient,
                $appConfigRepository,
                $filesystem,
            );

            $migrator->ensureTableExists();

            $migrator->runUp(
                $this->getMigrationsDirectoryPath(),
                $this->getOutput(),
                $this->getStep(),
            );
        }

        return self::SUCCESS;
    }

    private function getStep(): int
    {
        return intval($this->option('step'));
    }

    private function getMigrationsDirectoryPath(): string
    {
        $appConfigRepository = $this->laravel->get(AppConfigRepositoryInterface::class);

        return $appConfigRepository->get(
            'clickhouse.migrations.path',
        );
    }
    
    private function resolveMigrator(
        ClickhouseClient $clickhouseClient,
        AppConfigRepositoryInterface $appConfigRepository,
        Filesystem $filesystem
    ): Migrator {
        $repository = new MigrationRepository(
            $clickhouseClient,
            $appConfigRepository->get('clickhouse.migrations.table'),
        );

        return new Migrator(
            $clickhouseClient,
            $repository,
            $filesystem,
        );
    }
}

I haven't tested it, just pseudo-code.

I'm not against merging this PR, but only if there is no need to make Client not singleton after it.

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.

2 participants