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

Metadata: Remove leading underscore #29024

Merged
merged 9 commits into from
Aug 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

from metadata_service.constants import METADATA_FILE_NAME, METADATA_FOLDER, ICON_FILE_NAME
from metadata_service.validators.metadata_validator import POST_UPLOAD_VALIDATORS, validate_and_load
from metadata_service.utils import to_json_sanitized_dict
from metadata_service.models.transform import to_json_sanitized_dict
from metadata_service.models.generated.ConnectorMetadataDefinitionV0 import ConnectorMetadataDefinitionV0

from dataclasses import dataclass
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@

from typing import Optional

from pydantic import BaseModel, Extra, Field
from pydantic import BaseModel, Extra
from typing_extensions import Literal


class AirbyteInternal(BaseModel):
class Config:
extra = Extra.allow

field_sl: Optional[Literal[100, 200, 300]] = Field(None, alias="_sl")
field_ql: Optional[Literal[100, 200, 300, 400, 500, 600]] = Field(None, alias="_ql")
sl: Optional[Literal[100, 200, 300]] = None
ql: Optional[Literal[100, 200, 300, 400, 500, 600]] = None
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ class AirbyteInternal(BaseModel):
class Config:
extra = Extra.allow

field_sl: Optional[Literal[100, 200, 300]] = Field(None, alias="_sl")
field_ql: Optional[Literal[100, 200, 300, 400, 500, 600]] = Field(None, alias="_ql")
sl: Optional[Literal[100, 200, 300]] = None
ql: Optional[Literal[100, 200, 300, 400, 500, 600]] = None


class JobTypeResourceLimit(BaseModel):
Expand Down Expand Up @@ -185,7 +185,7 @@ class Config:

class Data(BaseModel):
class Config:
extra = Extra.forbid
extra = Extra.allow
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't allowing extra risky? Forbidding extra is a nice way to have consistency guarantees across metadata files


name: str
icon: Optional[str] = None
Expand Down Expand Up @@ -224,7 +224,7 @@ class Config:
normalizationConfig: Optional[NormalizationDestinationDefinitionConfig] = None
suggestedStreams: Optional[SuggestedStreams] = None
resourceRequirements: Optional[ActorDefinitionResourceRequirements] = None
field_ab_internal: Optional[AirbyteInternal] = Field(None, alias="_ab_internal")
ab_internal: Optional[AirbyteInternal] = None


class ConnectorMetadataDefinitionV0(BaseModel):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ class AirbyteInternal(BaseModel):
class Config:
extra = Extra.allow

field_sl: Optional[Literal[100, 200, 300]] = Field(None, alias="_sl")
field_ql: Optional[Literal[100, 200, 300, 400, 500, 600]] = Field(None, alias="_ql")
sl: Optional[Literal[100, 200, 300]] = None
ql: Optional[Literal[100, 200, 300, 400, 500, 600]] = None


class JobTypeResourceLimit(BaseModel):
Expand Down Expand Up @@ -190,4 +190,4 @@ class Config:
)
allowedHosts: Optional[AllowedHosts] = None
releases: Optional[ConnectorReleases] = None
field_ab_internal: Optional[AirbyteInternal] = Field(None, alias="_ab_internal")
ab_internal: Optional[AirbyteInternal] = None
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ class AirbyteInternal(BaseModel):
class Config:
extra = Extra.allow

field_sl: Optional[Literal[100, 200, 300]] = Field(None, alias="_sl")
field_ql: Optional[Literal[100, 200, 300, 400, 500, 600]] = Field(None, alias="_ql")
sl: Optional[Literal[100, 200, 300]] = None
ql: Optional[Literal[100, 200, 300, 400, 500, 600]] = None


class JobTypeResourceLimit(BaseModel):
Expand Down Expand Up @@ -179,4 +179,4 @@ class Config:
description="Number of seconds allowed between 2 airbyte protocol messages. The source will timeout if this delay is reach",
)
releases: Optional[ConnectorReleases] = None
field_ab_internal: Optional[AirbyteInternal] = Field(None, alias="_ab_internal")
ab_internal: Optional[AirbyteInternal] = None
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ class AirbyteInternal(BaseModel):
class Config:
extra = Extra.allow

field_sl: Optional[Literal[100, 200, 300]] = Field(None, alias="_sl")
field_ql: Optional[Literal[100, 200, 300, 400, 500, 600]] = Field(None, alias="_ql")
sl: Optional[Literal[100, 200, 300]] = None
ql: Optional[Literal[100, 200, 300, 400, 500, 600]] = None


class SuggestedStreams(BaseModel):
Expand Down Expand Up @@ -197,7 +197,7 @@ class Config:
description="Number of seconds allowed between 2 airbyte protocol messages. The source will timeout if this delay is reach",
)
releases: Optional[ConnectorReleases] = None
field_ab_internal: Optional[AirbyteInternal] = Field(None, alias="_ab_internal")
ab_internal: Optional[AirbyteInternal] = None


class ConnectorRegistryDestinationDefinition(BaseModel):
Expand Down Expand Up @@ -244,7 +244,7 @@ class Config:
)
allowedHosts: Optional[AllowedHosts] = None
releases: Optional[ConnectorReleases] = None
field_ab_internal: Optional[AirbyteInternal] = Field(None, alias="_ab_internal")
ab_internal: Optional[AirbyteInternal] = None


class ConnectorRegistryV0(BaseModel):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ description: Fields for internal use only
type: object
additionalProperties: true
properties:
_sl:
sl:
type: integer
enum:
- 100
- 200
- 300
_ql:
ql:
type: integer
enum:
- 100
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ properties:
- githubIssueLabel
- connectorSubtype
- releaseStage
additionalProperties: false
additionalProperties: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we allowing additionalProperties now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it to allow _ prefixes? I'm not sure I get why we want to continue allowing _ prefix and I'm afraid enabling additionalProperties weakens the validation and can lead to sneaky errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So Were in a messy middle after discovering the pydantic model generated did not like the leading underscore.

The immediate fix(s) we deployed

  1. Allowed prerealease to use the alias when writing the prerelease metadata file
  2. Updated the additionalProperties to true for the registry to handle a parsing error resulting from above

Now going forward we've got a few metadata files out there that have _ab_internal when they should have ab_internal
This is why im setting additionalProperties to true, just so things dont blow up when rolling these new fields out, and in the next version of the metadata we can revert it back to false

properties:
name:
type: string
Expand Down Expand Up @@ -105,5 +105,5 @@ properties:
"$ref": SuggestedStreams.yaml
resourceRequirements:
"$ref": ActorDefinitionResourceRequirements.yaml
_ab_internal:
ab_internal:
"$ref": AirbyteInternal.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,5 @@ properties:
"$ref": AllowedHosts.yaml
releases:
"$ref": ConnectorReleases.yaml
_ab_internal:
ab_internal:
"$ref": AirbyteInternal.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,5 @@ properties:
type: integer
releases:
"$ref": ConnectorReleases.yaml
_ab_internal:
ab_internal:
"$ref": AirbyteInternal.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import json
from pydantic import BaseModel

def _apply_default_pydantic_kwargs(kwargs: dict) -> dict:
"""A helper function to apply default kwargs to pydantic models.

Args:
kwargs (dict): the kwargs to apply

Returns:
dict: the kwargs with defaults applied
"""
default_kwargs = {
"by_alias": True, # Ensure that the original field name from the jsonschema is used in the event it begins with an underscore (e.g. ab_internal)
"exclude_none": True, # Exclude fields that are None
}

return {**default_kwargs, **kwargs}

def to_json_sanitized_dict(pydantic_model_obj: BaseModel, **kwargs) -> dict:
"""A helper function to convert a pydantic model to a sanitized dict.

Without this pydantic dictionary may contain values that are not JSON serializable.

Args:
pydantic_model_obj (BaseModel): a pydantic model

Returns:
dict: a sanitized dictionary
"""

return json.loads(to_json(pydantic_model_obj, **kwargs))

def to_json(pydantic_model_obj: BaseModel, **kwargs) -> str:
"""A helper function to convert a pydantic model to a json string.

Without this pydantic dictionary may contain values that are not JSON serializable.

Args:
pydantic_model_obj (BaseModel): a pydantic model

Returns:
str: a json string
"""
kwargs = _apply_default_pydantic_kwargs(kwargs)

return pydantic_model_obj.json(**kwargs)

def to_dict(pydantic_model_obj: BaseModel, **kwargs) -> dict:
"""A helper function to convert a pydantic model to a dict.

Without this pydantic dictionary may contain values that are not JSON serializable.

Args:
pydantic_model_obj (BaseModel): a pydantic model

Returns:
dict: a dict
"""
kwargs = _apply_default_pydantic_kwargs(kwargs)

return pydantic_model_obj.dict(**kwargs)

This file was deleted.

Loading