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

Resolve issue with MyPy seeing files in fidesops as missing imports #719

Merged
merged 19 commits into from
Jul 5, 2022
Merged

Resolve issue with MyPy seeing files in fidesops as missing imports #719

merged 19 commits into from
Jul 5, 2022

Conversation

sanders41
Copy link
Contributor

@sanders41 sanders41 commented Jun 27, 2022 •
edited by pattisdr

Purpose

Fix issues with MyPy.

In several cases I used # type: ignore when I wasn't sure what the proper fix was. Where possible we should come back later and do a proper fix for these.

Changes

  • Remove __init__.py from the src directory.
  • Fix type errors that were present.

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)
    • documentation issue created (tag docs-team to complete issue separately)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #629

@sanders41 sanders41 marked this pull request as ready for review June 27, 2022 19:09
@sanders41 sanders41 added the run unsafe ci checks Triggers running of unsafe CI checks label Jun 27, 2022
pattisdr
pattisdr reviewed Jul 5, 2022
View reviewed changes
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.

Just small questions/suggestions. I also don't think #631 should be closed, I'd still like to keep an open issue to address some of these type: ignore

pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
dev-requirements.txt Show resolved Hide resolved
src/fidesops/util/saas_util.py Outdated Show resolved Hide resolved
src/fidesops/util/saas_util.py Outdated
@@ -44,8 +44,8 @@ def get_collection_grouped_inputs(
collections: List[Collection], name: str
) -> Optional[Set[str]]:
"""Get collection grouped inputs"""
collection: Collection = next(
(collect for collect in collections if collect.name == name), {}
collection = next(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a type of Optional[Collection] instead of removing altogether?

Copy link
Contributor Author

@sanders41 sanders41 Jul 5, 2022 •
edited

Choose a reason for hiding this comment

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

Same as previous. I'll update all the same based on the answer to the first.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, we can compromise! I'd recommend leaving the internal type hints on files in the src/fidesops/graph/graph.py folder at least. Those files can be really tricky to trace through

src/fidesops/schemas/saas/strategy_configuration.py Show resolved Hide resolved
src/fidesops/service/connectors/saas_query_config.py Outdated
@@ -128,7 +128,7 @@ def generate_query(
query statement (select statement, where clause, limit, offset, etc.)
"""

current_request: SaaSRequest = self.get_request_by_action("read")
current_request = self.get_request_by_action("read")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be optional instead of removing? current_request: Optional[SaaSRequest] = self.get_request_by_action("read")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as previous. I'll update all the same based on the answer to the first.

src/fidesops/service/masking/strategy/masking_strategy_aes_encrypt.py Outdated
Comment on lines 44 to 60
key: bytes = SecretsUtil.get_or_generate_secret(
key = SecretsUtil.get_or_generate_secret(
request_id, SecretType.key, masking_meta[SecretType.key]
)
key_hmac: str = SecretsUtil.get_or_generate_secret(
key_hmac = SecretsUtil.get_or_generate_secret(
request_id,
SecretType.key_hmac,
masking_meta[SecretType.key_hmac],
)

# The nonce is generated deterministically such that the same input val will result in same nonce
# and therefore the same masked val through the aes strategy. This is called convergent encryption, with this
# implementation loosely based on https://www.vaultproject.io/docs/secrets/transit#convergent-encryption

masked_values: List[str] = []
for value in values:
nonce: bytes = self._generate_nonce(
value, key_hmac, request_id, masking_meta
)
masked: str = encrypt(value, key, nonce)
nonce = self._generate_nonce(value, key_hmac, request_id, masking_meta) # type: ignore
masked: str = encrypt(value, key, nonce) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these removed hints just be made optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as previous. I'll update all the same based on the answer to the first.

src/fidesops/service/masking/strategy/masking_strategy_hash.py Outdated
@@ -50,14 +50,15 @@ def mask(
masking_meta: Dict[
SecretType, MaskingSecretMeta
] = self._build_masking_secret_meta()
salt: str = SecretsUtil.get_or_generate_secret(
salt = SecretsUtil.get_or_generate_secret(
Copy link
Contributor

Choose a reason for hiding this comment

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

optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as previous. I'll update all the same based on the answer to the first.

src/fidesops/service/masking/strategy/masking_strategy_hmac.py Outdated
Comment on lines 52 to 55
key: str = SecretsUtil.get_or_generate_secret(
key = SecretsUtil.get_or_generate_secret(
request_id, SecretType.key, masking_meta[SecretType.key]
)
salt: str = SecretsUtil.get_or_generate_secret(
salt = SecretsUtil.get_or_generate_secret(
Copy link
Contributor

Choose a reason for hiding this comment

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

optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as previous. I'll update all the same based on the answer to the first.

@pattisdr
Copy link
Contributor

pattisdr commented Jul 5, 2022

only safe migration check tests failing which is a known error

@pattisdr pattisdr merged commit ce471af into main Jul 5, 2022
@pattisdr pattisdr deleted the mypy branch July 5, 2022 20:46
sanders41 added a commit that referenced this pull request Sep 22, 2022
@sanders41
Resolve issue with MyPy seeing files in fidesops as missing imports (#…
f40c1ee 
…719)


Co-authored-by: Paul Sanders <pau@ethyca.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pattisdr pattisdr pattisdr approved these changes

Assignees

@pattisdr pattisdr

Labels
run unsafe ci checks Triggers running of unsafe CI checks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MyPy settings causing errors to be ignored
2 participants
@sanders41 @pattisdr