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

update TableRecord type annotation to accept None values #21866

Merged
merged 1 commit into from
May 15, 2024

Conversation

sryza
Copy link
Contributor

@sryza sryza commented May 15, 2024

Summary & Motivation

This reflects the runtime type-checking we're doing inside the constructor.

This was causing problems, because we were loading values from the DB that had None values and then an outer class that was a Pydantic model was using the type annotations to validate them, and erroring.

records.0.0.extension.bool
  Input should be a valid boolean [type=bool_type, input_value=None, input_type=NoneType]
    For further information visit https://errors.pydantic.dev/2.6/v/bool_type

How I Tested These Changes

@alangenfeld
Copy link
Member

would be good to add a test that would have caught this

@alangenfeld
Copy link
Member

outer class that was a Pydantic model was using the type annotations to validate them

can use InstanceOf at the pydantic -> NT barrier to prevent shenanigans https://github.com/dagster-io/dagster/pull/21702/files

@sryza
Copy link
Contributor Author

sryza commented May 15, 2024

would be good to add a test that would have caught this

I started to do this but it felt a little goofy. I.e. it would basically be:

  • Construct one of these objects with a None value
  • Stick it inside a TableMetadataValue

Which is not a a hard test to write, but I think I'd scratch my head a bit if I came across it - wondering why we don't have some version of this for every place we're checking types, which feels a little absurd.

Maybe I'm overthinking it.

@sryza sryza merged commit 5a41b22 into master May 15, 2024
1 check failed
@sryza sryza deleted the table-record-none branch May 15, 2024 17:52
alangenfeld pushed a commit that referenced this pull request May 15, 2024
## Summary & Motivation

This reflects the runtime type-checking we're doing inside the
constructor.

This was causing problems, because we were loading values from the DB
that had `None` values and then an outer class that was a Pydantic model
was using the type annotations to validate them, and erroring.

```
records.0.0.extension.bool
  Input should be a valid boolean [type=bool_type, input_value=None, input_type=NoneType]
    For further information visit https://errors.pydantic.dev/2.6/v/bool_type
```

## How I Tested These Changes
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.

None yet

2 participants