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 issue when a model has a Literal #49

Merged
merged 6 commits into from
Apr 8, 2022

Conversation

swails
Copy link
Contributor

@swails swails commented Apr 7, 2022

A Literal is special in that the argument to the literal is the actual
collection of exact values that are permissible to that field.


Bug description. Start with a file named erdbug.py with the following contents:

from pydantic import BaseModel
from typing import Literal

class Species(BaseModel):
    name: str

class PigmentedFlower(BaseModel):
    color: str
    species: Species

class BlackFlower(PigmentedFlower):
    color: Literal["black"] = "black"

class BlueFlower(PigmentedFlower):
    color: Literal["blue"] = "blue"

If we run the following command:

$ PYTHONPATH=`pwd` erdantic -o diagram.png erdbug.PigmentedFlower erdbug.BlackFlower erdbug.BlueFlower erdbug.Species

We get the following error:

Traceback (most recent call last):
  File "/Applications/Anaconda/anaconda3/envs/foundry-dev/bin/erdantic", line 33, in <module>
    sys.exit(load_entry_point('erdantic', 'console_scripts', 'erdantic')())
  File "/Applications/Anaconda/anaconda3/envs/foundry-dev/lib/python3.8/site-packages/typer/main.py", line 214, in __call__
    return get_command(self)(*args, **kwargs)
  File "/Applications/Anaconda/anaconda3/envs/foundry-dev/lib/python3.8/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/Applications/Anaconda/anaconda3/envs/foundry-dev/lib/python3.8/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/Applications/Anaconda/anaconda3/envs/foundry-dev/lib/python3.8/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Applications/Anaconda/anaconda3/envs/foundry-dev/lib/python3.8/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/Applications/Anaconda/anaconda3/envs/foundry-dev/lib/python3.8/site-packages/typer/main.py", line 500, in wrapper
    return callback(**use_params)  # type: ignore
  File "/Users/swails/src/entos/erdantic/erdantic/cli.py", line 87, in main
    diagram = create(*model_classes, termini=termini_classes)
  File "/Users/swails/src/entos/erdantic/erdantic/erd.py", line 192, in create
    search_composition_graph(model=model, seen_models=seen_models, seen_edges=seen_edges)
  File "/Users/swails/src/entos/erdantic/erdantic/erd.py", line 245, in search_composition_graph
    raise StringForwardRefError(
erdantic.exceptions.StringForwardRefError: Forward reference 'black' for field 'color' on model 'BlackFlower' is a string literal and not a typing.ForwardRef object. erdantic is unable to handle forward references that aren't transformed into typing.ForwardRef. Declare explicitly with 'typing.ForwardRef("black", is_argument=False)'.

The problem is that it's trying to evaluate the value passed to the Literal as a forward reference if it's a string, when in reality Literal is a special typing utility to indicate that the variable carries a specific value.

After this PR, the following diagram is generated:

diagram

A `Literal` is special in that the argument to the literal is the actual
collection of exact values that are permissible to that field.
@jayqi jayqi self-requested a review April 7, 2022 02:23
Copy link
Member

@jayqi jayqi left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! One comment regarding an expected test failure when I enable the CI for this PR.

erdantic/typing.py Outdated Show resolved Hide resolved
@jayqi
Copy link
Member

jayqi commented Apr 7, 2022

It'd be great if you can also add a test case. Either to test_get_recursive_args or it's own test function.

https://github.com/drivendataorg/erdantic/blob/main/tests/test_typing.py

@juviwhale
Copy link

@swails Thanks for working on this, it is affecting me too

@swails
Copy link
Contributor Author

swails commented Apr 7, 2022

Test case added.

@swails
Copy link
Contributor Author

swails commented Apr 8, 2022

I finally broke down and installed a local python 3.6 interpreter and fixed the tests locally.

@jayqi
Copy link
Member

jayqi commented Apr 8, 2022

Thanks for your patience in getting through this. This is the first time GitHub's forced me to approve the GitHub Actions run on every update to the PR, and I think maybe there's some investigation for me to do on a better security–convenience balance in the settings.

Also thanks for your patience in dealing with Python 3.6. We're probably overdue on dropping support for it, so sorry for the extra work that has caused you.

@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #49 (27fa6c4) into main (80c95bd) will decrease coverage by 0.3%.
The diff coverage is 81.8%.

@@           Coverage Diff           @@
##            main     #49     +/-   ##
=======================================
- Coverage   98.0%   97.6%   -0.4%     
=======================================
  Files         13      13             
  Lines        504     514     +10     
=======================================
+ Hits         494     502      +8     
- Misses        10      12      +2     
Impacted Files Coverage Δ
erdantic/typing.py 97.6% <81.8%> (-2.4%) ⬇️

@swails
Copy link
Contributor Author

swails commented Apr 8, 2022

If I read the message right, then once my first PR has been merged, PRs I raise after that will run automatically. But it looks like all of the tests have now passed! Let me know if there's anything else you want before merging.

Copy link
Member

@jayqi jayqi left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for finding this issue and fixing.

@jayqi jayqi merged commit f0ec86c into drivendataorg:main Apr 8, 2022
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

3 participants