Issue51 copy file

[coleshaw]

@graft , can you give this another look? One thing I wasn't sure about in the test stubs was the exact format of the returned "routes" that get fed into Etna::Client. The definitions in Metis show:

/:project_name/file/copy/:bucket_name/*file_path

But I don't think the route parser handles *file_path correctly, and I'm not exactly sure how it gets populated "in real life" in Etna (i.e. *file_path or :file_path). Is there a way I can investigate that locally? I've tried going into a Magma console (bin/magma console), but can't stand up an Etna::Client because it can't verify SSL certs.

[coleshaw]

@graft , I believe this works. At least locally, when testing manually against Metis, I get a new entry in the database!

  4 | proj          | test.txt                                 |         1 | f         | user | Developer | 2020-05-07 18:31:40.460931 | 2020-05-07 18:31:40.460931 |           |             6
  5 | proj          | model-record-attribute.txt         |         2 | f         | user | Developer | 2020-05-14 20:27:39.075861 | 2020-05-14 20:27:39.075861 |           |             6
(5 rows)

I'm missing the methods that handle ::nil and ::blank revisions, but want to put them into a separate PR, since this one seemed like it was getting big.

[graft]

This is nice stuff. I might pay attention and simulate in the tests some sad-path stuff, like Metis replying with a 400 preventing the copy operation.

In this vein, if the user updates multiple records at once, you are sending several requests instead of a single large request. One or more of these requests could 400. Since we may have already asked metis to copy some of the entries successfully we have no choice but to link the record. On the other hand, we can't complete the entire update, since some of our requests have failed. This leaves us in a "partial update" state, very bad. I'm not really sure what to do in this case.

[coleshaw]

Great points. I can add in more tests for non-successful copies.

Regarding multiple updates, I should add tests for those, too. What are your thoughts on using Threads or Fibers to wait for all requests to return, like in this question? If all requests return 200, we continue on to load_revisions. If one or more fail, then we clean up by deleting the successful ones on Metis.

[graft]

The async nature of threads doesn't seem to buy anything here since we must still block, and the problem is rather that we might be partially successful. So far the validation logic tries to be transactional - either the whole update works or not.

[coleshaw]

Hm, sorry, not quite sure I understand why something like this approach won't help avoid the partially successful problem. With Threads or some sort of async operation, if we block all the updates, can't we "undo" the successful copies on the Metis side if any single Metis copy fails? From the original point (just for context):

In this vein, if the user updates multiple records at once, you are sending several requests instead of a single large request. One or more of these requests could 400. Since we may have already asked metis to copy some of the entries successfully we have no choice but to link the record <Here, can't we "remove" the copy entries that succeeded on Metis, then throw an exception and skip the next steps on Magma? I'm confused about why we are left with no choice but to proceed with the linking on Magma?>. On the other hand, we can't complete the entire update, since some of our requests have failed. This leaves us in a "partial update" state, very bad. I'm not really sure what to do in this case.

Maybe this is easier discussed verbally (and maybe with a whiteboard!)?

[coleshaw]

Just a random question about the partial update problem ... is that something that you think we need to address now, in this PR, or something we should address later? You can't update multiple records through the new Timur UI right, so is that more of a future-issue we tackle later?

And for the Thread idea, wasn't sure if that is the right Ruby construct or if there is another approach. I was thinking of an equivalent of JavaScript Promise.all.... don't think it would necessarily look clean to remove the partially created copy links, but might be possible to do.

[coleshaw]

Yes, any errors in execute_bulk_copy_on_metis will cause load_revisions to not run. There is a spec for that scenario.

[coleshaw]

@graft , refactored with body_request per your feedback. Thoughts on current state? I'll branch off of this for the continuing work so this PR doesn't get too big.

[coleshaw]

Superseded by #180