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

Allow nullable foreign keys #12

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

idan-david
Copy link
Contributor

In the case of a model with a nullable foreign key to another model the from_django method does not currently work.

I've added a check for Optional types in the dict method, which extracts the correct type

class A(models.Model):
    value = models.CharField(max_length=256, null=True, blank=True)


class B(models.Model):
    a = models.ForeignKey(A, null=True, blank=True, on_delete=models.CASCADE)

djantic/main.py Outdated Show resolved Hide resolved
tests/testapp/models.py Outdated Show resolved Hide resolved
djantic/main.py Outdated
@@ -2,6 +2,7 @@
from enum import Enum
from functools import reduce
from itertools import chain
from types import UnionType
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like UnionType doesn't exist in python 3.8 and 3.9 either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. After some reading, UnionType was introduced in 3.10, with the "|" annotation (e.g. int | str).
I can make the tests compatible for python 3.8 by removing this syntax, but still supporting this check (with an import only if sys.version_info >= (3,10)).
This means I can support this syntax, but not check for it in a test.
What do you suggest we do?

if sys.version_info >= (3, 10):
from types import UnionType
else:
from typing import Union as UnionType
Copy link
Owner

Choose a reason for hiding this comment

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

@idan-david This looks ok to me. The import on line 6 needs to be removed though. Let's see if the tests pass after this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the mess, removed the import on line 6

(get_origin(annotation) is Union or get_origin(annotation) is UnionType)
and type(None) in args
and len(args) == 2
and any(inspect.isclass(arg) and issubclass(arg, ModelSchema) for arg in args)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing problems with python 3.9, added the inspect.isclass(arg)

@jonathan-s jonathan-s changed the title BUGFIX: allow nullable FKS Allow nullable foreign keys Jun 27, 2024
@jonathan-s jonathan-s merged commit e23cce7 into jonathan-s:main Jun 27, 2024
5 checks passed
@jonathan-s
Copy link
Owner

@idan-david 🎉 thanks for taking the time

@idan-david idan-david deleted the bugfix/nullable-fks branch June 27, 2024 13:41
@idan-david
Copy link
Contributor Author

Thank you for your patience

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.

2 participants