Snowflake Query Execution [#73] #104

seanpreston · 2021-11-24T20:13:50Z

Purpose

This PR contains query execution for the Snowflake external datastore.

Changes

Overrides the SQLQueryConfig class to with a SnowflakeQueryConfig
Adds methods in the base SQLQueryConfig class to format variables that eventually end up in the query string
Implements those methods in the SnowflakeQueryConfig
Adds a dataset for our test Snowflake schema with data_type and length meta annotations
Adds an API test that runs an access request through Snowflake
Adds an API test that runs an erasure request through Snowflake

Left to do

Make similar query formatting changes for erasure handling in Snowflake
Add a separate test schema (Atlas uses the current one) on Snowflake for Fidesops
Configure test data programmatically for the new Snowflake test schema in the test fixtures
More thorough testing, e.g. specifically for the Variant type and erasure handling
Add type attribute into Snowflake dataset

Ticket

Closes #73

data/dataset/snowflake_example_test_dataset.yml

…it's tested

seanpreston · 2021-12-08T22:00:43Z

tests/fixtures.py

 @pytest.fixture
 def example_datasets() -> List[Dict]:
    example_datasets = []
    example_filenames = [
        "data/dataset/postgres_example_test_dataset.yml",
        "data/dataset/mongo_example_test_dataset.yml",
+        "data/dataset/snowflake_example_test_dataset.yml",


My plan from here out is to avoid adding any subsequent datasets to this fixture, preferring to have them loaded in by a dedicated fixture for each one.

seanpreston · 2021-12-08T22:03:18Z

tests/service/privacy_request/request_runner_service_test.py

@@ -129,7 +131,7 @@ def test_create_and_process_access_request(

    policy.delete(db=db)
    pr.delete(db=db)
-    db.expunge_all()


This line was causing downstream failures in the test teardown where FKs also needed to be cleaned up (specifically the ConnectionConfig -> DatasetConfig relation). After talking to @pattisdr I discovered this line was here as a forcing function to ensure that pr is no longer in the database session when the execution logs are checked for their .privacy_request_id attribute, a step to ensure that value is not cleared when pr is deleted.

I've opted to manually check whether pr is in the session in the line below, as this performs the same function.

seanpreston · 2021-12-08T22:04:20Z

tests/util/test_logger.py

 from fidesops.util import logger
 from fidesops.util.logger import NotPii, MASKED


-def test_logger_masks_pii() -> None:
+@pytest.fixture(scope="function")
+def toggle_testing_envvar() -> None:


This is useful to explicitly check areas where we use the os.environ["TESTING"] in production code

seanpreston · 2021-12-08T22:07:29Z

tests/conftest.py

@@ -38,8 +41,7 @@ def migrate_test_db() -> None:
 def db() -> Generator:
    """Return a connection to the test DB"""
    # Create the test DB enginge
-    ## This asserts that TESTING==True
-    assert os.getenv("TESTING", False)
+    assert os.getenv("TESTING") == "True"


cc @stevenbenjamin — I've updated these checks to look like this now as that's the correct convention according to pylint. Environment vars can only be strings so returning a bool as the default arg is dangerous. Hopefully this syntax is clearer too.

seanpreston · 2021-12-08T22:13:11Z

src/fidesops/service/connectors/query_config.py

+        operator: str,
+    ) -> str:
+        """Returns field names in clauses surrounded by quotation marks as required by Snowflake syntax."""
+        return f'"{field_name}" {operator} (:{field_name})'


Adding the () around :{field_name} is an interesting difference to how the more vanilla SQL connectors work. It could be that currently in our Postgres and MySQL connectors IN functions are accepted without parenthesis, and if so we should add them in because with-parenthesis is the standard SQL syntax.

I don't think this is a snowflake thing, but an idiosyncracy of the the way the dialect is realized in sqlalchemy. the other dialects we've used seem to add the '()' around tuple IN values automatically.

pattisdr

this looks good @seanpreston, not merging yet in case there's anything lingering you want to address but it looks good.

code was organized and readable about why certain decisions were made. I completed local access request testing outside of our regular unit tests - this behaved as expected and it was nice to see the queries being executed too in testing mode.

data/dataset/snowflake_example_test_dataset.yml

src/fidesops/models/connectionconfig.py

tests/api/v1/endpoints/test_dataset_endpoints.py

tests/fixtures.py

pattisdr · 2021-12-08T23:40:34Z

tests/fixtures.py

@@ -256,6 +262,45 @@ def snowflake_connection_config(db: Session) -> Generator:
    connection_config.delete(db)


+@pytest.fixture(scope="function")
+def snowflake_connection_config(


Naming convention is confusing now - snowflake_connection_config is based off of safe_snowflake_connection_config, but has write permissions, and then separate snowflake_read_connection_config has read. I was confused at first because I incorrectly assumed safe_snowflake_connection_config` meant read.

Good spot, I'll tidy this up in some way. Maybe unsafe_snowflake_connection_config or just snowflake_connection_config and it always has secrets if available. The read only config isn't being used anywhere since that property isn't enforced in the traversal.

@seanpreston I added this PR #49 to not do erasures if a node's connection config doesn't have write access -

src/fidesops/service/connectors/query_config.py

pattisdr · 2021-12-09T00:08:29Z

data/dataset/snowflake_example_test_dataset.yml

@@ -0,0 +1,229 @@
+dataset:
+  - fides_key: snowflake_example_test_dataset
+    name: Snowflake Example Test Dataset


As we're thinking about test fixtures for these different datasources, it would be nice to create some interdependency between them, like how the mongo example test dataset relies on a field in the postgres example test dataset, I don't think our datasets should all be completely independent datastores.

It would be nice to have the postman collection be able to make requests against our fidesops snowflake db - but the way i understand it, we wipe and repopulate it from these unit tests?

we wipe and repopulate it from these unit tests?

Not quite — we're only wiping and repopulating the data we use in erasures. Everything else is part of a copy of the dataset Atlas uses for Snowflake. There are some frustrating minor differences here, for example, in Snowflake the collection is order and in Postgres it's orders plural.

I'll factor this into the testing design for sure! Thanks

src/fidesops/service/connectors/base_connector.py

pattisdr

looks ready to me @seanpreston

Sean Preston added 10 commits November 22, 2021 21:04

fixtures for Snowflake datasets

07bed10

update key

567a1ce

use snowflake connector for snowflake dbs

18620e2

adds fixture for integration config

6da4210

set secrets on the Snowflake connection config if available

aeeb0a9

add initial test for Snowflake privacy request

716153d

extend SQLQueryConfig to cater to Snowflake dialect

3c1fdfe

Merge branch 'main' into seanpreston-73-snowflake-execution

fe91118

adds docstrings

a9267cb

add snowflake test dataset

27c4084

seanpreston added the run unsafe ci checks Triggers running of unsafe CI checks label Nov 24, 2021

Sean Preston added 2 commits November 24, 2021 20:26

fix typechecks

1334cbd

fix expected return counts

95d32df

seanpreston changed the title ~~Seanpreston 73 snowflake execution~~ Snowflake Query Execution [#73] Nov 25, 2021

seanpreston added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Nov 25, 2021

eastandwestwind reviewed Dec 2, 2021

View reviewed changes

data/dataset/snowflake_example_test_dataset.yml Show resolved Hide resolved

Sean Preston added 3 commits December 3, 2021 20:48

update statement formatting for snowflake

50ba838

fix key, add secrets

40231c2

update customer name

0dc3094

seanpreston added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Dec 6, 2021

Sean Preston added 2 commits December 6, 2021 09:13

conflict resolution

527f81f

Merge branch 'main' into seanpreston-73-snowflake-execution

ef26eb8

seanpreston added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Dec 6, 2021

Sean Preston added 4 commits December 6, 2021 09:32

adds docstring

9190fc9

ensure deleting connection configs removes related datasets

e11338b

handle snowflake

2e10b37

update methodology for testing datasets, delete privacy request once …

da53475

…it's tested

Sean Preston added 3 commits December 8, 2021 16:25

add scope to fixture

b3224d2

variant support

4acfea3

remove expunge_all in new file

9285cd3

seanpreston commented Dec 8, 2021

View reviewed changes

seanpreston added the run unsafe ci checks Triggers running of unsafe CI checks label Dec 8, 2021

seanpreston commented Dec 8, 2021

View reviewed changes

pattisdr reviewed Dec 9, 2021

View reviewed changes

Sean Preston added 2 commits December 8, 2021 19:43

remove copypasta

043b032

add docstring

def2484

seanpreston added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Dec 9, 2021

stevenbenjamin reviewed Dec 9, 2021

View reviewed changes

src/fidesops/service/connectors/base_connector.py Outdated Show resolved Hide resolved

Sean Preston added 2 commits December 9, 2021 08:24

add config.is_test_mode, remove superfluous name

8624fb3

remove circular dependency

1625ed1

seanpreston added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Dec 9, 2021

remove unused imports and fixtures

c023f63

seanpreston added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Dec 9, 2021

correct var names, allow fixture to proceed without secrets

3241ed9

seanpreston assigned pattisdr Dec 9, 2021

seanpreston added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Dec 9, 2021

pattisdr approved these changes Dec 9, 2021

View reviewed changes

pattisdr merged commit ede6f93 into main Dec 9, 2021

pattisdr deleted the seanpreston-73-snowflake-execution branch February 9, 2022 19:08

sanders41 pushed a commit that referenced this pull request Sep 22, 2022

Snowflake Query Execution [#73] (#104)

085a16a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snowflake Query Execution [#73] #104

Snowflake Query Execution [#73] #104

seanpreston commented Nov 24, 2021 •

edited

Loading

seanpreston Dec 8, 2021

seanpreston Dec 8, 2021

seanpreston Dec 8, 2021

seanpreston Dec 8, 2021

seanpreston Dec 8, 2021 •

edited

Loading

stevenbenjamin Dec 9, 2021

pattisdr left a comment

pattisdr Dec 8, 2021

seanpreston Dec 9, 2021 •

edited

Loading

pattisdr Dec 9, 2021

pattisdr Dec 9, 2021

pattisdr Dec 9, 2021

seanpreston Dec 9, 2021

pattisdr left a comment

Snowflake Query Execution [#73] #104

Snowflake Query Execution [#73] #104

Conversation

seanpreston commented Nov 24, 2021 • edited Loading

Purpose

Changes

Left to do

Ticket

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanpreston Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattisdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanpreston Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattisdr left a comment

Choose a reason for hiding this comment

seanpreston commented Nov 24, 2021 •

edited

Loading

seanpreston Dec 8, 2021 •

edited

Loading

seanpreston Dec 9, 2021 •

edited

Loading