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

Validate component children #1647

Merged
merged 5 commits into from
Aug 23, 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
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 36 additions & 14 deletions reflex/components/component.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ class Component(Base, ABC):

# components that cannot be children
invalid_children: List[str] = []

# components that are only allowed as children
valid_children: List[str] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also thinking in a follow up if these should maybe be List[Type[Component]] rather than str


# custom attribute
custom_attrs: Dict[str, str] = {}

Expand Down Expand Up @@ -103,9 +107,10 @@ def __init__(self, *args, **kwargs):
TypeError: If an invalid prop is passed.
"""
# Set the id and children initially.
children = kwargs.get("children", [])
initial_kwargs = {
"id": kwargs.get("id"),
"children": kwargs.get("children", []),
"children": children,
**{
prop: Var.create(kwargs[prop])
for prop in self.get_initial_props()
Expand All @@ -114,6 +119,8 @@ def __init__(self, *args, **kwargs):
}
super().__init__(**initial_kwargs)

self._validate_component_children(children)

# Get the component fields, triggers, and props.
fields = self.get_fields()
triggers = self.get_triggers()
Expand Down Expand Up @@ -381,6 +388,7 @@ def create(cls, *children, **props) -> Component:
else Bare.create(contents=Var.create(child, is_string=True))
for child in children
]

return cls(children=children, **props)

def _add_style(self, style):
Expand Down Expand Up @@ -435,30 +443,44 @@ def render(self) -> Dict:
),
autofocus=self.autofocus,
)
self._validate_component_children(
Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep the check here? Only reason is because if they somehow added children dynamically after creating their component, then we wouldn't catch it. Though I don't think I've seen people really do that...

rendered_dict["name"], rendered_dict["children"]
)
return rendered_dict

def _validate_component_children(self, comp_name: str, children: List[Dict]):
def _validate_component_children(self, children: List[Component]):
"""Validate the children components.

Args:
comp_name: name of the component.
children: list of children components.
children: The children of the component.

Raises:
ValueError: when an unsupported component is matched.
"""
if not self.invalid_children:
if not self.invalid_children and not self.valid_children:
return
for child in children:
name = child["name"]
if name in self.invalid_children:

comp_name = type(self).__name__

def validate_invalid_child(child_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we could inline these functions ore define them outside of the function. I've noticed before in Python that defining functions within tight loops is often a performance issue, and these checks will be called many times per compile.

if child_name in self.invalid_children:
raise ValueError(
f"The component `{comp_name.lower()}` cannot have `{name.lower()}` as a child component"
f"The component `{comp_name}` cannot have `{child_name}` as a child component"
)

def validate_valid_child(child_name):
if child_name not in self.valid_children:
valid_child_list = ", ".join(
[f"`{v_child}`" for v_child in self.valid_children]
)
raise ValueError(
f"The component `{comp_name}` only allows the components: {valid_child_list} as children. Got `{child_name}` instead."
)

for child in children:
name = type(child).__name__

if self.invalid_children:
validate_invalid_child(name)

if self.valid_children:
validate_valid_child(name)

def _get_custom_code(self) -> Optional[str]:
"""Get custom code for the component.

Expand Down
4 changes: 4 additions & 0 deletions reflex/components/forms/button.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""A button component."""
from typing import List

from reflex.components.libs.chakra import ChakraComponent
from reflex.vars import Var
Expand Down Expand Up @@ -42,6 +43,9 @@ class Button(ChakraComponent):
# The type of button.
type_: Var[str]

# Components that are not allowed as children.
invalid_children: List[str] = ["Button", "MenuButton"]


class ButtonGroup(ChakraComponent):
"""A group of buttons."""
Expand Down
5 changes: 4 additions & 1 deletion reflex/components/overlay/menu.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""Menu components."""

from typing import Set
from typing import List, Set

from reflex.components.component import Component
from reflex.components.libs.chakra import ChakraComponent
Expand Down Expand Up @@ -100,6 +100,9 @@ class MenuButton(ChakraComponent):
# The variant of the menu button.
variant: Var[str]

# Components that are not allowed as children.
invalid_children: List[str] = ["Button", "MenuButton"]

# The tag to use for the menu button.
as_: Var[str]

Expand Down
70 changes: 64 additions & 6 deletions tests/components/test_component.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,47 @@ def component5() -> Type[Component]:
"""

class TestComponent5(Component):
tag = "Tag"
tag = "RandomComponent"

invalid_children: List[str] = ["Text"]

valid_children: List[str] = ["Text"]

return TestComponent5


@pytest.fixture
def component6() -> Type[Component]:
"""A test component.

Returns:
A test component.
"""

class TestComponent6(Component):
tag = "RandomComponent"

invalid_children: List[str] = ["Text"]

return TestComponent6


@pytest.fixture
def component7() -> Type[Component]:
"""A test component.

Returns:
A test component.
"""

class TestComponent7(Component):
tag = "RandomComponent"

valid_children: List[str] = ["Text"]

return TestComponent7


@pytest.fixture
def on_click1() -> EventHandler:
"""A sample on click function.
Expand Down Expand Up @@ -461,16 +495,40 @@ def test_get_hooks_nested2(component3, component4):
)


def test_unsupported_child_components(component5):
"""Test that a value error is raised when an unsupported component is provided as a child.
@pytest.mark.parametrize("fixture", ["component5", "component6"])
def test_unsupported_child_components(fixture, request):
"""Test that a value error is raised when an unsupported component (a child component found in the
component's invalid children list) is provided as a child.

Args:
fixture: the test component as a fixture.
request: Pytest request.
"""
component = request.getfixturevalue(fixture)
with pytest.raises(ValueError) as err:
comp = component.create(rx.text("testing component"))
comp.render()
assert (
err.value.args[0]
== f"The component `{component.__name__}` cannot have `Text` as a child component"
)


@pytest.mark.parametrize("fixture", ["component5", "component7"])
def test_component_with_only_valid_children(fixture, request):
"""Test that a value error is raised when an unsupported component (a child component not found in the
component's valid children list) is provided as a child.

Args:
component5: the test component
fixture: the test component as a fixture.
request: Pytest request.
"""
component = request.getfixturevalue(fixture)
with pytest.raises(ValueError) as err:
comp = component5.create(rx.text("testing component"))
comp = component.create(rx.box("testing component"))
comp.render()
assert (
err.value.args[0]
== f"The component `tag` cannot have `text` as a child component"
== f"The component `{component.__name__}` only allows the components: `Text` as children. "
f"Got `Box` instead."
)