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

Refactor gegevensgroep/gegevensgroepserializer implementation #4

Open
sergei-maertens opened this issue Dec 20, 2022 · 0 comments
Open
Labels
Refactor Refactor/rework to future proof again
Milestone

Comments

@sergei-maertens
Copy link
Member

Currently the GegevensGroepSerializer works by invoking multiple setattr calls on the instance after calling the parent create() or update() methods. This is done in an atomic DB transaction, which has some drawbacks:

  • it's a custom implementation to manage "gegevensgroepen"
  • It requires special attention w/r to the serializer/schema generation for the API spec
  • performance is excessive: 2 queries are needed for the transactional behaviour + at least 1 query for the update by the .save call
  • saving may trigger excessive ETag updates with duplicated queries/cycles

The performance problems have been observed in the Zaak create endpoint in Open Zaak, see open-zaak/open-zaak#1271

This should instead be reworked according to the following ideas/principles:

  • Build this on top of drf-serializers without custom setters/getters/descriptors. A list of field names should be sufficient to generate a serializer on the fly with source="*" and the correct nested serializer field mapping via foo = serializser.Field(source="some_prefix_foo") (see the ModelTranslationSerializer in Open Forms for an example)
  • Remove the custom schema generation utils - genering a proper serializer on the fly should result in the correct schema
  • There's no guarantee anymore that all fields inside are required (the purpose of gegevensgroep was to treat it as an atomic unit). Instead, define the constraints properly in the database using models.CheckConstraint for data integrity purposes
  • Document the recommendation that the outer view(set) create should enforce transaction.atomic - refactoring in proper serializer fields with source attributes should automatically do the attribute assignment, which allows us to drop the custom create and update implementation. This also removes the need for the mixin.
@sergei-maertens sergei-maertens added the Refactor Refactor/rework to future proof again label Dec 20, 2022
@sergei-maertens sergei-maertens added this to the 2.0 milestone Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Refactor/rework to future proof again
Projects
None yet
Development

No branches or pull requests

1 participant