nextstrain / nextstrain.org

The Nextstrain website
https://nextstrain.org
GNU Affero General Public License v3.0
88 stars 49 forks source link

[resource collection] Interaction with PUT/DELETE/OPTIONS methods is likely wrong #744

Open jameshadfield opened 11 months ago

jameshadfield commented 11 months ago

In #719 @tsibley states that "Interaction with PUT/DELETE/OPTIONS methods is likely wrong. Not an immediate problem because while we have endpoints for those methods on core/staging, we don't use them. But as-written the interaction/behaviour if they did run is incorrect, and it will be an issue for Groups or if we start using those methods for core/staging (which I think we should)."

@tsibley could you say more here? Are there tests you can write which would highlight the problem for me? I'd be happy to fix these issues.

tsibley commented 11 months ago

The issue is that we haven't considered what it means to PUT and DELETE past versions (and how that affects OPTIONS). We can't support replacing past versions with PUT, so that would fail when tried, and while maybe we'd want to support deleting past versions with DELETE, we'd need to think thru the implications.

I could see a couple potential ways to address this.

One way to potentially address this is to extend the handlers (e.g. putDataset and deleteDataset) to throw a 405 Method Not Allowed if the resource in question is not the latest version. And something similar for optionsDataset, though that currently is solely based on authorization.

Alternatively, we could avoid registering handlers for those methods in the first place by using separate routes for versioned vs. unversioned paths. This might make more sense if part of a broader change to how versions are extracted by the handlers (something I've sketched out some unfinished thoughts on for myself).

tsibley commented 11 months ago

Ah, and this is also a place where having a ResourceVersion class in the sources data model might be more helpful (compared to a version-aware Resource): to let us more easily express policies like "only owners can delete past versions". (Assuming we'd want to support deletion.)