-
Notifications
You must be signed in to change notification settings - Fork 610
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
Prevent similar versions that only differ in build metadata from being published #6518
Conversation
…g published `cargo` does not handle these situations particularly well and the semver specification is a little ambiguous about how to handle build metadata. We still have roughly 600 problematic versions in the database, so we can't introduce a unique index yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good. 👍
For what it's worth, playing around with this locally seemed to throw up some bigger deltas between the old query and the new one. With a cold cache, the old query executed in 12 ms for me, whereas it took 48 ms for the new query to execute. (I tested this with both the crate ID you used, and the one that has the most actual versions; the delta was similar in both cases.)
I can get the difference back below the noise floor by adding an index on the computed field, which I've pushed to 757dce5. My guess is that you're right that this PR isn't likely to cause problems anyway, but you could cherry pick that in if you want to add the index for more assurance.
yep, I remember getting similar numbers here, though for the publish endpoint I guess this would probably be acceptable?
As I mentioned in 757dce5#r114738341, the performance actually got worse for me locally when I added this index. If I add an index on the Also, I am a bit scared about adding indices in migrations. Locally, adding the index took about three seconds, but I've been bitten by this in the past already, where in production it took a lot longer. Since migrations are currently running after shutdown of the old app version and startup of the new app version it would effectively cause multi-second downtime for us if we add the index in a migration. We can use |
Assuming that this was discussed in the team meeting last week (where I unfortunately couldn't attend) and since there are apparently no objections I will merge and deploy this change now. Since it does not involve any database migrations we can always revert this if we decide that this is the wrong step forward. |
Resolves #1059
cargo
does not handle these situations particularly well and the SemVer specification is a little ambiguous about how to handle build metadata.We still have roughly 600 problematic versions in the database, so we can't introduce a unique index yet, but the query appears to be fast enough according to my local testing:
vs.
The
cost
is certainly higher, but the actual wall time for these queries appears to be roughly similar in the end. Since the publish endpoint is only receiving few requests per minute this should not result in any issues AFAICT.