Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable path-only changes to metadata to trigger asset change #1689

Merged
merged 4 commits into from
Oct 4, 2023

Conversation

waxlamp
Copy link
Member

@waxlamp waxlamp commented Sep 27, 2023

The logic for PUT on asset metadata includes a check to see if the asset is "different" enough to warrant an actual change. This works in part by stripping the supplied metadata of all computed fields, of which path is one, and then comparing what remains. This had the effect of barring any "move" type operations (by changing just the path value of a metadata and performing a PUT), as changes to the path did not "count" towards being different enough. The workaround was to also change a non-computed metadata field; this made the asset different enough to trigger a change, which included the changed path.

The fix is to teach the "is different" function about path-only changes.

Thanks to @AlmightyYakob for his help in figuring this out.

TODO:

  • add a test (@AlmightyYakob)

Closes #1682.

@jjnesbitt
Copy link
Member

I've confirmed that the test included in 65cf41f fails on master, but succeeds on this branch.

@jjnesbitt
Copy link
Member

jjnesbitt commented Sep 28, 2023

The tests seem to be failing due to some underlying issue which is also affecting master, and it not a result of this PR (evidenced via https://github.com/dandi/dandi-archive/actions/runs/6319690526).

@waxlamp waxlamp marked this pull request as ready for review September 28, 2023 15:32
@mvandenburgh
Copy link
Member

The tests seem to be failing due to some underlying issue which is also affecting master, and it not a result of this PR (evidenced via dandi/dandi-archive/actions/runs/6319690526).

#1691 should make CI green again.

@yarikoptic
Copy link
Member

Let's rebase to see tests pass (or not)?

@satra
Copy link
Member

satra commented Oct 3, 2023

let's also get this in a release so that @slaytonmarx can fix some of those files.

@waxlamp waxlamp added patch Increment the patch version when merged release Create a release when this pr is merged labels Oct 4, 2023
@waxlamp waxlamp merged commit 8983b91 into master Oct 4, 2023
10 checks passed
@waxlamp waxlamp deleted the fix-put-path branch October 4, 2023 21:46
@dandibot
Copy link
Member

dandibot commented Oct 4, 2023

🚀 PR was released in v0.3.55 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PUT "silently" (returns 200) doesn't rename and doesn't update metadata (path remains old)
7 participants