Improve Snowflake quote ident

sqitchers · Jul 29, 2023 · 29838d1 · 29838d1
1 parent 9b4327a
commit 29838d1
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 7 deletions.
diff --git a/.github/workflows/snowflake.yml b/.github/workflows/snowflake.yml
@@ -29,5 +29,5 @@ jobs:
         env:
           PERL5LIB: "${{ github.workspace }}/local/lib/perl5"
           LIVE_SNOWFLAKE_REQUIRED: true
-          SQITCH_TEST_SNOWFLAKE_URI: db:snowflake://${{ secrets.SNOWFLAKE_USERNAME }}:${{ secrets.SNOWFLAKE_PASSWORD }}@sra81677.us-east-1/sqitchtest?Driver=Snowflake;warehouse=COMPUTE_WH
+          SQITCH_TEST_SNOWFLAKE_URI: db:snowflake://${{ secrets.SNOWFLAKE_USERNAME }}:${{ secrets.SNOWFLAKE_PASSWORD }}@sra81677.us-east-1/sqitchtest?Driver=Snowflake;warehouse=compute_wh
         run: prove -lvr t/snowflake.t
diff --git a/Changes b/Changes
@@ -1,8 +1,9 @@
 Revision history for Perl extension App::Sqitch
 
 1.3.2
-     - Fixed Snowflake warehouse and role setup to properly quote identifiers.
-       Thanks to @marc-marketparts for the report (#685).
+     - Fixed Snowflake warehouse and role setup to properly quote identifiers
+       unless they're valid unquoted identifiers or already quoted. Thanks to
+       @marc-marketparts for the report (#685).
      - Fixed a bug reworking a change when a rework directory is configured
        but not created. Thanks to @jfeaver for the report (#686).
      - Output the list of changes to be deployed or reverted when --verbose is

diff --git a/lib/App/Sqitch/Engine/snowflake.pm b/lib/App/Sqitch/Engine/snowflake.pm
@@ -207,23 +207,23 @@ has dbh => (
             Callbacks         => {
                 connected => sub {
                     my $dbh = shift;
-                    my $wh = $dbh->quote_identifier($self->warehouse);
+                    my $wh = _quote_ident($dbh, $self->warehouse);
                     my $role = $self->role;
                     $dbh->do($_) or return for (
-                        ($role ? ("USE ROLE " . $dbh->quote_identifier($role)) : ()),
+                        ($role ? ("USE ROLE " . _quote_ident($dbh, $role)) : ()),
                         "ALTER WAREHOUSE $wh RESUME IF SUSPENDED",
                         "USE WAREHOUSE $wh",
                         'ALTER SESSION SET TIMESTAMP_TYPE_MAPPING=TIMESTAMP_LTZ',
                         "ALTER SESSION SET TIMESTAMP_OUTPUT_FORMAT='YYYY-MM-DD HH24:MI:SS'",
                         "ALTER SESSION SET TIMEZONE='UTC'",
                     );
-                    $dbh->do('USE SCHEMA ' . $dbh->quote_identifier($self->registry))
+                    $dbh->do('USE SCHEMA ' . _quote_ident($dbh, $self->registry))
                         or $self->_handle_no_registry($dbh);
                     return;
                 },
                 disconnect => sub {
                     my $dbh = shift;
-                    my $wh = $dbh->quote_identifier($self->warehouse);
+                    my $wh = _quote_ident($dbh, $self->warehouse);
                     $dbh->do("ALTER WAREHOUSE $wh SUSPEND");
                     return;
                 },
@@ -232,6 +232,14 @@ has dbh => (
     }
 );
 
+sub _quote_ident {
+    my ($dbh, $ident) = @_;
+    # https://docs.snowflake.com/en/sql-reference/identifiers-syntax
+    return $ident if $ident =~ /^[_a-zA-Z][_a-zA-Z0-9\$]*$/;
+    return $ident if $ident =~ /^"/ && $ident =~ /"$/;
+    return $dbh->quote_identifier($ident);
+}
+
 # Need to wait until dbh is defined.
 with 'App::Sqitch::Role::DBIEngine';
 

diff --git a/t/snowflake.t b/t/snowflake.t
@@ -514,6 +514,22 @@ ok my $now = App::Sqitch::DateTime->now, 'Construct a datetime object';
 is $snow->_char2ts($now), $now->as_string(format => 'iso'),
     'Should get ISO output from _char2ts';
 
+##############################################################################
+# Test _quote_ident.
+# Mock DBI method.
+sub quote_identifier { qq{"$_[1]"} }
+
+ok my $quote_ident = $CLASS->can('_quote_ident'), 'Should have _quote_ident sub';
+# https://docs.snowflake.com/en/sql-reference/identifiers-syntax#unquoted-identifiers
+for my $ident (qw(foo FOO _xXx _ a id1 My$Thing), "foo") {
+    is $quote_ident->(__PACKAGE__, $ident), $ident, "Should not quote “$ident”";
+}
+
+for my $ident (qw(my.thing 1go $foo идентификатор), 'hi there', qq{'thing'}) {
+    is $quote_ident->(__PACKAGE__, $ident), quote_identifier(__PACKAGE__, $ident),
+        "Should quote “$ident”";
+}
+
 ##############################################################################
 # Can we do live tests?
 my $dbh;