Enforce uniqueness explicitly

[Charl1996]

Technical Summary

This PR is an attempt to fix this sentry error.

It's slightly surprising to me that this error occurred, since we explicitly checked whether the a record with the given request_id already existed...but nonetheless, the current implementation handles this situation at the source, which also accounts for potential race conditions.

Logging and monitoring

N.A

Safety Assurance

Safety story

• I am confident that this change will not break current and/or previous versions of CommCare apps

Automated test coverage

Extensive unit tests already exists.

QA Plan

No QA.

Labels & Review

• The set of people pinged as reviewers is appropriate for the level of risk of the change

[zandre-eng]

also accounts for potential race conditions.

@Charl1996 Could you say more about this? My understanding is that if a specific device hits the view that calls log_sample_request() then there shouldn't be a race condition since this device won't hit this endpoint more than once at a time. If it does then I would be surprised and we should maybe check to make sure that this endpoint isn't getting spammed by mobile.

[Charl1996]

My understanding is that if a specific device hits the view that calls log_sample_request() then there shouldn't be a race condition since this device won't hit this endpoint more than once at a time.

Correct, although there could be a bug or something on mobile. It doesn't look like there's multiple such cases on sentry. I'll let mobile know it happened, maybe they can spot something.

[Charl1996]

I put the check back where we filter for request_id, because if it's the case that the API is hit multiple times with the same request_id we don't want to hit Google's API only to find out later that the request should not be completed.

[Charl1996]

So we can track if there's something funky going on...

[calellowitz]

nit: I don't think this is strictly necessary (but fine to do). I don't know that getting a duplicate request is something we need to be concerned about, and I want to be in the habit of keeping sentry full of only actionable errors rather than ones we say "that is fine" about as much as we can.

[Charl1996]

The only reason I added the sentry logging was so we can know if there's spamming or so going on (i.e. endpoint is hit with the same request_id multiple times) so we can let mobile know to look into it.

[calellowitz]

left a few comments the code structure one is more important than the sentry one

[calellowitz]

How did we decide to keep the old vs new values? It seems like we may want to update to the latest.

If we are going to keep the old ones and raise an error on conflict, I think you can leave this as create and let the db error be raised, since we aren't actually doing a get_or_create but create or exception.

[calellowitz]

Alternatively, we could consider just storing both, since it is a new request and new data from the server side.

[Charl1996]

How did we decide to keep the old vs new values? It seems like we may want to update to the latest.

Theoretically there won't ever be a "newest" value (only one), because mobile does not make the request twice for the same request_id. In the fringe case (like the one we had) I highly doubt the outcome of the sample will change, unless mobile explicitly makes a request again for the same device after the user has made updates to their device which would result in a different outcome. And even if the user made updates that now results in a different outcome, that's only one sample in literally thousands, which I think renders it somewhat insignificant...that's why I didn't bother catering for a second-request-scenario too much.

If we are going to keep the old ones and raise an error on conflict, I think you can leave this as create and let the db error be raised

If I let the DB error raise I'd have to catch that and return a sensible result to mobile, but the current implementation essentially mimicks that same behaviour with the custom error. I can update if you feel strong about it, but it's not changing any behaviour (just getting rid of a custom error).

[calellowitz]

nit: I don't think this is strictly necessary (but fine to do). I don't know that getting a duplicate request is something we need to be concerned about, and I want to be in the habit of keeping sentry full of only actionable errors rather than ones we say "that is fine" about as much as we can.