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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cardinality incorrect with typing.Optional typing.List for Pydantic dataclasses? #105

Closed
sdruskat opened this issue Feb 19, 2024 · 2 comments 路 Fixed by #106
Closed

Cardinality incorrect with typing.Optional typing.List for Pydantic dataclasses? #105

sdruskat opened this issue Feb 19, 2024 · 2 comments 路 Fixed by #106
Labels
bug Something isn't working
Milestone

Comments

@sdruskat
Copy link

sdruskat commented Feb 19, 2024

Hi 馃憢, and thanks for developing such a helpful project!

I'm wondering whether cardinality for optional lists is currently correctly defined as zero-or-one? I'm not very knowledgeable when it comes to ERDs, so this may be just wrong thinking on my side.

(This may be related to #90 as well.)

The following Pydantic model (two Child classes to avoid rendering issues) gives the ERD below, created by running erdantic my_module.Parent -d | dot -Tsvg > parent.svg. (pygraphviz 1.12)

from typing import List, Optional
from pydantic.dataclasses import dataclass

@dataclass
class Child1:
    something: str

@dataclass
class Child2:
    something: str

@dataclass
class Parent:
    required_list: List[Child1]
    optional_list: Optional[List[Child2]]

ERD for the parent model

I think the cardinality (0..*) at Child1 is correct: required_list is a list that can contain anything from zero to many instances of Child1.

However, the cardinality at Child2 (0..1) is wrong - I think? Following the logic derived from required_list (0..* instances of Child1 can be included in required_list), I would expect the same cardinality at Child2 as well, meaning 0..* instances of Child2 can be included in optional_list (regardless of whether the list must exist, i.e., the field must be defined on instantiation).

I haven't had a look at the source code yet, but would assume that perhaps Optional fields are processed without looking at the type of the wrapped value?

Thanks, and do please correct me if I got it wrong.

@jayqi
Copy link
Member

jayqi commented Feb 25, 2024

You are right. The current implementation of is_many only checks whether the outermost type is a container.

This actually surfaces a deeper issue, which is that our representation of manyness is wrong. Currently, it's set as a property of the field on the parent class, when it's really a property of the edge. For example, one field with the type annotation Union[Child1, List[Child2]] should be one-to-one with Child1 and one-to-many with Child2.

@jayqi jayqi added the bug Something isn't working label Feb 26, 2024
@jayqi jayqi added this to the v1.0 rewrite milestone Feb 26, 2024
@jayqi
Copy link
Member

jayqi commented Mar 31, 2024

This has been fixed in v1.0.0. This is temporarily only available as a pre-release version (pip install erdantic --pre).

Here are the test cases that cover this:

def test_is_collection_type_of():
class Target: ...
assert is_collection_type_of(typing.List[Target], Target)
assert is_collection_type_of(typing.Optional[typing.List[Target]], Target)
assert is_collection_type_of(typing.List[typing.Optional[Target]], Target)
assert is_collection_type_of(typing.Union[Target, typing.List[Target], None], Target)
assert not is_collection_type_of(Target, Target)
assert not is_collection_type_of(typing.Optional[Target], Target)
assert not is_collection_type_of(typing.Union[Target, int], Target)
assert not is_collection_type_of(typing.List[int], Target)
assert not is_collection_type_of(typing.Union[Target, typing.List[int]], Target)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants