Populate Application DB instead of the Test DB #1731

pattisdr · 2022-11-09T23:40:50Z

❗ This could create breaking changes in the -plus repos that would need to be handled before they are upgraded to use this

Closes #1175

Code Changes

Mostly ctl-side changes:

Environment Variables

Don't set FIDES__TEST_MODE in the docker-compose.yml - this should be set by pytest only. So this is only True when running pytest.
Update incorrect variable name TESTING to FIDES__TEST_MODE in pyproject.toml to set test mode to true for pytest only. TESTING was the old variable we used in ops.
Update base credentials > app_postgres to be a connection string pointing to the fides db instead of the fides_test db in the .fides/fides.toml. Running fides scan dataset db --credentials-id app_postgres locally should scan the application db not the test db.

Sessions

Dependency injection: Inject an async session as a dependency into the ctl crud, datamap, view, and generate routes. Pass that session onto the crud methods. (Ops already injects a session into its routes. ) The admin route doesn't seem to need a session because it is using database_uri's that have test_mode awareness. The health endpoint is already injecting the sync session from ops. The validate routes don't seem to need a session either.
- Create an async_db generator for API endpoint dependency injection.
Update the ctl database crud methods to take in an async_session as a variable (api/ctl/database/crud.py), which is test_mode aware
Update ctl route utils to take in an async_session parameter (api/ctl/routes/util.py) that is test mode aware

Test refactors

Add an async_session to the ctl conftest for use in testing methods that need an async_session
Remove unused user fixtures from the ctl conftest
Monkeypatch several methods in the requests library to use the TestClient (which calls functions directly instead of sending a request to a running webserver). This is primarily for the methods in src/fides/ctl/core/api.py. Now all the CLI-related tests and all methods calling api.get or api.post etc. should be usingthe TestClient instead of the requests library.

Ops changes

Ops was already using a TestClient to test its endpoints, which does not ping a running webserver.

Update ops test setup_db to reset the test db, not the application db (by using the test client).

Misc

Fix existing logging tests to remove need for unnecessary toggle_testing_envvar fixture
Have logging_sessions log debug logs on dev mode, not test mode (test mode is false now, when running application locally). Running the application had been resulting in fewer logs now that test mode was false.
Have load_default_organization and load_default_taxonomy take in an async_session (ctl-side)

Steps to Confirm

Run nox -s dev -- postgres to bring up the webserver
Connect to the fides application db psql -h localhost -p 5432 -U postgres -d fides locally and verify that this application db is up and populated
Run a privacy request against the postgres db and verify the supporting resources are all created in the application db
Try to connect to the test db and observe it doesn't exist yet psql -h localhost -p 5432 -U postgres -d fides_test
Now bring up the shell nox -s dev -- postgres shell
Run some tests in pytest pytest tests/ctl/database/test_crud.py, put a breakpoint in the test after resources are created
Connect to the test db ``psql -h localhost -p 5432 -U postgres -d fides_test`. Observe the test db was created, and the resources exist in the test db
Query the application db - observe that it is not wiped, and previously created resources still exist
Run fides get data_category user - observe successful return from application db

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation Updated:
- documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
- documentation issue created (tag docs-team to complete issue separately)
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md

Description Of Changes

Fides is currently writing to the test db both when running the application and when we're running pytest. This is difficult for local development - when you run tests in pytest, the test db is wiped and populated with fixture data. It would be useful to populate the application db when running the application locally and run the test db when running pytest.

Note that before the merge of fidesctl and fidesops, fidesops already had this division, writing to the application db or the test db depending on the context . Fidesctl differed in that it spun up a webserver and the tests made requests against that running webserver. Fidesops used test clients. This refactor primarily injects sessions into ctl-routes, and then uses test clients that have awareness of test mode to talk to the test db during testing. However, for the CLI tests that are talking to a running webserver, we're monkeypatching the requests library to use the TestClient instead.

Note

I want to make sure we're not being too aggressive with the monkey patching - ops uses the requests library to make connector requests in many places. I am only monkeypatching the requests library ctl side.

…cation. - Don't set FIDES__TEST_MODE in docker-compose - this should be set by pytest only - Update pytest's setting of FIDES__TEST_MODE to be the correct variable - Rename the ops "get_api_session" to "get_sync_session" to mirror the language that ctl is using. It is a session that uses a synchronous engine. - Fix logging test to remove need for unnecessary toggle_testing_envvar fixture - Update the ctl database crud methods to take in an async_session as a variable - Inject an async session as a dependency into the crud, datamap, and generate routes. Pass that session onto the crud methods. - Update ctl route utils to take in an async_session parameter - Update ops and ctl setup_db in tests to not use db_action method - instead use the TestClient as to not not wipe the application db - Add an async_session to the ctl conftest for use in testing - Have logging_sessions use debug logs on dev mode, not test mode (test mode is false now, when running application locally) - Have load_default_organization and load_default_taxonomy take in an async_session (ctl) - Load_default_dsr_policies (ops) still uses a sync session - For ctl, create an async_db generator for API endpoint dependency injection. Also create an async_session and async_sessionmaker that shares the same engine - Use the api client in several of the ctl tests, which is aware of test mode, instead of hitting the server.

tests/ctl/core/test_system.py

- This verifies that if we're in test mode, we should be supplying a TestClient to these methods and we'll use that client to make the requests. Otherwise, we use the requests library which makes requests to the running webserver. (Also explored using httpx.Client to have both prod and testing use a "client" but this seems to just add an extra requirement. - A lot of methods were refactored to optionally pass in this client for "test mode" only.

…t fides_test db. - Fix AnalyticsClient developer_mode to reference dev mode instead of incorrect test mode (FIDES_TEST_MODE was incorrect variable anyway - it's FIDES__TEST_MODE)

src/fides/ctl/core/api.py

.fides/fides.toml

docker-compose.yml

pyproject.toml

src/fides/api/ctl/database/crud.py

src/fides/api/ctl/database/session.py

src/fides/api/ops/api/deps.py

src/fides/cli/utils.py

src/fides/ctl/core/config/logging_settings.py

tests/ctl/conftest.py

tests/ctl/cli/test_cli.py

ThomasLaPiana · 2022-11-16T19:39:40Z

after talking in a 1:1 chat, we discussed trying to monkey-patch requests.post -> test_client.post to avoid the api.py code from needing to know about testing

…methods." This reverts commit 0255f59.

… instead.

This reverts commit a89f0bb.

…tion level and ensure it is called before setup db to prevent wiping the application db during testing. - Remove changes that are no longer necessary with the monkeypatch changes in an attempt to reduce the size of the diff.

- Remove monkeypatching requests on ops side. Ping the reset URL in setup_db with the api_client instead of the requests library.

tests/ctl/conftest.py

# Conflicts: # tests/ctl/api/test_seed.py

pattisdr · 2022-11-22T21:19:16Z

Issues still existing from the merge ^ will address

…ctl side which is now being used in the seed tests.

…yncClient that was instantiated with the application so it is aware that we are in test mode and talks to the correct test db instead of the application db

pattisdr · 2022-12-05T17:21:34Z

Latest commit f5ddef8 needed to get up to date with recent changes in #1860 and #1861. These tests were failing because they were talking to the application db instead of the test db. We're now populating the test db for tests.

…lication_fixtures to be used elsewhere. Monkeypatch Session.send to be TestClient.send (which is instantiated with the application and is test-mode aware) in testing.

pattisdr · 2022-12-05T23:52:58Z

^ I still have one test hanging for the FidesConnector work: tests/ops/service/connectors/test_fides_connector.py::TestFidesConnectorIntegration::test_retrieve_data after merging in main

The FidesConnector/FidesClient use their own requests.Sessions and httpx.AsyncClient()s to make requests against the running webserver. I tried to monkeypatch similar to what we did on the -ctl side to use TestClients instead. This test is currently creating/queueing a privacy request but the privacy request status is not getting updated.

EDIT Going back into draft mode

ThomasLaPiana · 2022-12-06T04:36:38Z

^ I still have one test hanging for the FidesConnector work: tests/ops/service/connectors/test_fides_connector.py::TestFidesConnectorIntegration::test_retrieve_data after merging in main

The FidesConnector/FidesClient use their own requests.Sessions and httpx.AsyncClient()s to make requests which make requests against the running webserver. I'm trying to monkeypatch similar to what we did on the -ctl side to use TestClients instead. This test is currently creating/queueing a privacy request but the privacy request status is not getting updated.

EDIT Going back into draft mode

Sounds good, lmk if I can help!

src/fides/api/ops/service/privacy_request/request_service.py

tests/ops/service/connectors/test_fides_connector.py

Conflicts: src/fides/api/ctl/routes/util.py src/fides/api/ops/service/connectors/fides/fides_client.py src/fides/cli/utils.py src/fides/ctl/core/config/logging_settings.py src/fides/ctl/core/config/utils.py tests/ctl/api/test_seed.py tests/ctl/cli/test_cli_utils.py tests/ctl/conftest.py tests/ops/conftest.py tests/ops/util/test_logger.py

…ConnectorIntegration.test_retrieve_data. Both of these will just make requests to the running webserver (which is talking to the application db), but we need them to talk to the test db. - If no privacy request found in poll_server_for_completion, raise a specific error instead of throwing a vague IndexError: list index out of range.

pattisdr · 2022-12-23T20:32:05Z

OK I think we're ready to go again!

pattisdr · 2022-12-23T21:58:34Z

@ThomasLaPiana this PR has sat for awhile, as I deprioritized to work on unified-fides-resources, but with this last commit 834aae1, I think it's good to go.

ThomasLaPiana · 2022-12-24T00:27:53Z

@pattisdr you're a hero, thank you! I've been anxiously awaiting! I'll re-review/retest 🙂

pattisdr · 2022-12-24T02:09:43Z

hooray! I think this functionality 0882943 that Adam merged while this was sitting might have helped tests pass too. Previously in trying to monkeypatch async clients in the fides connector test I had been running into errors with multiple event loops.

src/fides/api/ops/service/connectors/fides/fides_client.py

tests/ctl/conftest.py

ThomasLaPiana

Fantastic work here! Excited to see this merged and making development easier 🙂

…from pytest.

pattisdr · 2023-01-03T16:40:19Z

Appreciate the re-review @ThomasLaPiana!

pattisdr added 2 commits November 9, 2022 17:38

Restore deleted fixture and refactor.

ddf38f3

pattisdr commented Nov 10, 2022

View reviewed changes

tests/ctl/core/test_system.py Outdated Show resolved Hide resolved

pattisdr added 3 commits November 13, 2022 13:28

Fix missed mock assertions.

a89f0bb

Update "app_postgres" in main fides.toml file to point to fides db no…

8c8215b

…t fides_test db. - Fix AnalyticsClient developer_mode to reference dev mode instead of incorrect test mode (FIDES_TEST_MODE was incorrect variable anyway - it's FIDES__TEST_MODE)

ThomasLaPiana reviewed Nov 14, 2022

View reviewed changes

src/fides/ctl/core/api.py Outdated Show resolved Hide resolved

Remove unused fixtures from the -ctl side.

7b474f8

pattisdr changed the title ~~[Draft] Populate Application DB instead of the Test DB~~ Populate Application DB instead of the Test DB Nov 14, 2022

pattisdr marked this pull request as ready for review November 14, 2022 20:26

pattisdr requested review from a team and ThomasLaPiana November 14, 2022 20:26

pattisdr self-assigned this Nov 14, 2022

pattisdr commented Nov 14, 2022

View reviewed changes

Update CHANGELOG.

885c83a

pattisdr added 7 commits November 18, 2022 09:13

Merge branch 'main' into fides_1175_populate_application_db

886cba7

Revert "Add a verify_client decorator for use in the ctl/core/api.py …

6bcd29b

…methods." This reverts commit 0255f59.

Monkeypatch "requests" functions in tests to use TestClient functions…

8115826

… instead.

Revert "Fix missed mock assertions."

f86d260

This reverts commit a89f0bb.

Override requests.get for the test that is pinging a bad URL.

836845b

- Remove monkeypatching requests on ops side. Ping the reset URL in setup_db with the api_client instead of the requests library.

Add some more sanity checks.

47ab61f

pattisdr commented Nov 18, 2022

View reviewed changes

tests/ctl/conftest.py Outdated Show resolved Hide resolved

pattisdr marked this pull request as draft November 18, 2022 21:21

pattisdr marked this pull request as ready for review November 18, 2022 21:21

pattisdr added 2 commits November 18, 2022 15:30

Fix failing to inject fixture.

45f74e3

Merge branch 'main' into fides_1175_populate_application_db

50a8325

# Conflicts: # tests/ctl/api/test_seed.py

Restore removed db fixture which yields a synchronous session on the …

7709adb

…ctl side which is now being used in the seed tests.

pattisdr added 2 commits December 5, 2022 10:04

Update Changelog.

353736f

For testing purposes of "test_poll_for_completion", pass in a test As…

f5ddef8

…yncClient that was instantiated with the application so it is aware that we are in test mode and talks to the correct test db instead of the application db

Move test_fides_client and authenticated_fides_client fixtures to app…

7f42881

…lication_fixtures to be used elsewhere. Monkeypatch Session.send to be TestClient.send (which is instantiated with the application and is test-mode aware) in testing.

pattisdr marked this pull request as draft December 6, 2022 00:09

adamsachs reviewed Dec 6, 2022

View reviewed changes

src/fides/api/ops/service/privacy_request/request_service.py Outdated Show resolved Hide resolved

adamsachs reviewed Dec 6, 2022

View reviewed changes

tests/ops/service/connectors/test_fides_connector.py Show resolved Hide resolved

pattisdr force-pushed the fides_1175_populate_application_db branch from a4123b2 to 7f42881 Compare December 7, 2022 14:38

pattisdr added 4 commits December 23, 2022 11:08

Fix items missed in merge.

c7c8571

Clean up user fixtures and don't drop all the tables.

9cd1b6f

pattisdr marked this pull request as ready for review December 23, 2022 20:32

pattisdr added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Dec 23, 2022

ThomasLaPiana reviewed Jan 3, 2023

View reviewed changes

src/fides/api/ops/service/connectors/fides/fides_client.py Show resolved Hide resolved

ThomasLaPiana reviewed Jan 3, 2023

View reviewed changes

tests/ctl/conftest.py Show resolved Hide resolved

ThomasLaPiana reviewed Jan 3, 2023

View reviewed changes

tests/ctl/conftest.py Outdated Show resolved Hide resolved

ThomasLaPiana approved these changes Jan 3, 2023

View reviewed changes

Add docstring to monkeypatch_requests fixture and import MonkeyPatch …

ed45acf

…from pytest.

pattisdr merged commit 2ef39ad into main Jan 3, 2023

pattisdr deleted the fides_1175_populate_application_db branch January 3, 2023 16:43

pattisdr mentioned this pull request Jan 13, 2023

[Unified Fides Resources] Update Test Env Setup and Quickstart #2225

Merged

11 tasks

pattisdr mentioned this pull request Feb 6, 2023

provide config overrides for db engine pool params #2511

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Populate Application DB instead of the Test DB #1731

Populate Application DB instead of the Test DB #1731

pattisdr commented Nov 9, 2022 •

edited

Loading

ThomasLaPiana commented Nov 16, 2022 •

edited

Loading

pattisdr commented Nov 22, 2022

pattisdr commented Dec 5, 2022

pattisdr commented Dec 5, 2022 •

edited

Loading

ThomasLaPiana commented Dec 6, 2022

pattisdr commented Dec 23, 2022

pattisdr commented Dec 23, 2022

ThomasLaPiana commented Dec 24, 2022

pattisdr commented Dec 24, 2022

ThomasLaPiana left a comment

pattisdr commented Jan 3, 2023

Populate Application DB instead of the Test DB #1731

Populate Application DB instead of the Test DB #1731

Conversation

pattisdr commented Nov 9, 2022 • edited Loading

Code Changes

Environment Variables

Sessions

Test refactors

Ops changes

Misc

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

Note

ThomasLaPiana commented Nov 16, 2022 • edited Loading

pattisdr commented Nov 22, 2022

pattisdr commented Dec 5, 2022

pattisdr commented Dec 5, 2022 • edited Loading

ThomasLaPiana commented Dec 6, 2022

pattisdr commented Dec 23, 2022

pattisdr commented Dec 23, 2022

ThomasLaPiana commented Dec 24, 2022

pattisdr commented Dec 24, 2022

ThomasLaPiana left a comment

Choose a reason for hiding this comment

pattisdr commented Jan 3, 2023

pattisdr commented Nov 9, 2022 •

edited

Loading

ThomasLaPiana commented Nov 16, 2022 •

edited

Loading

pattisdr commented Dec 5, 2022 •

edited

Loading