Skip to content

Show at most one crate from the same repo in /releases search #1375

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

Closed
wants to merge 2 commits into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 21, 2021

This avoids showing rustc_ap_* three dozen times in a row.

TODO: is it correct to pick the latest published crate? Is there a way
to find the "main" crate from a repository?

The first commit caught several bugs and should go in separately even if this doesn't land.

Fixes #985.

@jyn514 jyn514 added A-backend Area: Webserver backend S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Apr 21, 2021
@jyn514 jyn514 changed the title Show at most one crates from the same repo in /releases search Show at most one crate from the same repo in /releases search Apr 21, 2021
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the statement gives us the results we want we should check the EXPLAIN output with a production-sized DB to be sure if it's fast enough.

There are several other ways do the query, we can try these if the PARTITION happens to be too slow.

@@ -736,6 +736,11 @@ pub fn migrate(version: Option<Version>, conn: &mut Client) -> crate::error::Res
"ALTER TABLE builds RENAME COLUMN cratesfyi_version TO docsrs_version",
"ALTER TABLE builds RENAME COLUMN docsrs_version TO cratesfyi_version",
),
migration!(
context, 30, "Ensure repos don't have multiple ids",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
context, 30, "Ensure repos don't have multiple ids",
context, 30, "Ensure repos don't have multiple names",

releases.release_time,
releases.rustdoc_status,
repositories.stars,
rank() OVER (PARTITION BY repository_id ORDER BY release_time DESC) AS rank
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will filter all releases without repository (where releases.repository_id is NULL) except the most recent one.

((NOT $3) OR (releases.build_status = FALSE AND releases.is_library = TRUE))
AND {0} IS NOT NULL
) i
WHERE rank = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WHERE rank = 1
WHERE i.repository_id IS NULL OR rank = 1

@syphar
Copy link
Member

syphar commented Apr 21, 2021

other thought, only to be safe:

It was an intended change that this grouping will happen for all release lists?
That would make them repo with release lists, not release-lists any more, right?

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Apr 21, 2021
jyn514 added 2 commits April 21, 2021 10:19
Before, it would create two different repos with the same name, which
messed up searching.
This avoids showing rustc_ap_* three dozen times in a row.

TODO: is it correct to pick the latest published crate? Is there a way
to find the "main" crate from a repository?
@jyn514
Copy link
Member Author

jyn514 commented Apr 21, 2021

I reverted the unique constraint after the discussion we had on discord:

  • It was a test-only issue; if we get wrong IDs in production, it's because github has a bug, not us
  • It could prevent real usecases where a repository gets renamed and a new repository takes its old name

@syphar
Copy link
Member

syphar commented Nov 13, 2021

@jyn514 we can probably close this PR in favor of #1521 and #1546 , right?

Or am I missing something?

@jyn514
Copy link
Member Author

jyn514 commented Nov 13, 2021

Sure, seems reasonable. I won't have time to work on this any time soon anyway.

@jyn514 jyn514 closed this Nov 13, 2021
@jyn514 jyn514 deleted the release-repo branch November 13, 2021 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend Area: Webserver backend S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Releases by Stars" should only show one crate per github repository
2 participants