Opened 8 years ago

Closed 8 years ago

#7924 closed defect (fixed)

Compass: search by AS does not work

Reported by: mo Owned by: cwacek
Priority: Medium Milestone:
Component: Metrics/Compass Version:
Severity: Keywords:
Cc: gsathya, cwacek@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Child Tickets

Change History (14)

comment:1 Changed 8 years ago by karsten

Cc: gsathya cwacek@… added

The command-line version of Compass works correctly, and I vaguely remember the web version worked correctly before we merged #6612. cwacek, would you mind looking into this?

comment:2 in reply to:  1 Changed 8 years ago by cwacek

Owner: changed from gsathya to cwacek
Status: newaccepted

Replying to karsten:

The command-line version of Compass works correctly, and I vaguely remember the web version worked correctly before we merged #6612. cwacek, would you mind looking into this?

Yes I'll take a look.

comment:3 Changed 8 years ago by cwacek

Status: acceptedneeds_review

I've submitted a patch to my repository (https://github.com/cwacek/compass-1/tree/angularize) that fixes the issue.

comment:4 Changed 8 years ago by karsten

Status: needs_reviewneeds_revision

I tried your patch, but it doesn't seem to fix the issue.

However, I found out more about the bug: if you ask for "AS123456789", you get all relays in ASes 3 and 9; there are no relays in ASes 1, 2, 4, 5, 6, 7, or 8, but I guess they would be displayed as well. So, it looks like we treat the string as char array and look up relays in every single-digit AS. Sounds like an easy fix, but I didn't come up with one yet. Maybe you have an idea?

comment:5 in reply to:  4 Changed 8 years ago by cwacek

Replying to karsten:

I tried your patch, but it doesn't seem to fix the issue.

However, I found out more about the bug: if you ask for "AS123456789", you get all relays in ASes 3 and 9; there are no relays in ASes 1, 2, 4, 5, 6, 7, or 8, but I guess they would be displayed as well. So, it looks like we treat the string as char array and look up relays in every single-digit AS. Sounds like an easy fix, but I didn't come up with one yet. Maybe you have an idea?

Apologies for the delay - I was out of town this weekend.

You're correct, that was the issue, but I just double checked and it's fixed in my branch (AS123456789 correctly returns nothing). Did you apply this patch? https://github.com/cwacek/compass-1/commit/8c2259c944f8e737a7728c74e9e5dcd3e8534bf2

comment:6 Changed 8 years ago by karsten

Running 8c2259c on https://compass.torproject.org/ now, but the issue is still there. :/

comment:7 in reply to:  6 Changed 8 years ago by cwacek

Replying to karsten:

Running 8c2259c on https://compass.torproject.org/ now, but the issue is still there. :/

I just checked out the latest from https://git.torproject.org/compass.git, and the issue is fixed there as well. It looks like the code is correct, but the server must still be running the old version somehow. Maybe Apache is acting up - I'll dig further...

comment:8 Changed 8 years ago by karsten

I just set up a new VM on EC2 running Compass master (8c2259c), but the issue is still there. Hmmmmmm...

comment:9 in reply to:  8 ; Changed 8 years ago by cwacek

Status: needs_revisionneeds_review

Replying to karsten:

I just set up a new VM on EC2 running Compass master (8c2259c), but the issue is still there. Hmmmmmm...

Okay, that was ridiculous, but I think I've actually fixed it this time.

In versions below 2.7.3, shlex doesn't properly account for
unicode characters. This only occurred with Apache for some reason,
either because mod_wsgi uses Python_2.7.2+ or because Apache was
handing it Unicode strings.

This fix simply converts the string to ASCII before handing it to
shlex. Since the set of valid characters for an AS parameter are
'AS1234567890[],' and should always encode to ASCII, this should be
safe.

comment:10 in reply to:  9 Changed 8 years ago by karsten

Replying to cwacek:

Okay, that was ridiculous, but I think I've actually fixed it this time.

Yay, I can confirm that it works here, too.

gsathya, want to review the changes, including the new unit tests, before I merge and deploy?

comment:11 Changed 8 years ago by gsathya

I'm lost. Is there a branch with all the changes?

comment:13 Changed 8 years ago by gsathya

looks ok to me!

comment:14 Changed 8 years ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Merged and deployed. Closing. Thanks!

Note: See TracTickets for help on using tickets.