purescript / registry-dev

Development work related to the PureScript Registry
https://github.com/purescript/registry
95 stars 80 forks source link

Incorrect time measurements in unpublish workflow prevents unpublishing #652

Closed thomashoneyman closed 1 year ago

thomashoneyman commented 1 year ago

As seen in: https://github.com/purescript/registry/issues/346#issuecomment-1665808201

...a package published just minutes prior cannot be unpublished because, and I quote:

Cannot unpublish web-fetch@4.0.0 because it was published 6063.176484722222 hours ago. Packages can only be unpublished for 48 hours after publication.

This error arises here: https://github.com/purescript/registry-dev/blob/3166e0982d37d96cfc586fcd0791de937822d2c1/app/src/App/API.purs#L243-L256

But the real implementation is deeper, in the registry library itself: https://github.com/purescript/registry-dev/blob/3166e0982d37d96cfc586fcd0791de937822d2c1/lib/src/Operation/Validation.purs#L133-L158

We're comparing the current time in the UTC time zone to the published time associated with the package, which is calculated here: https://github.com/purescript/registry-dev/blob/3166e0982d37d96cfc586fcd0791de937822d2c1/app/src/App/Effect/Source.purs#L94-L108

The problem in this specific case:

  1. A commit to the repository was made in 2022
  2. The tag associated with that commit wasn't made until today
  3. We published the package, looked up the ref time, and voila: we see 2022 and assign that as the 'published time'
  4. Now the publish time is long in the past, and you can't unpublish.

The reason why we use the ref time as the publish time is to support the legacy importer, in which we need to associate old publish times so the registry doesn't say everything was published in August 2023. But we probably should be using the current time in the publish pipeline instead.

f-f commented 1 year ago

But we probably should be using the current time in the publish pipeline instead

I think there's value in preserving timestamps of old packages. We can take care of this issue by:

thomashoneyman commented 1 year ago

Good point on the second note there, and I agree with you we ought to preserve time stamps for old packages

pete-murphy commented 1 year ago
  • distinguishing when something is coming from the legacy importer vs the new pipeline

Would this just amount to distinguishing between when this handle is called from the LegacyImporter module vs elsewhere? Effectively something like this diff here (https://github.com/purescript/registry-dev/pull/656/commits/15d747fa58977c297a7f5b55a7924ea96996f6f4) or would it be more involved than that?

thomashoneyman commented 1 year ago

I think you're right that it's basically choosing between two scenarios: one in which this is an import of a recent package initiated by a user's action, and one in which this is an automatic import of a legacy package. In fact, we already have a type for the distinction between the two:

https://github.com/purescript/registry-dev/blob/282a4f049af74df888c9871b340b2c4100650491/app/src/App/API.purs#L98-L101

The easiest thing is probably to move this data type into Prelude.purs and then have the fetch function take it as an argument: https://github.com/purescript/registry-dev/blob/282a4f049af74df888c9871b340b2c4100650491/app/src/App/Effect/Source.purs#L44-L45

thomashoneyman commented 1 year ago

(The comment on the Source data type isn't quite right, because with #255 we'll require all packages to compile; no exemption of "legacy" packages. But the idea is the same: in some ways we treat legacy packages differently than 'current' ones.)