Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Fideslang Typing #3839

Merged
merged 31 commits into from
Sep 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
342cd4c
Fix Fideslang Typing
ThomasLaPiana Jul 20, 2023
f738bb7
fix: implicit exports causing mypy issues
ThomasLaPiana Jul 20, 2023
3e2bccf
fix: more mypy
ThomasLaPiana Jul 20, 2023
45afc28
Merge branch 'main' into ThomasLaPiana-fix-fideslang-types
ThomasLaPiana Jul 27, 2023
845e770
feat: fix more mypy issues
ThomasLaPiana Jul 27, 2023
a72c709
fix: more mypy issues
ThomasLaPiana Jul 27, 2023
4f04a36
fix: more mypy issues
ThomasLaPiana Jul 31, 2023
aeec902
fix: mypy
ThomasLaPiana Jul 31, 2023
db8eb1c
fix: static_checks
ThomasLaPiana Jul 31, 2023
8457985
Merge branch 'main' into ThomasLaPiana-fix-fideslang-types
ThomasLaPiana Aug 3, 2023
6e27666
fix: get_server_resource tests
ThomasLaPiana Aug 3, 2023
fb22e0e
fix: TestLoadDefaultTaxonomy tests
ThomasLaPiana Aug 3, 2023
69a3d8e
fix: lib tests
ThomasLaPiana Aug 3, 2023
3f81430
fix: privacy notice tests
ThomasLaPiana Aug 3, 2023
7a52269
fix: static checks
ThomasLaPiana Aug 3, 2023
b7f0b9c
fix: revert bad system_manager_oauth_util change
ThomasLaPiana Aug 3, 2023
672163b
fix: add explicit model parsing to `audit` code
ThomasLaPiana Aug 4, 2023
d140ea2
fix: db scanning
ThomasLaPiana Aug 4, 2023
b790acb
fix: cli init test
ThomasLaPiana Aug 4, 2023
030efe1
feat: refactor a test
ThomasLaPiana Aug 6, 2023
874500f
feat: add helpful assertion messages
ThomasLaPiana Aug 6, 2023
c69c72a
fix: type signature
ThomasLaPiana Aug 8, 2023
e0f2f0c
Merge branch 'main' into ThomasLaPiana-fix-fideslang-types
ThomasLaPiana Aug 8, 2023
a72383f
fix: pylint
ThomasLaPiana Aug 8, 2023
a42893f
Merge branch 'ThomasLaPiana-fix-fideslang-types' of https://github.co…
ThomasLaPiana Aug 8, 2023
3b5b3e2
fix: explicitly convert to Cookie models
ThomasLaPiana Aug 9, 2023
761b9fc
Merge branch 'main' into ThomasLaPiana-fix-fideslang-types
ThomasLaPiana Sep 16, 2023
4255114
fix: mypy
ThomasLaPiana Sep 16, 2023
0498dd2
fix: system scanning
ThomasLaPiana Sep 17, 2023
cb8f7d0
Merge branch 'main' into ThomasLaPiana-fix-fideslang-types
ThomasLaPiana Sep 21, 2023
b1e20ae
feat: address review comments and add a new Protocol
ThomasLaPiana Sep 22, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/backend_checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ jobs:
[
"isort",
"black",
"mypy",
ThomasLaPiana marked this conversation as resolved.
Show resolved Hide resolved
"pylint",
"xenon",
"check_install",
Expand Down
3 changes: 1 addition & 2 deletions noxfiles/git_nox.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@
from enum import Enum
from typing import List, Optional

from packaging.version import Version

import nox
from packaging.version import Version

RELEASE_BRANCH_REGEX = r"release-(([0-9]+\.)+[0-9]+)"
RELEASE_TAG_REGEX = r"(([0-9]+\.)+[0-9]+)"
Expand Down
1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ module = [
"dask.*",
"deepdiff.*",
"defusedxml.ElementTree.*",
"fideslang.*",
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this section tells mypy to ignore missing type imports from imported libraries. Everything from fideslang is typed so this is redundant

at least that's my understanding

"fideslog.*",
"firebase_admin.*",
"google.api_core.*",
Expand Down
2 changes: 1 addition & 1 deletion src/fides/api/api/v1/endpoints/generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
This module generates all of the routers for the boilerplate/generic
objects that don't require any extra logic.
"""
from fideslang import Dataset, Evaluation, Organization, Policy, Registry
from fideslang.models import Dataset, Evaluation, Organization, Policy, Registry

from fides.api.schemas.taxonomy_extensions import (
DataCategory,
Expand Down
7 changes: 4 additions & 3 deletions src/fides/api/api/v1/endpoints/router_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
from typing import Dict, List

from fastapi import Depends, HTTPException, Response, Security, status
from fideslang import Dataset, FidesModelType
from fideslang import FidesModelType
from fideslang.models import Dataset
from fideslang.validation import FidesKey
from pydantic import ValidationError as PydanticValidationError
from sqlalchemy.ext.asyncio import AsyncSession
Expand All @@ -25,7 +26,7 @@
from fides.api.db.ctl_session import get_async_db
from fides.api.models.sql_models import (
DataCategory,
models_with_default_field,
ModelWithDefaultField,
sql_model_map,
)
from fides.api.oauth.utils import verify_oauth_client_prod
Expand Down Expand Up @@ -143,7 +144,7 @@ async def create(
sql_model = sql_model_map[model_type]
if isinstance(resource, Dataset):
await validate_data_categories(resource, db)
if sql_model in models_with_default_field and resource.is_default:
if isinstance(sql_model, ModelWithDefaultField) and resource.is_default:
raise errors.ForbiddenError(model_type, resource.fides_key)
return await create_resource(sql_model, resource.dict(), db)

Expand Down
2 changes: 1 addition & 1 deletion src/fides/api/db/samples.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import yaml
from expandvars import expandvars # type: ignore
from fideslang import Taxonomy
from fideslang.models import Taxonomy
from fideslang.validation import FidesKey

# DEFER: This can be changed to importlib.resources once we drop support for Python 3.8
Expand Down
2 changes: 1 addition & 1 deletion src/fides/api/db/seed.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"""
from typing import Dict, List, Optional

from fideslang import DEFAULT_TAXONOMY
from fideslang.default_taxonomy import DEFAULT_TAXONOMY
from loguru import logger as log
from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy.orm import Session
Expand Down
27 changes: 17 additions & 10 deletions src/fides/api/db/system.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,20 +146,27 @@ async def upsert_privacy_declarations(

async def upsert_cookies(
async_session: AsyncSession,
cookies: Optional[List[Dict]], # CookieSchema
cookies: Optional[List[Dict]],
privacy_declaration: PrivacyDeclaration,
system: System,
) -> None:
"""Upsert cookies for the given privacy declaration: retrieve cookies by name/system/privacy declaration
"""
Upsert cookies for the given privacy declaration: retrieve cookies
by name/system/privacy declaration.

Remove any existing cookies that aren't specified here.
"""
cookie_list: List[CookieSchema] = cookies or []
ThomasLaPiana marked this conversation as resolved.
Show resolved Hide resolved
for cookie_data in cookie_list:

parsed_cookies = (
[CookieSchema.parse_obj(cookie) for cookie in cookies] if cookies else []
)

for cookie_data in parsed_cookies:
# Check if cookie exists for this name/system/privacy declaration
result = await async_session.execute(
select(Cookies).where(
and_(
Cookies.name == cookie_data["name"],
Cookies.name == cookie_data.name,
Cookies.system_id == system.id,
Cookies.privacy_declaration_id == privacy_declaration.id,
)
Expand All @@ -168,16 +175,16 @@ async def upsert_cookies(
row: Optional[Cookies] = result.scalars().first()
if row:
await async_session.execute(
update(Cookies).where(Cookies.id == row.id).values(cookie_data)
update(Cookies).where(Cookies.id == row.id).values(cookie_data.dict())
)

else:
await async_session.execute(
insert(Cookies).values(
{
"name": cookie_data.get("name"),
"path": cookie_data.get("path"),
"domain": cookie_data.get("domain"),
"name": cookie_data.name,
"path": cookie_data.path,
"domain": cookie_data.domain,
"privacy_declaration_id": privacy_declaration.id,
"system_id": system.id,
}
Expand All @@ -188,7 +195,7 @@ async def upsert_cookies(
delete_result = await async_session.execute(
select(Cookies).where(
and_(
Cookies.name.notin_([cookie["name"] for cookie in cookie_list]),
Cookies.name.notin_([cookie.name for cookie in parsed_cookies]),
Cookies.system_id == system.id,
Cookies.privacy_declaration_id == privacy_declaration.id,
)
Expand Down
6 changes: 3 additions & 3 deletions src/fides/api/models/datasetconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@

return dataset_graph

def get_dataset_with_stubbed_collection(self) -> Dataset:
def get_dataset_with_stubbed_collection(self) -> GraphDataset:
"""
Return a Dataset with a single mock Collection for use in building a graph
where we only want one node per dataset, instead of one node per collection. Note that
Expand All @@ -179,7 +179,7 @@
is that this single node represents the overall Dataset, and will execute Dataset-level requests,
not Collection-level requests.
"""
dataset_graph: Dataset = self.get_graph()
dataset_graph = self.get_graph()

Check warning on line 182 in src/fides/api/models/datasetconfig.py

View check run for this annotation

Codecov / codecov/patch

src/fides/api/models/datasetconfig.py#L182

Added line #L182 was not covered by tests
stubbed_collection = Collection(name=dataset_graph.name, fields=[], after=set())

dataset_graph.collections = [stubbed_collection]
Expand Down Expand Up @@ -256,7 +256,7 @@
data_categories=field.data_categories,
identity=identity,
data_type_name=data_type_name, # type: ignore
references=references,
references=references, # type: ignore
is_pk=is_pk,
length=length,
is_array=is_array,
Expand Down
2 changes: 1 addition & 1 deletion src/fides/api/models/policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from enum import Enum as EnumType
from typing import Any, Dict, List, Optional, Tuple, Union

from fideslang import DEFAULT_TAXONOMY
from fideslang.default_taxonomy import DEFAULT_TAXONOMY
from fideslang.models import DataCategory as FideslangDataCategory
from fideslang.validation import FidesKey
from sqlalchemy import Column
Expand Down
3 changes: 1 addition & 2 deletions src/fides/api/models/privacy_notice.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,7 @@ def generate_notice_key(cls, name: Optional[str]) -> FidesKey:
if not isinstance(name, str):
raise Exception("Privacy notice keys must be generated from a string.")
notice_key: str = re.sub(r"\s+", "_", name.lower().strip())
FidesKey.validate(notice_key)
return notice_key
return FidesKey(FidesKey.validate(notice_key))

def dry_update(self, *, data: dict[str, Any]) -> FidesBase:
"""
Expand Down
10 changes: 5 additions & 5 deletions src/fides/api/models/sql_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
from sqlalchemy.orm import Session, relationship
from sqlalchemy.sql import func
from sqlalchemy.sql.sqltypes import DateTime
from typing_extensions import Protocol, runtime_checkable

from fides.api.common_exceptions import KeyOrNameAlreadyExists
from fides.api.db.base_class import Base
Expand Down Expand Up @@ -551,11 +552,10 @@ class SystemScans(Base):
"evaluation": Evaluation,
}

models_with_default_field = [
sql_model
for _, sql_model in sql_model_map.items()
if hasattr(sql_model, "is_default")
]

@runtime_checkable
class ModelWithDefaultField(Protocol):
is_default: bool


class AllowedTypes(str, EnumType):
Expand Down
12 changes: 8 additions & 4 deletions src/fides/api/oauth/system_manager_oauth_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ async def verify_oauth_client_for_system_from_request_body(
authorization: str = Security(oauth2_scheme),
db: Session = Depends(get_db),
system_auth_data: SystemAuthContainer = Depends(_get_system_from_request_body),
) -> SystemSchema:
) -> Union[str, System]:
"""
Verifies that the access token provided in the authorization header contains the necessary scopes to be
able to access the System found in the *request body*
Expand All @@ -88,12 +88,14 @@ async def verify_oauth_client_for_system_from_request_body(

Yields a 403 forbidden error if not.
"""
return has_system_permissions(

system = has_system_permissions(
system_auth_data=system_auth_data,
authorization=authorization,
security_scopes=security_scopes,
db=db,
)
return system


async def verify_oauth_client_for_system_from_fides_key(
Expand All @@ -111,20 +113,22 @@ async def verify_oauth_client_for_system_from_fides_key(

Yields a 403 forbidden error if not.
"""
return has_system_permissions(
system = has_system_permissions(
system_auth_data=system_auth_data,
authorization=authorization,
security_scopes=security_scopes,
db=db,
)
assert isinstance(system, str)
return system


def has_system_permissions(
system_auth_data: SystemAuthContainer,
authorization: str,
security_scopes: SecurityScopes,
db: Session,
) -> Union[str, SystemSchema]:
) -> Union[str, System]:
"""
Helper method that verifies that the token has the proper permissions to access the system(s).

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import abc
from typing import Any, Dict, List, Type

from fideslang import FidesDatasetReference
from fideslang.models import FidesDatasetReference
from pydantic import BaseModel, Extra, Field, PrivateAttr, create_model, root_validator
from pydantic.fields import FieldInfo
from sqlalchemy.orm import Session
Expand Down
12 changes: 7 additions & 5 deletions src/fides/api/schemas/dataset.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Any, List, Optional, Type
from typing import Any, List, Optional, Sequence, Type

from fideslang.models import Dataset, DatasetCollection, DatasetField
from fideslang.validation import FidesKey
Expand All @@ -25,7 +25,9 @@ def validate_data_categories_against_db(
logger.info(
"No data categories in the database: reverting to default data categories."
)
defined_data_categories = list(DefaultTaxonomyDataCategories.__members__.keys())
defined_data_categories = [
FidesKey(key) for key in DefaultTaxonomyDataCategories.__members__.keys()
]

class DataCategoryValidationMixin(BaseModel):
@validator("data_categories", check_fields=False, allow_reuse=True)
Expand All @@ -36,17 +38,17 @@ def valid_data_categories(
return _valid_data_categories(v, defined_data_categories)

class FieldDataCategoryValidation(DatasetField, DataCategoryValidationMixin):
fields: Optional[List["FieldDataCategoryValidation"]]
fields: Optional[List["FieldDataCategoryValidation"]] # type: ignore[assignment]

FieldDataCategoryValidation.update_forward_refs()

class CollectionDataCategoryValidation(
DatasetCollection, DataCategoryValidationMixin
):
fields: List[FieldDataCategoryValidation] = []
fields: Sequence[FieldDataCategoryValidation] = [] # type: ignore[assignment]

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required due to Lists in Python being invariant, as a result of their mutability

class DatasetDataCategoryValidation(Dataset, DataCategoryValidationMixin):
collections: List[CollectionDataCategoryValidation]
collections: Sequence[CollectionDataCategoryValidation] # type: ignore[assignment]

DatasetDataCategoryValidation(**dataset.dict())

Expand Down
4 changes: 2 additions & 2 deletions src/fides/api/schemas/messaging/messaging.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from enum import Enum
from typing import Any, Dict, List, Optional, Tuple, Type, Union

from fideslang import DEFAULT_TAXONOMY
from fideslang.default_taxonomy import DEFAULT_TAXONOMY
from fideslang.validation import FidesKey
from pydantic import BaseModel, Extra, root_validator

Expand Down Expand Up @@ -126,7 +126,7 @@ def transform_data_use_format(cls, values: Dict[str, Any]) -> Dict[str, Any]:
preference.data_use = next(
(
data_use.name
for data_use in DEFAULT_TAXONOMY.data_use
for data_use in DEFAULT_TAXONOMY.data_use or []
if data_use.fides_key == preference.data_use
),
preference.data_use,
Expand Down
4 changes: 2 additions & 2 deletions src/fides/api/schemas/system.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from datetime import datetime
from typing import Any, Dict, List, Optional
from typing import Any, Dict, List, Optional, Sequence

from fideslang.models import Cookies, PrivacyDeclaration, System
from pydantic import Field
Expand Down Expand Up @@ -40,7 +40,7 @@ class SystemResponse(BasicSystemResponse):
System record. Attempting to return bulk results with this model can lead to n+1 query issues.
"""

privacy_declarations: List[PrivacyDeclarationResponse] = Field(
privacy_declarations: Sequence[PrivacyDeclarationResponse] = Field( # type: ignore[assignment]
description=PrivacyDeclarationResponse.__doc__,
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ def build_consent_dataset_graph(datasets: List[DatasetConfig]) -> DatasetGraph:
and saas_config.get("consent_requests")
):
consent_datasets.append(
dataset_config.get_dataset_with_stubbed_collection()
dataset_config.get_dataset_with_stubbed_collection() # type: ignore[arg-type, assignment]
)

return DatasetGraph(*consent_datasets)
Expand Down
4 changes: 2 additions & 2 deletions src/fides/api/util/connection_util.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import List, Optional, Union
from typing import List, Optional

from fastapi import Depends, HTTPException
from fideslang.validation import FidesKey
Expand Down Expand Up @@ -325,7 +325,7 @@ def connection_status(

connector = get_connector(connection_config)
try:
status: Union[ConnectionTestStatus, None] = connector.test_connection()
status: Optional[ConnectionTestStatus] = connector.test_connection()

except (ConnectionException, ClientUnsuccessfulException) as exc:
logger.warning(
Expand Down
2 changes: 1 addition & 1 deletion src/fides/api/util/data_category.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from enum import Enum as EnumType
from typing import List, Type

from fideslang import DEFAULT_TAXONOMY
from fideslang.default_taxonomy import DEFAULT_TAXONOMY
from fideslang.validation import FidesKey
from sqlalchemy.orm import Session

Expand Down
Loading
Loading