Hides crates on index calls unless they have at least one available v… #985
Conversation
|
This excludes yanked crates from ALL functionality that is powered by the index api route, which unfortunately is a lot:
Issue #985 was intended to be just excluding yanked crates from user pages and team pages. This comment of mine on #145 explains why I don't think we should just hide yanked crates everywhere all the time without a way to opt-out of doing that. If you'd like to just fix #958, could you change this PR to only include this Thinking about the query, could this be done with a LEFT JOIN that has a condition that the version is not yanked, and then the version must not be null? I haven't tried this yet, so it might not work, let me know if what I'm saying makes sense at all and I can elaborate. |
|
Definitely! Gonna fix and send it again later today! |
src/krate.rs
Outdated
| @@ -564,6 +564,10 @@ pub fn index(req: &mut Request) -> CargoResult<Response> { | |||
|
|
|||
| let recent_downloads = sql::<Nullable<BigInt>>("SUM(crate_downloads.downloads)"); | |||
|
|
|||
| let not_yanked_versions = sql::<Bool>( | |||
| "crates.id = ANY (SELECT vs.crate_id FROM versions vs WHERE vs.crate_id = crates.id AND vs.yanked IS FALSE)", | |||
There was a problem hiding this comment.
This could also be left_join(crates_versions.on(crates::id.eq(crates_versions::crate_id).and(crates_versions::yanked.eq(false)))).filter(crates_versions::id.is_null())
mauricio
force-pushed
the
issue-958
branch
2 times, most recently
from
d7f9385
to
059b87b
Compare
|
@carols10cents updated and included a new test for the no @sgrif couldn't get that to compile, couldn't find a table/class |
…ersion This makes the index call only show crates that have at least one available version. Crates that have all versions yanked will not show up anymore. Fixes rust-lang#958
|
@carols10cents so I tried to add a new join here but changing this changed the whole return type and then it doesn't compile anymore. Couldn't find any other examples on diesel's documentation on how to do it. Do you have any tips? |
|
@mauricio could you show me what you tried, explain what you're trying to do, and the error you were getting please? |
Perhaps we should defer this until we're pointing at Diesel master (soon)? Subselects can reference the outer table on master. |
| .any(|s| s.is_some()) | ||
| { | ||
| let not_yanked_versions = sql::<Bool>( | ||
| "crates.id = ANY (SELECT vs.crate_id FROM versions vs WHERE vs.crate_id = crates.id AND vs.yanked IS FALSE)", |
There was a problem hiding this comment.
Should this just be EXISTS (SELECT 1 FROM versions WHERE versions.crate_id = crates.id AND versions.yanked IS FALSE)?
| @@ -696,6 +696,17 @@ pub fn index(req: &mut Request) -> CargoResult<Response> { | |||
| )); | |||
| } | |||
|
|
|||
| if vec![params.get("user_id"), params.get("team_id")] | |||
There was a problem hiding this comment.
Do you think this is worth the complexity of sticking it in a vec and iterating over it, vs params.get("user_id").is_some() || params.get("team_id").is_some()?
|
@sgrif that would be much better! Much less magic to handle here. |
|
This has gotten quite stale so I'm going to go ahead and close it. Feel free to open a new PR if you're interested in reviving this. Implementing this feature should no longer require dropping down to raw SQL |
…ersion
This makes the index call only show crates that have at least one available version. Crates that have all versions yanked will not show up anymore.
Main problem for the implementation here is that
dieseldoesn't allow you to build a subquery that references a field from the parent query, so the only way to get this done is through pure SQL.Fixes #958