Source Klaviyo: update schema validation in SAT #6952
Conversation
|
@gaart I don't think I understand why disable check is the best fix here, could you add more details? |
|
How does this fix the build? |
|
@keu @davinchia but get We can skip the schema validation test since pydantic does the same during response processing and the test will fail anyway if schema is wrong |
|
/test connector=connectors/source-klaviyo
|
@gaart I see. @classmethod
def schema_extra(cls, schema: Dict[str, Any], model: Type["BaseModel"]) -> None:
schema.pop("title", None)
schema.pop("description", None)
for name, prop in schema.get("properties", {}).items():
prop.pop("title", None)
prop.pop("description", None)
allow_none = model.__fields__[name].allow_none
if allow_none:
if "type" in prop:
prop["type"] = ["null", prop["type"]]
elif "$ref" in prop:
ref = prop.pop("$ref")
prop["oneOf"] = [{"type": "null"}, {"$ref": ref}]this will produce the expected schema.
@sherifnada what do you think, should we keep pydantic validation during the read in the connector code? |
|
also, the details aboive should be in the ticket itself, and the actual fix should be reflected in the PR title (i.e. Klaviyo: disable schema validation in SAT). |
gaart
changed the title
|
This feels like we should fix the base model and not disable the validation or it will get confusing if we have many connectors doing this differently. I will defer to Sherif. |
|
I agree with Davin. Disabling schema validation would be throwing out the baby with the bathwater |
|
/test connector=connectors/source-klaviyo
|
|
@keu @davinchia @sherifnada updated |
| prop.pop("title", None) | ||
| prop.pop("description", None) | ||
| allow_none = model.__fields__[name].allow_none |
There was a problem hiding this comment.
this should have a comment describing what is happening
gaart
changed the title
|
/test connector=connectors/source-klaviyo
|
github-actions
bot
added
the
area/documentation
|
/publish connector=connectors/source-klaviyo
|
|
/publish connector=connectors/source-klaviyo
|
|
/test connector=connectors/source-klaviyo
|
|
/test connector=connectors/source-klaviyo
|
|
@gaart pulling master should fix the issue |
…ytehq/airbyte into gaart/6815-source-klaviyo-fix-build
|
/test connector=connectors/source-klaviyo
|
|
/publish connector=connectors/source-klaviyo
|
* Disable schema validation for configured_catalog * Upd pydantic-generated schema * Upd comment * Bump version, upd changelog
What
Fix #6815
How
There's a test which checks response schemas. Some fields can be optional (null value). Usually we mark them like
["null", "string"]. In this connector we use pydantic which generates invalid type, e.g. forname: Optional[str]we expect to havebut get
We can skip the schema validation test since pydantic does the same during response processing and the test will fail anyway if schema is wrong.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing./publishcommand described here