search

mountetna / magma

Data server with friendly data loaders
GNU General Public License v2.0
5 stars 2 forks source link

Support table attribute in /update #197

Closed graft closed 3 years ago

graft commented 3 years ago

This adds some basic support in the /update endpoint for updating table attributes. There were two main issues to resolve:

1) It is customary to use table to link to a leaf model with no identifier (that is, just an ancillary table attached to this model; the only relevant identifier is the parent's), in which case many Magma operations substitute the database id column for the identifier. The /update revision format, however, posts changes as { identifier => { identifier_changes } }. If we are inserting new records, there must be a way for us to specify a non-existent id (that is, cause the insertion to create a new id).

2) There might be existing records in a table that should be removed before inserting new values.

Here I have implemented (1) and not (2). I think it is possible to do (2) but not without increasing the coupling across several Magma classes, which I am not willing to do, and will instead opt for the somewhat more painful route of deleting records in the console manually before re-inserting (if necessary) to meet our v1 goals.

(1) is mostly accomplished by resurrecting the old TempId system - this allowed a way to create temporary associations by wrapping objects (e.g. the hash representing a patient) in a TempId, which following the insertion into the database can be updated with the real id of the insertion, allowing us to resolve links before the /update completes (see Magma::Loader#update_temp_ids). Here I have reattached this system to the /update input using the special identifier ::temp, followed by any unique string, e.g. ::temp1, ::tempkevin, etc., which may be used to generate a TempId.

Regarding said coupling, the main discovery I have made here (but not implemented) is how to resolve a good deal of the confusion currently at play in the code base. I don't think I can take the time to go through all of the changes required and meet our goals, so this discovery must play out later. Summarized in #196.

coleshaw commented 3 years ago

Outside of the issue with the Loader description method, looks good to me. Ran fine when I commented out the descriptions in each of the IPI loaders, but that should get addressed before deploying (either in Loader or in IPI), otherwise I worry the deployments will break.