feat: add endpoint for data manager to create/update/delete accounts

Part of #27 (closed)

Description

This MR creates a new API endpoint that will be used by the account-data-manager to create/update/delete accounts. The idea is that we will run a new Cloud Run instance that's only allowing internal traffic, meaning that only that account-data-manager can access this. We only expose this newly created API endpoint if DATA_MANAGER_ENABLED is True.

assigned to @st981

So while the ticket says:

Endpoints exist ... in separate docker image build (built by multi-stage docker builds CI)

I believe we can actually reuse the same container, but simply control whether we expose the endpoint by setting an environment variable. But please let me know if there are any reasons why it's better to have the overhead of having to create multiple docker images (which I assume set this environment variable in the container itself?).

It is not explicitly mentioned on the ticket, but I think it might be useful with how we set up the account-selector by passing through potentially non eligible users.

Is the create action also going to be doing a update if already created?

Either way, you could use django's Account.objects.update_or_create() for this?

Though possibly we should be logging these oddities as they expose something getting out of sync somewhere?

Is the create action also going to be doing a update if already created?

I had that, but then I removed it because it's not a scenario that we discussed can happen.

Either way, you could use django's Account.objects.update_or_create() for this?

It also requires removing the uniqueness constraint on crsid, which I didn't figure out how to do that on the serializer easily.

---

Let me know if you think we need this or whether we should only consider this if we think it's a scenario we will face with the account-data-manager.

changed this line in version 3 of the diff

I think we should log, although I'm not too concerned about things getting out of sync - we should have the system be resilient to changes coming in out-of-order anyway as our PubSub model means that's possible.

Happy for update to do a create if it needs to, and write a log saying that's what it's done.

I think because of potential out-of-orderness it might be worth including:

Is the create action also going to be doing a update if already created?

Even if we didn't discuss it, I think it is a possibility.

We could even just make this a single endpoint actually - expecting to PUT data. Not sure exactly how that maps in DRF terms.

As order is not guaranteed should we also be doing something like storing the timestamp of the previous event that caused a create/update in the DB and then ignoring events that have a timestamp before that time? Probably another issue (that may already exist)?

Yep, I think that's probably reasonable? Might be worth spinning out a separate issue for that - I don't think one exists yet.

So account-data-manager subscribe needs to get the event timestamp (I'm assuming all cloud events have one of these) and pass that to these API endpoints (as an additional parameter/field)?

Probably should be in uis/devops&188 (closed) epic as that is when we start subscribing to update events as well?

Agree, yep can live in that epic - for this issue do we want a single endpoint for create/update or to split them up? I'd prefer to avoid having two endpoints that do the same thing - e.g. if create and update are both accepting new or updated information we may as well just make it a single PUT type endpoint.

My vote is for single endpoint.

I think we would simplify things a lot for a single endpoint, happy with that being PUT. That way there's no surprise that when we want to create, we might update and the other way around. I will work on that and later create a new issue in uis/devops&188 (closed) if nobody else did that yet.

This has been changed now, there's a single endpoint that handles both updating and creating. Note that I didn't go for any update_or_create logic, because I believe it's better to handle the decision whether we want to update or create on the view level, so that we can use this information to pass back to the user what we actually did, creation (201) or update (200).

create_or_update does return a boolean saying whether the object was created or not - so I think if the only driver is the status return code we can use create_or_update for that job?

(I agree with the different return codes though, I think that's a useful thing to communicate back to the client).

create_or_update does return a boolean saying whether the object was created or not - so I think if the only driver is the status return code we can use create_or_update for that job?

Yes, but there's not really a "clean" way of feeding this back from the serializer to the view. You would have a serializer like:

    @transaction.atomic
    def create(self, validated_data):
        account_details = validated_data.pop("account_details")
        crsid = validated_data.pop("crsid")
        account, created = Account.objects.update_or_create(crsid=crsid, defaults=validated_data)
        AccountDetails.objects.update_or_create(
            account=account,
            defaults=account_details,
        )

        return account

    def update(self, instance, validated_data):
        raise NotImplementedError()

Now we have access to whether it's created or not, but, we can't simply pass that as a return value of create because save expects create to only return the instance (because it will assign it to self.instance). If we would return account, created, the serializer will break (for example, serializer.data, which is the first argument we pass to Response in the view, will not work).

So we could make something like:

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

        self.created = None

    @transaction.atomic
    def create(self, validated_data):
        account_details = validated_data.pop("account_details")
        crsid = validated_data.pop("crsid")
        account, created = Account.objects.update_or_create(crsid=crsid, defaults=validated_data)
        AccountDetails.objects.update_or_create(
            account=account,
            defaults=account_details,
        )

        self.created = created

        return account

    def update(self, instance, validated_data):
        raise NotImplementedError()

But I am not a big fan of that, because we have this attribute created on the serializer, which does not make sense if save is not called.

With the current approach, where we make the view responsible for the decision whether we want to create or update, we can keep the create and update method on the serializer, plus have a nice and clean way to decide whether we created or not.

Gotcha, yeah makes sense - happy with this approach then!

Created #33 (closed)

added 1 commit

• 83313198 - feat: add endpoint for data manager to create/update/delete accounts

Compare with previous version

mentioned in issue #27 (closed)

added 1 commit

• 19697420 - feat: add endpoint for data manager to create/update/delete accounts

Compare with previous version

requested review from @mk2155

added 1 commit

• 8a90ac71 - feat: add endpoint for data manager to create/update/delete accounts

Compare with previous version

resolved all threads

approved this merge request

mentioned in commit 52092458

merged

mentioned in issue #33 (closed)