maykinmedia/commonground-api-common

Shared functions between Common Ground API components

commonground-api-common's Issues

Avoid "island-like" approach for mixins like audit trail/notifications...

Should be tackled in notifications-api-common (partially) - but the interaction is definitely relevant.

We now have Audit trail, notification and cache mixins/decorators that all operate on the viewset action serializer (instance). Often these mixins query the involved object and/or construct a fresh serializer instance. Serializing each of these instances of the same serializer class results in bad query performance because it repeats queries that were done already, especially to serialize the response data.

This whole mixin approach needs to be revisited for a better/more granular approach giving more control for re-use of instances that can leverage their own caches, so that performance is maximized without dirty tricks.

Additionally, the @transaction.atomic behaviour should either be configurable or managed in the downstream project - now often mixins create stacks of savepoints/savepoint releases that introduce unnecessary query IO.

Remove APICredential-based machinery

We can rely 100% on zgw-consumers instead of having this mixed approach based on a vanilla gemma-zds-client implementation.

zgw-consumers is richer and provides django support. The core mechanisms (get_auth etc.) should be adapted to build on top of zgw-consumers.

Introduce celery (opt-in?) for certain workflows

For example: ETag updates

On create/update of resources we schedule etag (re-)calculations that are currently done in the request-response cycle. Since the caching decorator only emits this header for detail resources (retrieve action), this does not have to commit in that cycle - meaning we can delegate that to celery.

This can seriously boost performance, especially if the involved resource is input for many other resources that may need to be recalculated (e.g. zaak-status).

Alternatively, we should investigate a mechanism to better avoid queries that don't matter anyway and/or better de-duplicate etag updates.

Possibly it must be possible per operation/endpoint/viewset to specify whether the update should be sync or async.

Update fork to version 1.9.0

After 1.9.0 the refactor from upstream to drf-spectacular has been started/done, so we should take a critical look at that before adopting it.

Once this fork is on 1.9.0, we can swap out vng-api-common in Open Zaak with commonground-api-common and continue development independently.

Refactor gegevensgroep/gegevensgroepserializer implementation

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.

Drop support for old dependencies/structures

The following patterns are outdated or obsolete and should no longer be supported:

(will require a bump to 2.0 to stick to semver)

• PyJWT 2.0+ required
• ZDS-client 2.0+ required (and ideally replaced with generic cg-api-client fork of gemma-zds-client)
• support DRF 3.12 and newer (now pinned to ~3.12)
• Drop vng_api_common.mocks, downstream needs to use requests-mock or similar instead. Lots of overlap with zgw-consumers, so possibly introduce that as dependency?
• Streamline setting names - now there's a prefix missing making it ambiguous
• Drop vng_api_common.compat
• Drop vng_api_common.notifications - make sure migrations have been executed to notifications-api-common
• Drop testing mixins - Open Zaak has its own implementation anyway
• Drop static assets/templates?