Metadata correction

[candleindark]

This PR provides a solution to correct the corruptions in metadata documented in dandi/dandi-schema#276.

The solution is implemented in two parts.

1. A general command, correct_metadata, to correct dandiset metadata.
2. A specific helper function which the general command calls to do the actual correction.

Extensive tests are provided for the helper function and its supporting function. However, because I am not familiar with this repo and Django in general, I am not able to provide tests for the command which interacts with the database. Advice and additional tests are very much appreciated.

The command can be run on a targeted dandiset at a particular version and run on all versions of all dandisets. Running on all dandiset versions will only correct the corrupted dandisets. If running only on targeted dandiset version is preferable, please let me know, and I will provide the list of corrupted dandiset versions. (Additionally, would changing the interface of the command to a file consisting of corrupted dandiset versions be better?)

[asmacdo]

a couple of questions to start off with-- I'll have another look tomorrow.

[asmacdo]

What happens if any of the file writes fail or time out?

dandi-archive/dandiapi/api/tasks/__init__.py

Lines 58 to 62 in 7728016

	write_dandiset_yaml(version)
	write_assets_yaml(version)
	write_dandiset_jsonld(version)
	write_assets_jsonld(version)
	write_collection_jsonld(version)

Maybe we should at least keep the AsyncResults and monitor that they go as planned?

[candleindark]

I actually don't know if I need to call write_manifest_files.delay(ver.id) at all. I was trying to mimic

dandi-archive/dandiapi/api/management/commands/migrate_published_version_metadata.py

Line 52 in 7728016

write_manifest_files.delay(version.id)

, but https://github.com/dandi/dandi-archive/blob/master/dandiapi/api/management/commands/migrate_version_metadata.py didn't call write_manifest_files() at all.

Maybe we should at least keep the AsyncResults and monitor that they go as planned?

I think the answer is related to how important that statement gets executed successfully. In the example above, its execution status is not checked. If a success execution is important, I guess I can't interact with the DB the current way.

@dandi/dandiarchive

[asmacdo]

IIUC the original intent of the change that led to this problem was to remove all fields but the allowed ones-- would it be beneficial to also have a list of allowed_fields to confirm that they are the only ones?

[candleindark]

In concept, you are right. I considered do that as well. However, I want to be careful for now. I want to remove only the ones that I know are causing problems. I don't want to remove anything that we haven't analyzed previously.

[waxlamp]

Thank you, @candleindark! One thing to change listed below. And I've asked @jjnesbitt to provide a more comprehensive review on this R as well.

[candleindark]

@jjnesbitt I have put in changes requested by @waxlamp regarding the command line interface. With the latest push, two unrelated tests failed. Can they be related to the latest changes in dandi-cli?

[jjnesbitt]

I've pushed a commit which aims to simplify the logic in this management command. Namely:

• Remove the configuration of the correcting function.
  • We have no real use case for this yet, and no way to change this function is provided.
• Removal of verbose error handling logic
  • Along the same lines as the above point, since this is a single function, this is largely unnecessary. We have tests, and if the correcting function fails for some reason, the error will simply bubble up.
• Only support correction of the Affiliation schema key
  • Similarly again, we only have the use case of Affiliation at the moment, so all of the configuration surrounding other schema keys is unnecessary at the moment.
• Remove find_objs tests against generalized schema key
• Write manifest files synchronously
  • Since we are not in a request context, it's better to write the manifest files to S3 synchronously, so that we can be sure it succeeds.
• Use transactions to couple logic of metadata save and manifest file generation

IMO this is ready, maybe @waxlamp should take another look.