search

NCEAS / metacatui

MetacatUI: A client-side web interface for DataONE data repositories
https://nceas.github.io/metacatui
Apache License 2.0
41 stars 26 forks source link

Saving in the editor can sometimes break when replacing an item #1355

Closed amoeba closed 4 years ago

amoeba commented 4 years ago

I'm seeing the following behavior on my feature-replace-item branch.

  1. Create and save a package with metadata, a CSV, and an R script
  2. Open the package in the editor and replace the R script with another
  3. Click Submit to save the package
  4. End up with a broken/partial save and a "Not all of your changes could be submitted." message up top with no additional info.

Scanning my Network pane I see a culprit for the breakage:

  1. HTTP PUT MNStorage.update fires off, is successful (HTTP 200), returns $NEWID in response like it should. Good so far.
  2. HTTP GET MNStorage.getSystemMetadata($NEWID) 404s with

    No system metadata could be found for given PID: $NEWID

  3. HTTP GET MNStorage.getSystemMetadata($NEWID) 200s with our system metadata in the response

It looks like the error at (2) breaks the overall save process even though the failing request appears to get retried and succeeds. DataPackage.save has retry logic so I'm going to start debugging near that to see why the save process fails even though it's retried like we expect.

mbjones commented 4 years ago

Interesting. Given that MNStorage.update provides a SystemMetadata document as a parameter of the payload, why do we then need to immediately go retrieve that info in steps 2 and 3? If there is a version conflict, presumably 1 would fail and need to be retried. If step 1 succeeds, then presumably the sysmeta that was sent was accepted as valid...

amoeba commented 4 years ago

I don't know about the rationale for the getSystemMetadata call immediately after the MNStorage.update call. It may be unnecessary. Though I'll note that the MN mutates the System Metadata after MNStorage.create/update with properties the application may need later on down the line so a fetch after create/update doesn't sound unreasonable just yet.

amoeba commented 4 years ago

Turned out to be a bit of a red herring and was actually caused by a double-save issue which was itself caused by a race condition in DataPackage.save()'s logic. See https://github.com/NCEAS/metacatui/commit/0455281ca18b9c686680ff799cfb1ece918d3a7c for a writeup. I modified the logic to guard against the race condition and have tested that the double-save goes away.