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.

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

allow input with all empty values for non-required gegevensgroup #13

Closed
wants to merge 2 commits into from
Closed

allow input with all empty values for non-required gegevensgroup #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8a0b4f
:test_tube: test serializer with empty gegevensgroup items
annashamray Oct 12, 2023
8acc9cc
:sparkles: allow empty data for non-required gegevensgroup
annashamray Oct 12, 2023
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
13 changes: 13 additions & 0 deletions tests/test_gegevensgroepen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,16 @@ def test_nullable_gegevensgroep():
"street": "",
"number": "",
}


def test_gegevensgroep_not_required_with_all_empty_values():
"""
check that if GegevensGroepSerializer is not required and all its items are empty
it's processed as valid
"""
serializer = PersonSerializer2(
data={"name": "Willy De Kooning", "address": {"street": "", "number": ""}},
partial=False,
)

assert serializer.is_valid()
4 changes: 3 additions & 1 deletion vng_api_common/descriptors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ def _value_getter(attr):

def __set__(self, obj, value: Optional[dict]):
# value can be empty, if that's the case, empty all model fields
if not value:
# if all nested values are empty - treat it as empty value
all_empty = all(not bool(v) for k, v in value.items()) if value else True
Copy link
Member

Choose a reason for hiding this comment

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

what if you have a number field where 0 is the value? the bool(0) will give False, but it's not empty. Emptiness should typically be compared against the field-specific data type & empty check.

Copy link
Author

Choose a reason for hiding this comment

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

@sergei-maertens Could you maybe help me with how to do it in the most meaningful way?

The problem:
In Catalogi API ZaakType will have optional gegevensgroups bronzaaktype and broncatalogus (here is OAS - https://redocly.github.io/redoc/?url=https://raw.githubusercontent.com/VNG-Realisatie/catalogi-api/1.2.1/src/openapi.yaml#tag/zaaktypen/operation/zaaktype_retrieve)
So when you request zaaktype via API of export catalogus you will see gegevensgroups with empty items inside.
But when you try to import it or use it for PUT for example there would be validation error, because some items inside these gegevensgroups are not optional.
I want to avoid it. The best idea I came up with - to treat the gegevensgroup with all empty items like the empty one.

Do you perhaps have an idea how to do it better way?

Copy link
Member

Choose a reason for hiding this comment

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

from the API spec I read:

  • bronzaaktype key is optional
  • when bronzaaktype key is included, it may not be null
  • when bronzaaktype key is included, the object provided must have all nested keys specified, but the values may be empty strings (no min length specified)

the idea of some items inside a gegevensgroep being optional goes directly against the (initial) concept of gegevensgroep which was created to validate all or none are provided. I feel that gegevensgroep is being misused if that's not the case, instead it would make more sense to directly define a nested serializer with the appropriate field source set.

so maybe something like this could work:

class ZaakTypeBronZaakTypeSerializers(serializers.ModelSerializer):
    url = serializers.URLField(source="bronzaaktype_url")
    identificatie = serializers.CharField(source="bronzaaktype_identificatie")
    omschrijving = serializers.CharField(source="bronzaaktype_omschrijving")

class ZaakTypeSerializers(serializers.ModelSerializer):
    ...
    bronzaaktype = ZaakTypeBronZaakTypeSerializer(source="*", allow_null=False, required=False)
    ...

if not value or all_empty:
if self.required:
raise ValueError("A non-empty value is required")

Expand Down
11 changes: 8 additions & 3 deletions vng_api_common/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,15 @@ def validate_empty_values(self, data):
"""
(is_empty_value, data) = super().validate_empty_values(data)

if self.root.partial:
if is_empty_value:
return (is_empty_value, data)
if is_empty_value:
return (is_empty_value, data)

# allow to have all fields empty if the gegevensgroup is not required
all_empty = all(not bool(value) for name, value in data.items())
if not self.required and all_empty:
return (True, data)

if self.root.partial:
errors = OrderedDict()

for field_name, field in self.fields.items():
Expand Down
Loading