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

Add DatasetFieldBase model #49

Merged
merged 3 commits into from
Jun 6, 2022
Merged

Add DatasetFieldBase model #49

merged 3 commits into from
Jun 6, 2022

Conversation

sanders41
Copy link
Contributor

@sanders41 sanders41 commented Jun 5, 2022

Closes

Code Changes

  • Add a DatasetFieldBase modes.
  • Change DatasetField to inherit from DatasetFieldBase.
  • Import DatasetFieldBase in __init__.py.

Steps to Confirm

  • Run the test suite
  • In the fidesops repository, modify FidesopsDatasetField to the following and run the test suite.
class FidesopsDatasetField(FideslangDatasetFieldBase):
    """Extends fideslang DatasetField model with additional Fidesops annotations"""

    fidesops_meta: Optional[FidesopsMeta]
    fields: Optional[List["FidesopsDatasetField"]] = []

    @validator("data_categories")
    def valid_data_categories(
        cls, v: Optional[List[FidesOpsKey]]
    ) -> Optional[List[FidesOpsKey]]:
        """Validate that all annotated data categories exist in the taxonomy"""
        return _valid_data_categories(v)

    @validator("fidesops_meta")
    def valid_meta(cls, meta_values: Optional[FidesopsMeta]) -> Optional[FidesopsMeta]:
        """Validate upfront that the return_all_elements flag can only be specified on array fields"""
        if not meta_values:
            return meta_values

        is_array: bool = bool(
            meta_values.data_type and meta_values.data_type.endswith("[]")
        )
        if not is_array and meta_values.return_all_elements is not None:
            raise ValueError(
                "The 'return_all_elements' attribute can only be specified on array fields."
            )
        return meta_values

    @validator("fields")
    def validate_object_fields(
        cls,
        fields: Optional[List["FidesopsDatasetField"]],
        values: Dict[str, Any],
    ) -> Optional[List["FidesopsDatasetField"]]:
        """Two validation checks for object fields:
        - If there are sub-fields specified, type should be either empty or 'object'
        - Additionally object fields cannot have data_categories.
        """
        declared_data_type = None

        if values.get("fidesops_meta"):
            declared_data_type = values["fidesops_meta"].data_type

        if fields and declared_data_type:
            data_type, _ = parse_data_type_string(declared_data_type)
            if data_type != "object":
                raise InvalidDataTypeValidationError(
                    f"The data type {data_type} is not compatible with specified sub-fields."
                )

        if (fields or declared_data_type == "object") and values.get("data_categories"):
            raise ValueError(
                "Object fields cannot have specified data_categories. Specify category on sub-field instead"
            )

        return fields

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

This relates to the issue discussed here where fidesops_meta can't be added to the fideslang DatasetField. I tried several different options, and this was the only one that successfully passed the fidesops test suite.

Other things tried:

  1. Added anfidesops_meta field to DatasetField with a FidesopsMeta type. This didn't work because doing this would require fideslang to import fidesops.

I tried multiple iterations of the following ideas and they all either failed the fidesops test suite, or caused Pydantic to error.

  1. Added fidesops_meta to DatasetField making the field that requires fidesops to be imported a generic in order to avoid the import.
  2. Added fidesops_meta to DatasetField making the whole field generic and controlling things from fidesops.
  3. Added fidesops_meta to DatasetField making the whole field generic and controlling things from fidesops. In addition to this, made the fields type in DatasetField Optional[List[Generic[T]]].
  4. Added fidesops_meta to DatasetField making the whole field generic and controlling things from fidesops. In addition to this, made the fields type in DatasetField Optional[List[Generic[T]]]. Then only added validators to the inheriting class with no extra fields.
  5. Made the fields type in DatasetField Optional[List[Generic[T]]], and only had fidesops_meta in the inheriting model.

@sanders41 sanders41 requested a review from a team June 5, 2022 19:22
@seanpreston seanpreston self-assigned this Jun 6, 2022
Copy link
Contributor

@seanpreston seanpreston left a comment

Choose a reason for hiding this comment

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

This is a nice solution in the end, and respectful of ethyca/fidesops#454 (comment).

@sanders41 sanders41 merged commit 383cb31 into main Jun 6, 2022
@sanders41 sanders41 deleted the dataset-field-base branch June 6, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants