Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Adds MariaDB query execution tests #191

Merged
merged 3 commits into from
Feb 9, 2022
Merged

Conversation

eastandwestwind
Copy link
Contributor

@eastandwestwind eastandwestwind commented Feb 7, 2022

Purpose

Adds support for MariaDB query execution

Changes

  • Adds integration tests for MariaDB query execution

Checklist

  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram
  • Good unit test/integration test coverage

Ticket

Fixes #79

@eastandwestwind eastandwestwind added the run unsafe ci checks Triggers running of unsafe CI checks label Feb 7, 2022
Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

You've been knocking out a ton of db connections lately 💪 @eastandwestwind!

A few mariadb requests not related to specific lines in this PR:

  1. Can you update the mariadb port in the postman collection?
  2. Consider adding a SQLConnector.legacy_cursor_result_to_rows method that you can call from cursor_result_to_rows in MySQL, MariaDB, and MSSQL, now that we've duplicated that logic in several places.
  3. When I run make integration-env, and the MariaDB database comes up, I see it running 2022-02-09 15:27:48+00:00 [Note] [Entrypoint]: /usr/local/bin/docker-entrypoint.sh: running /docker-entrypoint-initdb.d/mariadb_example.sql in the mariadb logs, but it doesn't look like there's any data in the MariaDB database - a local privacy request doesn't turn up any data there.

I didn't realize that this branch wasn't up-to-date with main and your port change at first, so I did try bringing it up with the bad port to start, that may have affected things, but can you take a look?

@eastandwestwind
Copy link
Contributor Author

Thanks for the review!

  1. Done
  2. I wasn't 100% clear on what structure you were thinking, so let me know if this change makes sense
  3. Due to some restructuring ahead of 12619c1, we're no longer creating data in the MariaDB table as part of make steps (instead we are creating the data via test fixtures). This will be addressed by @seanpreston in the above PR so that we can test e2e requests via postman. In lieu of this, I was able to manually insert data using DBeaver, and retrieved the following access request results from Postman:
{"postgres_example:visit": [{"email": "[email protected]"}], "postgres_example:payment_card": [{"ccn": 123456789, "code": 321, "name": "Example Card 1"}], "mongo_test:customer_details": [{"gender": "male", "birthday": "1988-01-10T00:00:00"}], "mariadb_example_test_dataset:customer": [{"email": "[email protected]", "name": "John Customer"}], "postgres_example:customer": [{"email": "[email protected]", "name": "John Customer"}], "mariadb_example_test_dataset:payment_card": [{"ccn": 123456789, "code": 321, "name": "Example Card 1"}], "postgres_example:address": [{"city": "Exampletown", "house": 123, "state": "NY", "street": "Example Street", "zip": "12345"}, {"city": "Exampletown", "house": 4, "state": "NY", "street": "Example Lane", "zip": "12321"}], "postgres_example:service_request": [{"alt_email": "[email protected]"}], "mariadb_example_test_dataset:service_request": [{"alt_email": "[email protected]"}], "mariadb_example_test_dataset:address": [{"city": "Exampletown", "house": 123, "state": "NY", "street": "Example Street", "zip": "12345"}, {"city": "Exampletown", "house": 4, "state": "NY", "street": "Example Lane", "zip": "12321"}], "mariadb_example_test_dataset:visit": [{"email": "[email protected]"}]}

@pattisdr
Copy link
Contributor

pattisdr commented Feb 9, 2022

looks good @eastandwestwind! thanks for letting me know how we were planning on testing this in the future. looks like Sean is adding some steps to make integration-env to get this working again. Being able to test against a populated database isn't just useful for us, but helpful for users trying to get up to speed with fidesops.

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes!

@pattisdr pattisdr merged commit 188ebff into main Feb 9, 2022
@pattisdr pattisdr deleted the 79-mariadb-query-execution branch February 9, 2022 20:26
sanders41 pushed a commit that referenced this pull request Sep 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
run unsafe ci checks Triggers running of unsafe CI checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DB Compatibility] Query execution for scalar values in MariaDB
2 participants