Remove identifier quoting from snowflake

Added in v1.4.0. Turns out Snowflake allows a warehouse to be specified
in a different database, in which case dots are valid in the name and
should not be quoted! So users must properly quote when necessary, but
added notes to `sqitchtutorial-snowflake.pod` on the need to use URI
escapes for special characters. Resolves #685 (again/for real).

Also add a brief discussion on URL encoding to `sqitch-target.pod`, the
main reference for database URLs in the Sqitch documentation.

5 files changed, 60 insertions, 31 deletions

diff --git a/Changes b/Changes
index 6033fa3a..56bbdf67 100644
--- a/Changes
+++ b/Changes
@@ -1,6 +1,16 @@
 Revision history for Perl extension App::Sqitch
 
 1.4.1
+    - Removed the quoting of the role and warehouse identifiers that was
+      added to the Snowflake engine in v1.4.0. Turns out Snowflake allows a
+      warehouse to be specified in a different database, in which case dots
+      are valid in the name and should not be quoted! So users must properly
+      quote when necessary, but added notes to `sqitchtutorial-snowflake.pod`
+      on the need to use URI escapes for special characters. Thanks to
+      Patrick Sabo for the find, and to @marc-marketparts for validating
+      that URI encoding works.
+    - Added notes on URL encoding database URLs to `sqitch-target.pod`, the
+      main reference for database URLs in the Sqitch documentation.
 
 1.4.0 2023-08-01T23:37:30Z
      - Fixed Snowflake warehouse and role setup to properly quote identifiers
diff --git a/lib/App/Sqitch/Engine/snowflake.pm b/lib/App/Sqitch/Engine/snowflake.pm
index b8f45c07..d195142c 100644
--- a/lib/App/Sqitch/Engine/snowflake.pm
+++ b/lib/App/Sqitch/Engine/snowflake.pm
@@ -192,6 +192,8 @@ has dbh => (
         my $self = shift;
         $self->use_driver;
         my $uri = $self->uri;
+        my $wh = $self->warehouse;
+        my $role = $self->role;
         DBI->connect($uri->dbi_dsn, $self->username, $self->password, {
             PrintError        => 0,
             RaiseError        => 0,
@@ -207,23 +209,22 @@ has dbh => (
             Callbacks         => {
                 connected => sub {
                     my $dbh = shift;
-                    my $wh = _quote_ident($dbh, $self->warehouse);
                     my $role = $self->role;
                     $dbh->do($_) or return for (
-                        ($role ? ("USE ROLE " . _quote_ident($dbh, $role)) : ()),
+                        ($role ? ("USE ROLE $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 ' . _quote_ident($dbh, $self->registry))
+                    $dbh->do('USE SCHEMA ' . $self->registry)
                         or $self->_handle_no_registry($dbh);
                     return;
                 },
                 disconnect => sub {
                     my $dbh = shift;
-                    my $wh = _quote_ident($dbh, $self->warehouse);
+                    my $wh = $self->warehouse;
                     $dbh->do("ALTER WAREHOUSE $wh SUSPEND");
                     return;
                 },
@@ -232,14 +233,6 @@ 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/lib/sqitch-target.pod b/lib/sqitch-target.pod
index 3bf9e6da..9d5ba65c 100644
--- a/lib/sqitch-target.pod
+++ b/lib/sqitch-target.pod
@@ -40,7 +40,25 @@ Some examples:
 
 =back
 
-See the L<DB URI Draft|https://github.com/libwww-perl/uri-db> for details.
+Note that, as with any URI or URL, special characters must be
+L<URL encoded|https://en.wikipedia.org/wiki/URL_encoding>. For example, when
+a username contains a reserved character, such as the C<|> in C<ro|michelle>,
+it must be percent-encoded as C<%7c>:
+
+  db:pg://ro%7Cmichelle@examle.org/inventory
+
+The rules are even more strict for query parameters, as often used by for
+ODBC connections. For example, when using a
+L<Snowflake identifier|https://docs.snowflake.com/en/sql-reference/identifiers-syntax#double-quoted-identifiers>
+with special characters, such as a warehouse name with a C<.>, such as
+C<sqitch.dev>, the identifier must be double-quoted --- and double quote are
+reserved characters in URIs, so must be encoded as C<%22>.
+
+  db:snowflake://example/flipr?Driver=Snowflake;warehouse=%22sqitch.dev%22
+
+See L<URL encoding|https://en.wikipedia.org/wiki/URL_encoding> for details.
+and the L<DB URI Draft|https://github.com/libwww-perl/uri-db> for in-depth
+information on database URIs in general.
 
 =item C<registry>
 
diff --git a/lib/sqitchtutorial-snowflake.pod b/lib/sqitchtutorial-snowflake.pod
index 4347c4b9..f6694685 100644
--- a/lib/sqitchtutorial-snowflake.pod
+++ b/lib/sqitchtutorial-snowflake.pod
@@ -183,7 +183,13 @@ this:
 
   db:snowflake://movera@example/flipr?Driver=Snowflake;warehouse=sqitch
 
-Note that Sqitch requires a C<warehouse> parameter in order to record its work
+A few notes on the URL format:
+
+=over
+
+=item *
+
+Sqitch requires a C<warehouse> parameter in order to record its work
 in the registry. The default warehouse is named C<sqitch>, so you can omit it
 from the URI if that's the warehouse you want Sqitch to use (we'll omit it for
 the remainder of this tutorial). Otherwise, specify it in the URI. Snowflake
@@ -192,7 +198,25 @@ best to put it in the C<connections> section of the
 L<F<.snowsql/config> file|https://docs.snowflake.com/en/user-guide/snowsql-start.html#configuring-default-connection-settings>.
 See L<sqitch-authentication> for details.
 
-We just tell Sqitch to use that URI to deploy the change:
+=item *
+
+This is a proper URI, where special characters must be
+L<URL encoded|https://en.wikipedia.org/wiki/URL_encoding>. For example, when
+a warehouse name or role requires
+L<identifier quoting|https://docs.snowflake.com/en/sql-reference/identifiers-syntax#double-quoted-identifiers>,
+use C<%22> for the quotation marks:
+
+  db:snowflake://movera@example/flipr?Driver=Snowflake;warehouse=%22sqitch.dev%22&role=%22app.sqitch%22
+
+Of course that can be tricky to use on the command line, so there are also
+L<environment variables|sqitch-environment/Snowflake> that can simplify things:
+
+  export SNOWSQL_ROLE='"app.sqitch"'
+  export SNOWSQL_WAREHOUSE='"sqitch.dev"'
+
+=back
+
+Back to the change. We just tell Sqitch to use that URI to deploy it:
 
   > sqitch deploy 'db:snowflake://movera@example/flipr?Driver=Snowflake'
   Adding registry tables to db:snowflake://movera@example/flipr?Driver=Snowflake
diff --git a/t/snowflake.t b/t/snowflake.t
index e0001ad8..0f957e04 100644
--- a/t/snowflake.t
+++ b/t/snowflake.t
@@ -516,22 +516,6 @@ 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;
 my $id = DBIEngineTest->randstr;