Opened 8 months ago

Closed 4 months ago

#32683 closed defect (fixed)

Relay Search should be able to handle non-numbers in "as:" parameter

Reported by: karsten Owned by: metrics-team
Priority: Medium Milestone:
Component: Metrics/Relay Search Version:
Severity: Normal Keywords:
Cc: metrics-team, arma Actual Points: 0.2
Parent ID: Points:
Reviewer: Sponsor:

Description

01:44:28 <+arma2> irl, karsten: 
                  https://metrics.torproject.org/rs.html#search/as:azure tells me i 
                  have a problem with my javascript. but my javascript is fine. maybe 
                  that "as:" syntax expects a number only?
06:33:53 <+karsten> arma2: yes, "as:" expects a number, but you can use "as_name:" if 
                    you have part of the name.
06:34:05 <+karsten> arma2: still, it shouldn't throw a javascript error at you. I'll 
                    open a ticket.
06:35:56 <+arma2> ah, and i guess part of your ticket should be: in the text input 
                  box on https://metrics.torproject.org/rs.html#advanced called "AS", 
                  maybe it should figure out whether you put in a name or a number

Child Tickets

Change History (4)

comment:1 Changed 8 months ago by arma

Cc: arma added

comment:2 Changed 5 months ago by karsten

Status: newneeds_review

I took a quick look at the code, and it seems it's mainly a problem of printing out a better error message. In this case Relay Search sends the query to Onionoo which returns a 400. But Relay Search doesn't properly identify the error message as client-side/query problem and prints out the wrong message.

Please review commit 4613554 in my task-32683 branch that contains a possible fix that prints out a new "Query error!" message for all 4xx responses. That's at least more accurate than saying it's a JavaScript error, even though it doesn't go into the details of saying which query parameter part is wrong.

Regarding the suggestion to figure out whether the user put in a name or a number in a field, I believe that Relay Search isn't doing much parameter checking before sending requests to Onionoo. Of course it would be possible to do the same checks that Onionoo does in the browser. But that's not a trivial amount of work to do once and to keep in sync. In any case that would be an enhancement, not a defect, and if we really care, we should open a new (enhancement) ticket for that.

comment:3 Changed 4 months ago by irl

Status: needs_reviewmerge_ready

#8667 is related.

Relay Search indeed doesn't do any validation of the query before sending it. We could return a JSON document along with 400 error codes, to allow Onionoo to provide a more detailed message back to the user, but better error messages help too.

It's a bit upsetting that the same error message is in multiple files, I should learn more about the template language and see if we can use includes.

comment:4 in reply to:  3 Changed 4 months ago by karsten

Actual Points: 0.2
Resolution: fixed
Status: merge_readyclosed

Replying to irl:

#8667 is related.

Looks related, yes. When I worked on this ticket I focused on defects and didn't spot that other ticket. I'll leave a comment there.

Relay Search indeed doesn't do any validation of the query before sending it. We could return a JSON document along with 400 error codes, to allow Onionoo to provide a more detailed message back to the user, but better error messages help too.

You mean that Onionoo could return such a JSON document to allow Relay Search to provide a more detailed message? I made a similar suggestion on #32065 shortly after commenting here. I think it's a good idea. Let's discuss that more on #32065.

It's a bit upsetting that the same error message is in multiple files, I should learn more about the template language and see if we can use includes.

Agreed, not having to repeat ourselves would be nice. Not the end of the world in this case, though.

Alright, merged and deployed. Thanks!

Note: See TracTickets for help on using tickets.