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 input with all empty values for non-required gegevensgroup #13

Closed
wants to merge 2 commits into from

Conversation

annashamray
Copy link

No description provided.

@@ -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)
    ...

@annashamray
Copy link
Author

Discussed during the call. It was decided that this change is unnecessary

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