-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
don't use _outer_type if we don't have to #4528
base: main
Are you sure you want to change the base?
Changes from 3 commits
c9fe2e1
c2a39b4
d3fbf12
3fb8450
9e96333
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
Callable, | ||
ClassVar, | ||
Dict, | ||
ForwardRef, | ||
FrozenSet, | ||
Iterable, | ||
List, | ||
|
@@ -269,6 +270,20 @@ def is_optional(cls: GenericType) -> bool: | |
return is_union(cls) and type(None) in get_args(cls) | ||
|
||
|
||
def true_type_for_pydantic_field(f: ModelField): | ||
"""Get the type for a pydantic field. | ||
|
||
Args: | ||
f: The field to get the type for. | ||
|
||
Returns: | ||
The type for the field. | ||
""" | ||
if not isinstance(f.annotation, (str, ForwardRef)): | ||
return f.annotation | ||
return f.outer_type_ | ||
|
||
|
||
def value_inside_optional(cls: GenericType) -> GenericType: | ||
"""Get the value inside an Optional type or the original type. | ||
|
||
|
@@ -283,6 +298,29 @@ def value_inside_optional(cls: GenericType) -> GenericType: | |
return cls | ||
|
||
|
||
def get_field_type(cls: GenericType, field_name: str) -> GenericType | None: | ||
"""Get the type of a field in a class. | ||
|
||
Args: | ||
cls: The class to check. | ||
field_name: The name of the field to check. | ||
|
||
Returns: | ||
The type of the field, if it exists, else None. | ||
""" | ||
if ( | ||
hasattr(cls, "__fields__") | ||
and field_name in cls.__fields__ | ||
and hasattr(cls.__fields__[field_name], "annotation") | ||
and not isinstance(cls.__fields__[field_name].annotation, (str, ForwardRef)) | ||
): | ||
return cls.__fields__[field_name].annotation | ||
type_hints = get_type_hints(cls) | ||
if field_name in type_hints: | ||
return type_hints[field_name] | ||
return None | ||
|
||
|
||
def get_property_hint(attr: Any | None) -> GenericType | None: | ||
"""Check if an attribute is a property and return its type hint. | ||
|
||
|
@@ -320,24 +358,9 @@ def get_attribute_access_type(cls: GenericType, name: str) -> GenericType | None | |
if hint := get_property_hint(attr): | ||
return hint | ||
|
||
if ( | ||
hasattr(cls, "__fields__") | ||
and name in cls.__fields__ | ||
and hasattr(cls.__fields__[name], "outer_type_") | ||
): | ||
if hasattr(cls, "__fields__") and name in cls.__fields__: | ||
# pydantic models | ||
field = cls.__fields__[name] | ||
type_ = field.outer_type_ | ||
if isinstance(type_, ModelField): | ||
type_ = type_.type_ | ||
if ( | ||
not field.required | ||
and field.default is None | ||
and field.default_factory is None | ||
): | ||
# Ensure frontend uses null coalescing when accessing. | ||
type_ = Optional[type_] | ||
return type_ | ||
Comment on lines
-329
to
-340
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm really hesitant about removing these lines, unless we're sure this is not going to be hit. @benedikt-bartscher do you know under what circumstances this bit of code gets used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. import reflex as rx
class M1(rx.Base):
foo: str = "foo"
class M2(rx.Base):
m1: M1 = None
class State(rx.State):
m2: M2 = M2()
def index() -> rx.Component:
return rx.container(
rx.text(State.m2.m1.foo),
)
app = rx.App()
app.add_page(index) This is at least one of the cases; default is Again, why pydantic allows the default to not match the annotation and pass validation i'm unsure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is interesting, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @masenf sorry, I did not see your question for some reason. But it seems like you found a case.
@adhami3310 you can use this to determine unset defaults from pydantic.v1 import BaseModel
class User(BaseModel):
no_default: str | None
none_default: str | None = None
no_default_field = User.__fields__["no_default"]
none_default_field = User.__fields__["none_default"]
print(no_default_field.field_info.default)
print(none_default_field.field_info.default) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. last time i checked, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean for default and default factory values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i made the check for default use None, there's little I can do with default_factory returning None (i hope no one thinks that's a good idea) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure this is a bad idea? diff --git a/reflex/state.py b/reflex/state.py
index e7e6bcf3..46ba3133 100644
--- a/reflex/state.py
+++ b/reflex/state.py
@@ -1099,7 +1099,10 @@ class BaseState(Base, ABC, extra=pydantic.Extra.allow):
if (
not field.required
and field.default is None
- and field.default_factory is None
+ and (
+ field.default_factory is None
+ or types.default_factory_can_be_none(field.default_factory)
+ )
and not types.is_optional(prop._var_type)
):
# Ensure frontend uses null coalescing when accessing.
diff --git a/reflex/utils/types.py b/reflex/utils/types.py
index b8bcbf2d..60eb456d 100644
--- a/reflex/utils/types.py
+++ b/reflex/utils/types.py
@@ -333,7 +333,10 @@ def get_attribute_access_type(cls: GenericType, name: str) -> GenericType | None
if (
not field.required
and field.default is None
- and field.default_factory is None
+ and (
+ field.default_factory is None
+ or default_factory_can_be_none(field.default_factory)
+ )
):
# Ensure frontend uses null coalescing when accessing.
type_ = Optional[type_]
@@ -893,3 +896,18 @@ def typehint_issubclass(possible_subclass: Any, possible_superclass: Any) -> boo
for provided_arg, accepted_arg in zip(provided_args, accepted_args)
if accepted_arg is not Any
)
+
+
+def default_factory_can_be_none(default_factory: Callable) -> bool:
+ """Check if the default factory can return None.
+
+ Args:
+ default_factory: The default factory to check.
+
+ Returns:
+ Whether the default factory can return None.
+ """
+ type_hints = get_type_hints(default_factory)
+ if hint := type_hints.get("return"):
+ return is_optional(hint)
+ return default_factory() is None There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. most default factories are lambdas so i would imagine you're not getting a type hint in most cases, calling it could give you should be a reliable indicator but i'm afraid of side effects or performance costs |
||
return get_field_type(cls, name) | ||
elif isinstance(cls, type) and issubclass(cls, DeclarativeBase): | ||
insp = sqlalchemy.inspect(cls) | ||
if name in insp.columns: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_type_hints
is a really slow function, it doesn't make sense to call it multiple times for the samecls
without some kind of caching.we should profile and see what affect this change may or may not have on component
__init__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea i was thinking the same, we could also be more specific when we use this vs the other one