Skip to content

Conversation

@sgomezvillamor
Copy link
Contributor

@sgomezvillamor sgomezvillamor commented Oct 20, 2025

Migrate deprecated Pydantic V1 patterns to V2:

  • @validator@field_validator
  • @root_validator@model_validator
  • .dict().model_dump()

Banned APIs to prevent those APIs being used again.

@github-actions github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label Oct 20, 2025
schema_pattern: Optional[AllowDenyPattern] = values.get("schema_pattern")
@model_validator(mode="after")
def check_database_is_set(self) -> "RedshiftConfig":
assert self.database, "database must be set"
Copy link

@aikido-pr-checks aikido-pr-checks bot Oct 20, 2025

Choose a reason for hiding this comment

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

Dangerous use of assert - low severity
When running Python in production in optimized mode, assert calls are not executed. This mode is enabled by setting the PYTHONOPTIMIZE command line flag. Optimized mode is usually ON in production. Any safety check done using assert will not be executed.

Remediation: Raise an exception instead of using assert.
View details in Aikido Security

@alwaysmeticulous
Copy link

alwaysmeticulous bot commented Oct 20, 2025

🔴 Meticulous spotted visual differences in 2 of 1093 screens tested: view and approve differences detected.

Meticulous evaluated ~8 hours of user flows against your PR.

Last updated for commit 6d0d821. This comment will update as new commits are pushed.

Comment on lines +65 to 67
assert abs(delta) >= get_bucket_duration_delta(bucket_duration), (
"Relative start time should be in terms of configured bucket duration. e.g '-2 days' or '-2 hours'."
)
Copy link

@aikido-pr-checks aikido-pr-checks bot Oct 21, 2025

Choose a reason for hiding this comment

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

Dangerous use of assert - low severity
When running Python in production in optimized mode, assert calls are not executed. This mode is enabled by setting the PYTHONOPTIMIZE command line flag. Optimized mode is usually ON in production. Any safety check done using assert will not be executed.

Remediation: Raise an exception instead of using assert.
View details in Aikido Security

@sgomezvillamor sgomezvillamor changed the title fix: Remove Pydantic V1 deprecation warnings chore: Remove Pydantic V1 deprecation warnings Oct 22, 2025
Comment on lines 86 to 88
assert max_num_fields_to_profile is None, (
f"{max_num_fields_to_profile_key} should be set to None"
"max_number_of_fields_to_profile should be set to None"
)
Copy link

@aikido-pr-checks aikido-pr-checks bot Oct 22, 2025

Choose a reason for hiding this comment

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

Dangerous use of assert - low severity
When running Python in production in optimized mode, assert calls are not executed. This mode is enabled by setting the PYTHONOPTIMIZE command line flag. Optimized mode is usually ON in production. Any safety check done using assert will not be executed.

Remediation: Raise an exception instead of using assert.
View details in Aikido Security

Comment on lines 86 to 88
assert max_num_fields_to_profile is None, (
f"{max_num_fields_to_profile_key} should be set to None"
"max_number_of_fields_to_profile should be set to None"
)
Copy link

@aikido-pr-checks aikido-pr-checks bot Oct 22, 2025

Choose a reason for hiding this comment

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

Dangerous use of assert - low severity
When running Python in production in optimized mode, assert calls are not executed. This mode is enabled by setting the PYTHONOPTIMIZE command line flag. Optimized mode is usually ON in production. Any safety check done using assert will not be executed.

Remediation: Raise an exception instead of using assert.
View details in Aikido Security

@sgomezvillamor sgomezvillamor self-assigned this Oct 23, 2025
@sgomezvillamor sgomezvillamor marked this pull request as ready for review October 23, 2025 10:26
@datahub-cyborg datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Oct 23, 2025
Copy link
Collaborator

@jayacryl jayacryl left a comment

Choose a reason for hiding this comment

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

nice!

@datahub-cyborg datahub-cyborg bot added pending-submitter-merge and removed needs-review Label for PRs that need review from a maintainer. labels Oct 24, 2025
raise ValueError(f"Output port {v} is not an urn: {e}") from e
@field_validator("assets")
@classmethod
def assets_must_be_urns(cls, v):
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing type hints for params and return types, similar with output_ports_must_be_urns

raise ValueError(f"Output port {v} is not in asset list")
return v
@model_validator(mode="after")
def output_ports_must_be_from_asset_list(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

We should name the validator for better error messaging

raise ValueError(f"asset {v} is not an urn: {e}") from e
return v

@field_validator("output_ports")
Copy link
Contributor

Choose a reason for hiding this comment

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

each_item is deprecated -> the migration doc takes about Annotated metadata for better Type reuse. Not sure if we could apply here: https://docs.pydantic.dev/latest/migration/#changes-to-validators

return full_name
else:
return v
@model_validator(mode="after")
Copy link
Contributor

Choose a reason for hiding this comment

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

always=True -> Field(validate_default=True) as per the migration doc https://docs.pydantic.dev/latest/migration/#changes-to-validators

Copy link
Contributor

@askumar27 askumar27 left a comment

Choose a reason for hiding this comment

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

LGTM! Left some minor comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ingestion PR or Issue related to the ingestion of metadata pending-submitter-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants