Skip to content

Fixing case where multiple fields have the same field name #469

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.

Sign up for GitHub

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

Open
matt035343 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixing case where multiple fields have the same field name #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions dataclasses_json/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ def _decode_letter_case_overrides(field_names, overrides):
if field_override is not None:
letter_case = field_override.letter_case
if letter_case is not None:
names[letter_case(field_name)] = field_name
names[field_name] = letter_case(field_name)
if field_name not in names:
names[field_name] = field_name
return names


Expand All @@ -146,7 +148,8 @@ def _decode_dataclass(cls, kvs, infer_missing):
kvs = {} if kvs is None and infer_missing else kvs
field_names = [field.name for field in fields(cls)]
decode_names = _decode_letter_case_overrides(field_names, overrides)
kvs = {decode_names.get(k, k): v for k, v in kvs.items()}
undefined_fields = {k: v for k, v in kvs.items() if k not in decode_names.values()}
kvs = {**{k: kvs[v] for k, v in decode_names.items() if v in kvs}, **undefined_fields}
missing_fields = {field for field in fields(cls) if field.name not in kvs}

for field in missing_fields:
Expand Down
29 changes: 26 additions & 3 deletions tests/test_metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import datetime
import json
from dataclasses import dataclass, field
from typing import Optional
from typing import Optional, Dict

import pytest
from marshmallow import fields, ValidationError

from dataclasses_json import DataClassJsonMixin
from dataclasses_json import DataClassJsonMixin, dataclass_json, config


@dataclass
Expand All @@ -24,7 +25,8 @@ class StringDate(DataClassJsonMixin):
'decoder': str,
'mm_field': fields.String(required=False)}
})



@dataclass
class OptionalStringDate(DataClassJsonMixin):
string_date: Optional[datetime.datetime] = field(
Expand All @@ -36,6 +38,14 @@ class OptionalStringDate(DataClassJsonMixin):
})


@dataclass_json
@dataclass
class MyDataClass:
first: str = field(metadata=config(field_name="myJsonField", decoder=lambda x: x["first"]))
second: str = field(metadata=config(field_name="myJsonField", decoder=lambda x: x["second"]))
other_field: str = field(metadata=config(field_name="otherField"))
Comment on lines +43 to +46
Copy link
Copy Markdown
Collaborator

@george-zubrienko george-zubrienko Aug 12, 2023

Choose a reason for hiding this comment

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

I'm not sure, but is this a good test case? Like, declaring myJsonField as a regular dict will not trigger the fixed behaviour first place?

Copy link
Copy Markdown
Collaborator Author

@matt035343 matt035343 Aug 13, 2023

Choose a reason for hiding this comment

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

@george-zubrienko I am not sure I understand. Are you suggesting to use a dataclass instead of a dict?

Copy link
Copy Markdown
Collaborator

@george-zubrienko george-zubrienko Aug 13, 2023 •
edited by matt035343
Loading

Choose a reason for hiding this comment

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

I mean the MyDataClass can be defined like this:

class MyDataClass:
    myJsonField: dict
    other_field: str = field(metadata=config(field_name="otherField"))
    
    @property
    def first(self) -> str:
        return self.myJsonField.get('first', '')

    @property
    def second(self) -> str:
        return self.myJsonField.get('second', '')

Copy link
Copy Markdown
Collaborator

@george-zubrienko george-zubrienko Aug 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just IMO, the way the test case is defined, dataclass does not reflect the actual data structure being read, but rather relies on some rather artificial deserializing behaviour, which for example in v1 warrants a custom serializer for the class. If this is something people actually do in their user code, I'd like to see examples. I am near 100% certain we will not support these kind of cases in v1 and force people to write custom serializer, thus I'd be wary of using these as test cases for v0 as well.

Copy link
Copy Markdown
Collaborator Author

@matt035343 matt035343 Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@george-zubrienko This is PR spawned from #270. Indeed, it is a funky usecase, however, the core here is that two dataclass-json field names maps from the same encoded JSON field name

Copy link
Copy Markdown
Collaborator Author

@matt035343 matt035343 Aug 14, 2023

Choose a reason for hiding this comment

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

@george-zubrienko and I discussed that it is actually a bad practice do like #270 does. It will only work one way (decoding only, not encoding). Instead, use the approach @george-zubrienko outlined above.

I will discontinue this PR and make a new one that throws an exception instead.



car_schema = Car.schema()
string_date_schema = StringDate.schema()
opt_string_date_schema = OptionalStringDate.schema()
Expand All @@ -60,3 +70,16 @@ def test_optional_field_only_decoded_when_present(self):
another_obj = opt_string_date_schema.load({'string_date': 'today'})
assert isinstance(another_obj, OptionalStringDate)
assert another_obj.string_date == 'today'

def test_multiple_fields_same_name(self):
contents = json.dumps({
"myJsonField": {
"first": "val1",
"second": "val2"
},
"otherField": "valotherfield"
})
obj = MyDataClass.from_json(contents)
assert obj.first == "val1"
assert obj.second == "val2"
assert obj.other_field == "valotherfield"