Skip to content

Commit 266b693

Browse files
theorytheory
committed
Search for clickhouse and clickhouse-client
Bare `clickhouse` is the canonical name, and `clickhouse-client` should generally be a symlink, but some packages, like the [clickhouse-client Apt package], just have `clickhouse-client`. This is a bad assumption and should not be the default. So search the path for both and return the first one found. Raise an error if neither is found. Adopt and generalize the error message previously used by the Firebird engine. In passing, update the error regular expression to match more generally. Previously it matched unixODBC-specific errors, which did not work on macOS using the [clickhouse-odbc Homebrew formula], which uses iODBC. So switch to an error message that comes only from the ClickHouse server. [clickhouse-client Apt package]: https://packages.debian.org/bullseye/clickhouse-client [clickhouse-odbc Homebrew formula]: https://formulae.brew.sh/formula/clickhouse-odbc#default
1 parent b44bb66 commit 266b693

5 files changed

Lines changed: 103 additions & 44 deletions

File tree

‎Changes‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@ Revision history for Perl extension App::Sqitch
66
replaces inline uses of `split`. This allows the tests to more
77
consistently compare arrays as arrays, though `search_events` must now
88
always parse `tags`, `requires`, and `conflicts`.
9-
- Added support for ClickHouse. It relies on the C<clickhouse> CLI and
10-
ODBC driver. Like MySQL, it uses a database for the registry schema,
11-
where the tables use the C<MergeTree> engine, and supports client
12-
certificate authentication and the ClickHouse client configuration
13-
file format.
9+
- Added support for ClickHouse. It relies on the `clickhouse` (or
10+
`clickhouse-client`) CLI and ODBC driver. Like MySQL, it uses a
11+
database for the registry schema, where the tables use the `MergeTree`
12+
engine, and supports client certificate authentication and the
13+
ClickHouse client configuration file format.
1414

1515
1.5.2 2025-04-29T15:13:35Z
1616
- Added missing German translations, thanks to @0xflotus for the PR

‎lib/App/Sqitch/Engine/clickhouse.pm‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -328,8 +328,28 @@ sub cli { @{ shift->_cli } }
328328
sub key { 'clickhouse' }
329329
sub name { 'ClickHouse' }
330330
sub driver { 'DBD::ODBC 1.59' }
331-
# XXX Search path for clickhouse-client or just clickhouse?
332-
sub default_client { 'clickhouse-client' }
331+
332+
sub default_client {
333+
my $self = shift;
334+
my $ext = App::Sqitch::ISWIN || $^O eq 'cygwin' ? '.exe' : '';
335+
336+
# Try to find the client in the path.
337+
my @names = map { $_ . $ext } 'clickhouse', 'clickhouse-client';
338+
for my $dir (File::Spec->path) {
339+
for my $try ( @names ) {
340+
my $path = file $dir, $try;
341+
# GetShortPathName returns undef for nonexistent files.
342+
$path = Win32::GetShortPathName($path) // next if App::Sqitch::ISWIN;
343+
return $try if -f $path && -x $path;
344+
}
345+
}
346+
347+
hurl clickhouse => __x(
348+
'Unable to locate {cli} client; set "engine.{eng}.client" via sqitch config',
349+
cli => 'clickhouse',
350+
eng => 'clickhouse',
351+
);
352+
}
333353

334354
sub _char2ts { $_[1]->set_time_zone('UTC')->iso8601 }
335355

‎lib/App/Sqitch/Engine/firebird.pm‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -931,8 +931,10 @@ sub default_client {
931931
open STDERR, '>&', $olderr or hurl firebird => __x(
932932
'Cannot dup STDERR: {error}', $!
933933
);
934-
hurl firebird => __(
935-
'Unable to locate Firebird ISQL; set "engine.firebird.client" via sqitch config'
934+
hurl firebird => __x(
935+
'Unable to locate {cli} client; set "engine.{eng}.client" via sqitch config',
936+
cli => 'Firebird ISQL',
937+
eng => 'firebird',
936938
);
937939
}
938940

‎t/clickhouse.t‎

Lines changed: 68 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,6 @@ isa_ok my $ch = $CLASS->new(sqitch => $sqitch, target => $target), $CLASS;
5454
is $ch->key, 'clickhouse', 'Key should be "clickhouse"';
5555
is $ch->name, 'ClickHouse', 'Name should be "ClickHouse"';
5656

57-
my $client = 'clickhouse-client' . (App::Sqitch::ISWIN ? '.exe' : '');
58-
is $ch->client, $client, 'client should default to clickhouse';
5957
is $ch->registry, 'sqitch', 'registry default should be "sqitch"';
6058
my $sqitch_uri = $uri->clone;
6159
$sqitch_uri->dbname('sqitch');
@@ -65,44 +63,81 @@ is $ch->_dsn, 'dbi:ODBC:DSN=sqitch', 'DSN should use DBD::ODBC with registry dat
6563
is $ch->registry_destination, 'db:clickhouse:sqitch',
6664
'registry_destination should be the same as registry_uri';
6765

66+
67+
# Test the client.
68+
my $mock_sqitch = Test::MockModule->new('App::Sqitch');
6869
my @std_opts = (
6970
'--progress' => 'off',
7071
'--progress-table' => 'off',
7172
'--disable_suggestion',
7273
);
7374

74-
my $mock_sqitch = Test::MockModule->new('App::Sqitch');
75-
my $warning;
76-
$mock_sqitch->mock(warn => sub { shift; $warning = [@_] });
77-
$ch->uri->dbname('');
78-
is_deeply [$ch->cli], [$client, @std_opts],
79-
'clickhouse command should be std opts-only';
80-
is_deeply $warning, [__x
81-
'Database name missing in URI "{uri}"',
82-
uri => $ch->uri
83-
], 'Should have emitted a warning for no database name';
84-
85-
isa_ok $ch = $CLASS->new(sqitch => $sqitch, target => $target), $CLASS;
86-
ok $ch->set_variables(foo => 'baz', whu => 'hi there', yo => 'stellar'),
87-
'Set some variables';
88-
is_deeply [$ch->cli], [
89-
$client,
90-
'--param_foo' => 'baz',
91-
'--param_whu' => 'hi there',
92-
'--param_yo' => 'stellar',
93-
@std_opts,
94-
], 'Variables should be passed to psql via --set';
75+
##############################################################################
76+
NO_CLI: {
77+
# Make sure we get an error when it can't find the client.
78+
my $mock_fs = Test::MockModule->new('File::Spec');
79+
$mock_fs->mock(path => sub { 't', 'bin' });
80+
throws_ok {
81+
$CLASS->new( sqitch => $sqitch, target => $target )->client
82+
} 'App::Sqitch::X', 'Should get an error when cannot find CLI';
83+
is $@->ident, 'clickhouse', "Ident for missing CLI should be 'clickhouse'";
84+
is $@->message, __x(
85+
'Unable to locate {cli} client; set "engine.{eng}.client" via sqitch config',
86+
cli => 'clickhouse',
87+
eng => 'clickhouse',
88+
), "Message for missing CLI should be correct";
89+
}
9590

96-
$mock_sqitch->unmock_all;
91+
##############################################################################
92+
CLI: {
93+
my @client;
94+
if (my $cli = try { $ch->client }) {
95+
like $cli, qr/\Aclickhouse(?:-client)?(?:\.exe)?\z/,
96+
'client should default clickhouse or clickhouse-client';
97+
@client = $cli =~ /-client/ ? ($cli) : ($cli, 'client');
98+
} else {
99+
# No client found, explicitly set it to `clickhouse`.
100+
@client = (('clickhouse' . (App::Sqitch::ISWIN ? '.exe' : '')), 'client');
101+
$config->update( 'engine.clickhouse.client' => $client[0]);
102+
}
97103

98-
$target = App::Sqitch::Target->new(
99-
sqitch => $sqitch,
100-
uri => URI::db->new('db:clickhouse:'),
101-
);
102-
isa_ok $ch = $CLASS->new(
103-
sqitch => $sqitch,
104-
target => $target,
105-
), $CLASS;
104+
my $warning;
105+
$mock_sqitch->mock(warn => sub { shift; $warning = [@_] });
106+
$ch->uri->dbname('');
107+
is_deeply [$ch->cli], [@client, @std_opts],
108+
'clickhouse command should be std opts-only';
109+
is_deeply $warning, [__x
110+
'Database name missing in URI "{uri}"',
111+
uri => $ch->uri
112+
], 'Should have emitted a warning for no database name';
113+
114+
isa_ok $ch = $CLASS->new(sqitch => $sqitch, target => $target), $CLASS;
115+
ok $ch->set_variables(foo => 'baz', whu => 'hi there', yo => 'stellar'),
116+
'Set some variables';
117+
is_deeply [$ch->cli], [
118+
@client,
119+
'--param_foo' => 'baz',
120+
'--param_whu' => 'hi there',
121+
'--param_yo' => 'stellar',
122+
@std_opts,
123+
], 'Variables should be passed to psql via --set';
124+
125+
# Try alternate spelling of client.
126+
my $mock = Test::MockModule->new($CLASS);
127+
if (@client == 1) {
128+
push @client => 'client';
129+
} else {
130+
$client[0] =~ s/clickhouse/clickhouse-client/;
131+
pop @client;
132+
}
133+
$mock->mock(client => $client[0]);
134+
135+
isa_ok $ch = $CLASS->new(sqitch => $sqitch, target => $target), $CLASS;
136+
is_deeply [$ch->cli], [ @client, @std_opts ],
137+
'Alternate spelling of client should be returned';
138+
139+
$mock_sqitch->unmock_all;
140+
}
106141

107142
##############################################################################
108143
# Make sure config and environment variables are read.
@@ -971,7 +1006,7 @@ DBIEngineTest->run(
9711006
say '# Connected to ClickHouse ' . $self->_capture('--query' => 'SELECT version()');
9721007
1;
9731008
},
974-
engine_err_regex => qr/^Error while processing query /,
1009+
engine_err_regex => qr/Syntax error: failed at position 8/,
9751010
init_error => __x(
9761011
'Sqitch database {database} already initialized',
9771012
database => $reg2,

‎t/firebird.t‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -386,8 +386,10 @@ FSPEC: {
386386
throws_ok { $fb->default_client } 'App::Sqitch::X',
387387
'Should get error when no client found';
388388
is $@->ident, 'firebird', 'Client exception ident should be "firebird"';
389-
is $@->message, __(
390-
'Unable to locate Firebird ISQL; set "engine.firebird.client" via sqitch config'
389+
is $@->message, __x(
390+
'Unable to locate {cli} client; set "engine.{eng}.client" via sqitch config',
391+
cli => 'Firebird ISQL',
392+
eng => 'firebird',
391393
), 'Client exception message should be correct';
392394
}
393395

0 commit comments

Comments
 (0)