Adding support for default values for connector params #661

galvana · 2022-06-15T16:23:07Z

Purpose

Adding support for default values for connector params. Connector params with default_values defined will not be required during the initial secrets configuration. Additionally, the default_value will be used to initialize the value for the secret. This initialization will only happen if the secret value is empty, the secret will not be overwritten if the user has already defined a value.

Changes

Added a new default_value field for connector_params
Updated the SaaSSchema validator class to not require connector_params with default values
Initialize empty connection_config secrets with default_values if available
Misc cleanup for SaaS configs, any noteworthy changes will be called out in the PR comments

Checklist

Update CHANGELOG.md file
- Merge in main so the most recent CHANGELOG.md file is being appended to
- Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
- Add a link to this PR at the end of the description with the PR number as the text. example: #1
Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
If docs updated (select one):
- documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
Good unit test/integration test coverage

Ticket

Fixes #642

Misc connector config cleanup

galvana · 2022-06-15T16:26:57Z

data/saas/config/hubspot_config.yml

          method: POST
-          body: 
-            '{
+          body: |


Made this change across the board to avoid escaping single or double quotes.

galvana · 2022-06-15T16:27:46Z

data/saas/config/mailchimp_config.yml

-    
+
  endpoints:
-  - name: messages


Auto-formatting applied to SaaS configs.

galvana · 2022-06-15T16:28:50Z

data/saas/config/saas_example_config.yml

    - name: api_key
    - name: api_version
-    - name: page_limit
+    - name: page_size


Standardizing the use of page_size to represent records per page.

galvana · 2022-06-15T16:29:43Z

data/saas/config/sentry_config.yml

  client_config:
    protocol: https
-    host: <host>
+    host: <domain>


Standardizing the use of domain to represent the base URL of the SaaS provider.

galvana · 2022-06-15T16:33:20Z

src/fidesops/schemas/connection_configuration/connection_secrets_saas.py

-            connector_param.name: (str, ...)
-            for connector_param in self.saas_config.connector_params
-        }
+        field_definitions: Dict[str, Any] = {}


The field_definitions are used to denote a parameter as optional or required. A parameter with a default value will be flagged as optional and a parameter using Pydantic's (str, ...) notation will be flagged as required.

As stated in the comments

Pydantic uses the shorthand of (str, ...) to denote a required field of type str

so is this actually populating the default values? the comment makes it seems like it's just flagging them as required, but i think it's actually populating the values -- although i'm still trying to fully understand what's going on here :)

hmm, i take that back, i see that the above change (in connectionconfig.py) is actually populating the values. if that's the case, i still don't exactly get how this works - but probably just my lack of understanding. maybe we can clarify offline

This comes down to Pydantic's create_model function https://pydantic-docs.helpmanual.io/usage/models/#dynamic-model-creation. We need to create a dynamic model for secret validation based on the connector params. A sample connector_params to field_definitions conversion would look like this:

connector_params: - name: domain default_value: localhost - name: username

{ "domain": "localhost" "username": (str, ...) }

Then the cls.__fields__ value on line 23 would be

{ 'domain': ModelField(name='domain', type=str, required=False, default='localhost'), 'username': ModelField(name='username', type=str, required=True) }

And we can use the required param to determine which secrets will be required by the user.

thanks for explaining this, it makes sense now.

i think one thing that tripped me up a bit was a typo in your original comment here - you wrote

A parameter with a default value will be flagged as required

but instead it should read:

A parameter with a default value will be flagged as optional

assuming your later comment here is correct. right?

You're right, I fixed my typo in case we come back to this.

galvana · 2022-06-15T17:49:47Z

tests/service/connectors/test_queryconfig.py

        }

        # values exist for all query keys
-        assert found_query_keys(


No changes here but I got tired of having to omit this auto-format from my commits.

adamsachs

left some minor comments - most are to improve my understanding, probably just a few tweaks needed

adamsachs · 2022-06-16T18:59:49Z

src/fidesops/schemas/saas/saas_config.py

    data_protection_request: Optional[SaaSRequest] = None  # GDPR Delete

+    @validator("connector_params")
+    def validate_connector_params(cls, v: List[ConnectorParam]) -> List[ConnectorParam]:


i think we need to document this new constraint/convention we're enforcing, no?

i guess i'm also a little unsure why we're adding in this constraint as part of this PR. i can see that generally a domain field will be needed in saas configs. but unless i am missing some context, this change probably deserves its own issue/PR?

I was including this since we were talking about potentially using the domain to identify a SaaS connector. I got ahead of myself and agree with you that we should give this more thought, rolling back for now.

adamsachs · 2022-06-16T19:02:18Z

tests/fixtures/saas_example_fixtures.py

        "description": "Generic OAuth2 connector for testing",
        "version": "0.0.1",
-        "connector_params": [{"name": item} for item in secrets.values()],
+        "connector_params": [{"name": item} for item in secrets.keys()],


had this just been a bug/mistake? or is this related to the change? (still working through what exactly we're changing in the PR, so just validating my understanding...)

You are correct, this was a mistake that wasn't caught during initial development. We were adding the secret values as the names instead of the keys.

adamsachs · 2022-06-16T19:03:51Z

data/saas/config/saas_example_config.yml

+      default_value: localhost
    - name: username
    - name: api_key
    - name: api_version


we should update our docs to include the new default_value field, right?

Docs updated!

adamsachs · 2022-06-16T19:09:39Z

tests/schemas/connection_configuration/test_connection_secrets_saas.py

+        schema = SaaSSchemaFactory(saas_config).get_saas_schema()
+        del saas_example_secrets["domain"]
+        config = saas_example_secrets
+        schema.parse_obj(config)


should there be an assertion here that the schema has a default domain value? my understanding here is a bit shaky so i could be wrong.

You're right, we want to assert the initial state to make sure this passes for the right reasons.

adamsachs · 2022-06-16T19:30:30Z

src/fidesops/schemas/connection_configuration/connection_secrets_saas.py

-            connector_param.name: (str, ...)
-            for connector_param in self.saas_config.connector_params
-        }
+        field_definitions: Dict[str, Any] = {}


so is this actually populating the default values? the comment makes it seems like it's just flagging them as required, but i think it's actually populating the values -- although i'm still trying to fully understand what's going on here :)

adamsachs · 2022-06-16T19:34:47Z

src/fidesops/schemas/connection_configuration/connection_secrets_saas.py

-            connector_param.name: (str, ...)
-            for connector_param in self.saas_config.connector_params
-        }
+        field_definitions: Dict[str, Any] = {}


hmm, i take that back, i see that the above change (in connectionconfig.py) is actually populating the values. if that's the case, i still don't exactly get how this works - but probably just my lack of understanding. maybe we can clarify offline

adamsachs

approving -- all looks good now, just a minor question i put in for my own clarification.

galvana added 2 commits June 13, 2022 19:00

Adding support for default values for connector params

2163aa1

Misc connector config cleanup

Minor typo fix

0e990d5

galvana linked an issue Jun 15, 2022 that may be closed by this pull request

[SaaS Connectors] Support default values for connector params #642

Closed

galvana commented Jun 15, 2022

View reviewed changes

data/saas/config/mailchimp_config.yml

endpoints:

- name: messages

Copy link

Collaborator Author

galvana Jun 15, 2022

Choose a reason for hiding this comment

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

Auto-formatting applied to SaaS configs.

galvana commented Jun 15, 2022

View reviewed changes

Fixing unit tests

1662200

galvana commented Jun 15, 2022

View reviewed changes

galvana added 2 commits June 15, 2022 11:16

Formatting fix

8dc8212

Fixing unit tests

bdf10b5

galvana added the run unsafe ci checks Triggers running of unsafe CI checks label Jun 15, 2022

Updating fixtures

6e17340

galvana marked this pull request as ready for review June 16, 2022 17:22

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 Jun 16, 2022

adamsachs reviewed Jun 17, 2022

View reviewed changes

galvana removed the run unsafe ci checks Triggers running of unsafe CI checks label Jun 17, 2022

Updating based on PR feedback

88dcce9

galvana added the Needs doc review label Jun 17, 2022

adamsachs approved these changes Jun 17, 2022

View reviewed changes

conceptualshark approved these changes Jun 20, 2022

View reviewed changes

galvana merged commit 4624b0c into main Jun 20, 2022

galvana deleted the 642-saas-connectors-default-values-for-connector-params branch June 20, 2022 15:54

adamsachs mentioned this pull request Jun 22, 2022

Update broken references in integration tests to outdated host secrets param #697

Closed

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

Adding support for default values for connector params (#661)

44745f1

Adding support for default values for connector params #661

Adding support for default values for connector params #661

Uh oh!

Conversation

galvana commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Changes

Checklist

Ticket

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galvana Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsachs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsachs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

galvana commented Jun 15, 2022 •

edited

Loading

galvana Jun 15, 2022 •

edited

Loading