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: required fields in nested pydantic models need to follow parent nullability #2199

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

joscha
Copy link
Contributor

@joscha joscha commented Jan 8, 2025

Description

Fields of nested pydantic models that are not kept as JSON via skip_nested_types=True need to follow the "requiredness" of their parent.

Example:

class A(BaseModel):
  id: int

class B(BaseModel):
  id: int
  child_a: Optional[A]
  dlt_config: ClassVar[DltConfig] = {"skip_nested_types": True}

results in a table that looks like this:

table B:

name nullable
id false
child_a__id false

which is incorrect as I can serialize an instance of B that does not have a child_a, and thus, by extension, no id for child_a.

The change in this pull request retains the nullability of the field that contains the nested model, effectively making any required field of a nested, flattened model nullable as well if the parent model field is optional, i.e. the resulting table with the change in this PR is:

table B:

name nullable
id false
child_a__id true

If we agree this is a bug and the fix in this PR is the way to go I can add a test & fix for deeply nested models as well.

Copy link

netlify bot commented Jan 8, 2025

Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit a4789c4
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/677eaa9173bd080008bc9705

@sh-rp
Copy link
Collaborator

sh-rp commented Jan 9, 2025

@rudolfix I think it makes sense to forward the nullability to flattened fields, wdyt?

@rudolfix rudolfix requested a review from sh-rp January 14, 2025 14:42
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.

2 participants