rust-lang / crates.io

The Rust package registry
https://crates.io
Apache License 2.0
2.87k stars 588 forks source link

Exact match not being obvious #493

Closed Keats closed 7 years ago

Keats commented 7 years ago

When doing a search, a crate matching exactly the search will appear first, even if sorted by downloads. I initially thought it was a bug: https://github.com/rust-lang/crates.io/issues/396#issuecomment-265080412

Would it be possible to extract the exact match out of the list and display it differently?

carols10cents commented 7 years ago

Changing this bug's level to medium, because it involves a bunch of little pieces scattered all over. I think each of the pieces individually should be easy, but putting them all together might take a little while.

The pieces:

select id, name, name = 'sassers' as exact_match from crates;
 id |      name       | exact_match
----+-----------------+-------------
  2 | sassers         | t
  6 | piston          | f

If anyone wants to pick off a piece of this, I'd accept PRs for each little bit separately-- I don't think any of these individually should break other things (famous last words ;) )

venkatagiri commented 7 years ago

@carols10cents I wanted to work on this and ran cargo test before making any changes and few tests are failing trying to upload to S3 bucket. Logs are here http://pastebin.com/Hrei2g7H README says "No actual requests to s3 will be made". Not sure what's wrong with my setup.

carols10cents commented 7 years ago

Hi @venkatagiri, there is something weird with the .env file and the way crates.io uses env vars that I was hoping to fix before I needed to document it but I haven't yet :( Could you try changing your .env file to have export S3_REGION= instead of whatever you have for that variable?

If you already have that, could you pastebin your .env file, without any usernames, passwords, or keys of course?

Sorry about this! It's totally not your fault.

venkatagiri commented 7 years ago

The S3_REGION variable is empty. Here's the .env file I am using. http://pastebin.com/gLDh31Dn Just FYI, I have PostgreSQL running in a docker container.

If you can point me to the piece of code where the S3 calls get mocked, I will take a go at debugging this and fix it.

carols10cents commented 7 years ago

Hmm, strange. So in tests, the Config that gets used is defined here, getting S3_REGION from the environment: https://github.com/rust-lang/crates.io/blob/4f9aeb3ca1905fcb44fed18da0e4f8c769217a23/src/tests/all.rs#L86

The s3_proxy set in the config in the tests comes from record::proxy() The src/tests/record.rs file implements the mocking-- if the tests are run in recording mode, it stores a file of request/response data per test in src/tests/http-data. Then in replay mode (most of the time), when the config gets a request to s3, it tries to play back the data from the file for that test. The error you're seeing comes from this spot in record.rs that validates the request headers, but for some reason your app is creating requests to http://alexcrichton-test.s3-.amazonaws.com and the http-data files have requests to http://alexcrichton-test.s3.amazonaws.com recorded, so the headers don't match up.

This is where the S3 host gets constructed, so the http-data expect S3_REGION to be None but for some reason when you run the tests S3_REGION is Some(""), is what I think the problem is.

Something to try might be removing S3_REGION from your .env file completely, but I'd love to see a more robust solution to this problem. This is kind of related to #470... for some reason that I haven't completely figured out how to change yet, I have to set S3_REGION="" for running the server locally and S3_REGION= when running the tests :(

venkatagiri commented 7 years ago

Yes. It seems to return Some("") for S3_REGION. Modifying it with below changes fixes the issue with cargo test.

Is this change fine enough for a pull request or is there a simpler way to fix it?

vagrant@jessie crates.io $ gd
diff --git a/src/s3/lib.rs b/src/s3/lib.rs
index f4a43af..aa7d824 100644
--- a/src/s3/lib.rs
+++ b/src/s3/lib.rs
@@ -99,7 +99,8 @@ impl Bucket {
     pub fn host(&self) -> String {
         format!("{}.s3{}.amazonaws.com", self.name,
                 match self.region {
-                    Some(ref r) => format!("-{}", r),
+                    Some(ref r) if r != "" => format!("-{}", r),
+                    Some(_) => String::new(),
                     None => String::new(),
                 })
     }
carols10cents commented 7 years ago

That looks great to me! I might end up rearranging things if I figure out what I want to do about #470, but this looks like it would fix the problem for now. Please send this change in a PR and I'll merge! Thank you for your patience!! ❤️

carols10cents commented 7 years ago

Hi @venkatagiri, have you made any progress on this? I don't want to steal it from you if you have, but I'm thinking about using this bug at an OSS hack fest at a local college on Saturday, April 8, since it splits up into pieces nicely.

If you have any part that you'd like to submit, I'd love PRs for small pieces! Please let me know if you have any pieces by Saturday, so that I'm not duplicating any of your work.

If you haven't had a chance to make progress on this, no worries :) I'm happy to help you find something else whenever you have time! ❤️

venkatagiri commented 7 years ago

@carols10cents, I didn't get a chance to work on this. So, go ahead and use this at the hack fest.

carols10cents commented 7 years ago

To get some crates into your database to be able to test, start a psql session with:

psql

Then connect to the database crates.io uses with:

\c cargo_registry

Then insert some crates, named whatever you want. This will insert a crate named "hello":

insert into crates (name) values ('hello');

Then quit postgres with:

\q
carols10cents commented 7 years ago

Fixed by #673, #674, and #675 by students at the CyberWVU FOSSFest 2017!!!!! 🥇 🥇 🥇 🥇 🥇