Skip to content
This repository has been archived by the owner on Aug 19, 2023. It is now read-only.

Fix nested union dataclasses #119

Closed
wants to merge 3 commits into from
Closed

Fix nested union dataclasses #119

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 26, 2019

Fixes issue #118

@@ -398,7 +398,7 @@ def _decode_field(cls, field: str, field_type: Any, value: Any) -> Any:
# Replace any nested dictionaries with their targets
field_type_name = cls._get_field_type_name(field_type)
if cls._is_json_schema_subclass(field_type):
def decoder(_, ft, val): return ft.from_dict(val, validate=False)
def decoder(_, ft, val): return ft.from_dict(val)
Copy link
Owner

Choose a reason for hiding this comment

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

Unfortunately, whilst this fixes the issue, it also causes a performance regression by causing all nested objects to be validated more than once with both the parent and child schemas.

I don't think I can add commits to your PR but the following should fix this without revalidating every child schema:

diff --git a/dataclasses_jsonschema/__init__.py b/dataclasses_jsonschema/__init__.py
index 3d46fc2..86779f5 100644
--- a/dataclasses_jsonschema/__init__.py
+++ b/dataclasses_jsonschema/__init__.py
@@ -398,7 +398,7 @@ class JsonSchemaMixin:
             # Replace any nested dictionaries with their targets
             field_type_name = cls._get_field_type_name(field_type)
             if cls._is_json_schema_subclass(field_type):
-                def decoder(_, ft, val): return ft.from_dict(val)
+                def decoder(_, ft, val): return ft.from_dict(val, validate=False)
             elif is_nullable(field_type):
                 def decoder(f, ft, val): return cls._decode_field(f, unwrap_nullable(ft), val)
             elif is_optional(field_type):
@@ -412,7 +412,7 @@ class JsonSchemaMixin:
                     try:
                         decoded = cls._decode_field(field, variant, value)
                         break
-                    except (AttributeError, TypeError, ValueError, ValidationError):
+                    except (AttributeError, TypeError, ValueError):
                         continue
                 if decoded is not None:
                     return decoded
@@ -482,10 +482,14 @@ class JsonSchemaMixin:
             if f.mapped_name in data or (
                     f.field.default == MISSING and f.field.default_factory == MISSING):  # type: ignore
                 try:
-                    values[f.field.name] = cls._decode_field(f.field.name, f.field.type, data.get(f.mapped_name))
+                    values[f.field.name] = cls._decode_field(f.field.name, f.field.type, data[f.mapped_name])
+                except KeyError:
+                    if is_optional(f.field.type):
+                        values[f.field.name] = None
+                    continue
                 except ValueError:
                     if is_enum(f.field.type):
-                        values[f.field.name] = data.get(f.mapped_name)
+                        values[f.field.name] = data[f.mapped_name]
                     else:
                         raise

@snorfalorpagus
Copy link
Contributor

@narakhan Does the proposed change look OK to you? If so can you update your MR?

@richard-moss
Copy link

Hi all,

I had this problem with nested Union, and tried applying both changes above, but was still have the schema only produce the first listed cliass in a Union.

I did some more testing on my side, and it could be an issue where my Union was also Optional, ie I was using

    # A, B defined already
    a: Optional[Union[A,B]] = None

In the case above, only A would be defined in the schema.
If I removed the Optional typing, and default of None, and so had instead the definition below (as matches the test.py test), then both definitions A and B would be included in the schema:

    a: Union[A,B]

@jobh
Copy link
Contributor

jobh commented Oct 26, 2020

@richard-moss I have the same problem, see #147. I think it is another issue, not addressed by this PR.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants