Skip to content
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

Fix table/schema quoting on drop, truncate, and rename (#983) #991

Merged
merged 6 commits into from
Sep 13, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions dbt/adapters/default/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,14 @@ def cancel_connection(cls, project_cfg, connection):
raise dbt.exceptions.NotImplementedException(
'`cancel_connection` is not implemented for this adapter!')

@classmethod
def _quote_policy(cls, project_cfg):
quoting_cfg = project_cfg.get('quoting', {})
for quote_key in ('dtabase', 'schema', 'identifier'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be database?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to account for project here too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, I'm pretty confused about cls.DEFAULT_QUOTE. Why would we use that instead of the default quote policy on a given Relation class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't really realize how that worked, I've moved it to use that instead.

if quote_key not in quoting_cfg:
quoting_cfg[quote_key] = cls.DEFAULT_QUOTE
return quoting_cfg

###
# FUNCTIONS THAT SHOULD BE ABSTRACT
###
Expand All @@ -172,7 +180,8 @@ def drop(cls, profile, project_cfg, schema,
relation = cls.Relation.create(
schema=schema,
identifier=identifier,
type=relation_type)
type=relation_type,
quote_policy=cls._quote_policy(project_cfg))

return cls.drop_relation(profile, project_cfg, relation, model_name)

Expand All @@ -193,7 +202,8 @@ def truncate(cls, profile, project_cfg, schema, table, model_name=None):
relation = cls.Relation.create(
schema=schema,
identifier=table,
type='table')
type='table',
quote_policy=cls._quote_policy(project_cfg))

return cls.truncate_relation(profile, project_cfg,
relation, model_name)
Expand All @@ -208,12 +218,20 @@ def truncate_relation(cls, profile, project_cfg,
@classmethod
def rename(cls, profile, project_cfg, schema,
from_name, to_name, model_name=None):
quote_policy = cls._quote_policy(project_cfg)
from_relation = cls.Relation.create(
schema=schema,
identifier=from_name,
quote_policy=quote_policy
)
to_relation = cls.Relation.create(
identifier=to_name,
quote_policy=quote_policy
)
return cls.rename_relation(
profile, project_cfg,
from_relation=cls.Relation.create(
schema=schema, identifier=from_name),
to_relation=cls.Relation.create(
identifier=to_name),
from_relation=from_relation,
to_relation=to_relation,
model_name=model_name)

@classmethod
Expand Down
6 changes: 5 additions & 1 deletion dbt/adapters/postgres/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,11 @@ def alter_column_type(cls, profile, project, schema, table, column_name,
4. Rename the new column to existing column
"""

relation = cls.Relation.create(schema=schema, identifier=table)
relation = cls.Relation.create(
schema=schema,
identifier=table,
quote_policy=cls._quote_policy(project)
)

opts = {
"relation": relation,
Expand Down
85 changes: 85 additions & 0 deletions test/unit/test_postgres_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from dbt.adapters.postgres import PostgresAdapter
from dbt.exceptions import ValidationException
from dbt.logger import GLOBAL_LOGGER as logger # noqa
from psycopg2 import extensions as psycopg2_extensions
import agate


Expand Down Expand Up @@ -108,3 +109,87 @@ def test_get_catalog_various_schemas(self, mock_run):
set(map(tuple, catalog)),
{('foo', 'bar'), ('FOO', 'baz'), ('quux', 'bar')}
)

class TestConnectingPostgresAdapter(unittest.TestCase):
def setUp(self):
flags.STRICT_MODE = False

self.profile = {
'dbname': 'postgres',
'user': 'root',
'host': 'database',
'pass': 'password',
'port': 5432,
'schema': 'public'
}

self.project = {
'name': 'X',
'version': '0.1',
'profile': 'test',
'project-root': '/tmp/dbt/does-not-exist',
'quoting': {
'identifier': False,
'schema': True,
}
}

self.handle = mock.MagicMock(spec=psycopg2_extensions.connection)
self.cursor = self.handle.cursor.return_value
self.mock_execute = self.cursor.execute
self.patcher = mock.patch('dbt.adapters.postgres.impl.psycopg2')
self.psycopg2 = self.patcher.start()

self.psycopg2.connect.return_value = self.handle
conn = PostgresAdapter.get_connection(self.profile)

def tearDown(self):
# we want a unique self.handle every time.
PostgresAdapter.cleanup_connections()
self.patcher.stop()

def test_quoting_on_drop_schema(self):
PostgresAdapter.drop_schema(
profile=self.profile,
project_cfg=self.project,
schema='test_schema'
)

self.mock_execute.assert_has_calls([
mock.call('drop schema if exists "test_schema" cascade', None)
])

def test_quoting_on_drop(self):
PostgresAdapter.drop(
profile=self.profile,
project_cfg=self.project,
schema='test_schema',
relation='test_table',
relation_type='table'
)
self.mock_execute.assert_has_calls([
mock.call('drop table if exists "test_schema".test_table cascade', None)
])

def test_quoting_on_truncate(self):
PostgresAdapter.truncate(
profile=self.profile,
project_cfg=self.project,
schema='test_schema',
table='test_table'
)
self.mock_execute.assert_has_calls([
mock.call('truncate table "test_schema".test_table', None)
])

def test_quoting_on_rename(self):
PostgresAdapter.rename(
profile=self.profile,
project_cfg=self.project,
schema='test_schema',
from_name='table_a',
to_name='table_b'
)
self.mock_execute.assert_has_calls([
mock.call('alter table "test_schema".table_a rename to table_b', None)
])
94 changes: 94 additions & 0 deletions test/unit/test_snowflake_adapter.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import mock
import unittest

import dbt.flags as flags

import dbt.adapters
from dbt.adapters.snowflake import SnowflakeAdapter
from dbt.exceptions import ValidationException
from dbt.logger import GLOBAL_LOGGER as logger # noqa
from snowflake import connector as snowflake_connector

class TestSnowflakeAdapter(unittest.TestCase):
def setUp(self):
flags.STRICT_MODE = False

self.profile = {
'dbname': 'postgres',
'user': 'root',
'host': 'database',
'pass': 'password',
'port': 5432,
'schema': 'public'
}

self.project = {
'name': 'X',
'version': '0.1',
'profile': 'test',
'project-root': '/tmp/dbt/does-not-exist',
'quoting': {
'identifier': False,
'schema': True,
}
}

self.handle = mock.MagicMock(spec=snowflake_connector.SnowflakeConnection)
self.cursor = self.handle.cursor.return_value
self.mock_execute = self.cursor.execute
self.patcher = mock.patch('dbt.adapters.snowflake.impl.snowflake.connector.connect')
self.snowflake = self.patcher.start()

self.snowflake.return_value = self.handle
conn = SnowflakeAdapter.get_connection(self.profile)

def tearDown(self):
# we want a unique self.handle every time.
SnowflakeAdapter.cleanup_connections()
self.patcher.stop()

def test_quoting_on_drop_schema(self):
SnowflakeAdapter.drop_schema(
profile=self.profile,
project_cfg=self.project,
schema='test_schema'
)

self.mock_execute.assert_has_calls([
mock.call('drop schema if exists "test_schema" cascade', None)
])

def test_quoting_on_drop(self):
SnowflakeAdapter.drop(
profile=self.profile,
project_cfg=self.project,
schema='test_schema',
relation='test_table',
relation_type='table'
)
self.mock_execute.assert_has_calls([
mock.call('drop table if exists "test_schema".test_table cascade', None)
])

def test_quoting_on_truncate(self):
SnowflakeAdapter.truncate(
profile=self.profile,
project_cfg=self.project,
schema='test_schema',
table='test_table'
)
self.mock_execute.assert_has_calls([
mock.call('truncate table "test_schema".test_table', None)
])

def test_quoting_on_rename(self):
SnowflakeAdapter.rename(
profile=self.profile,
project_cfg=self.project,
schema='test_schema',
from_name='table_a',
to_name='table_b'
)
self.mock_execute.assert_has_calls([
mock.call('alter table "test_schema".table_a rename to table_b', None)
])