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

feat: Add Operation Type support #215

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rmanganiello
Copy link

@rmanganiello rmanganiello commented Jul 10, 2023

Problem

Currently, there's no way to determine if after an UPSERT operation in the database
a row was inserted or updated.

This functionality can be handy in different use cases.
i.e.:

  1. Exposing this to clients through an API (200 or 201 status codes).
  2. Deciding how to notify this event in an Event-based driven system.

Solution

This PR intends to add this functionality by adding a new flag called return_operation_type.

When the client specifies this flag, a new key _operation_type will be returned for each row
setting the corresponding operation ("INSERT" or "UPDATE") that was performed for it.

This new flag will be available in the bulk_upsert and bulk_insert methods of the PostgresQuerySet
class, and, only if the return_model flag is off.

Caveats

  1. This flag is only available if the return_model flag is off to avoid breaking the current contracts
    since injecting this data into a Django Model is hacky.
  2. The decision on having this flag off by default, is to avoid breaking any implementation where
    clients are building their own Django models by hand after calling the bulk_upsert/bulk_insert methods.

Related Issue: #100

There are pre-existing typing issues in the project that seems to be
ignored because the functions don't have typing hints.
@Photonios
Copy link
Member

Nice trick! I am not a fan of the API for this. We already have dynamic return types in bulk_upsert and adding yet another variation does not make for a predictable API.

Could we maybe abstract this in a separate method?

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