Opened 7 years ago

Closed 6 years ago

#6864 closed defect (wontfix)

search shouldn't use space separated values

Reported by: gsathya Owned by: gsathya
Priority: Medium Milestone:
Component: pyonionoo Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

"If multiple search terms are given, separated by spaces, the intersection of all relays and bridges matching all search terms will be returned" -- space is escaped in the url, we should either use comma separated values or something like /summary?search=foo&search=bar

Child Tickets

Change History (8)

comment:1 Changed 7 years ago by karsten

We allowed space-separated values for the search parameter, because Atlas can easily forward its own search term to Onionoo.

Why do you want to change that to comma-separated values or multiple parameter values?

I think that if we want to change the search parameter, we'd still have to support the current way for at least two or three more months.

comment:2 Changed 7 years ago by gsathya

>>> import urllib
>>> urllib.urlencode({'search':'foo bar'})
'search=foo+bar'

Spaces get encoded as a '+', Atlas and Onionoo need to be fixed to use that. Spaces aren't a legal part of the querystring(not safe either). I figured it might be better to stick to the RESTful API conventions which is using "multiple parameter values", but if it's going to be a pain to change, then we should at least use url encoded spaces.

comment:3 in reply to:  2 ; Changed 7 years ago by karsten

Replying to gsathya:

Spaces get encoded as a '+', Atlas and Onionoo need to be fixed to use that. Spaces aren't a legal part of the querystring(not safe either). I figured it might be better to stick to the RESTful API conventions which is using "multiple parameter values", but if it's going to be a pain to change, then we should at least use url encoded spaces.

We can do either '+' or multiple parameter values. I'm fine with either. If you say multiple parameter values conform more to the RESTful API conventions, let's do that. (Do you have a link stating that?)

Oh, this just comes to mind: it may be that we didn't use '+', because Tomcat didn't support it for some reason. That limitation will go away once we switch to Pyonionoo. Tomcat does support multiple parameter values, though.

We just need to keep supporting the current space-separated values for another two or three months in parallel to that, or we'll break existing applications without prior notice.

comment:4 Changed 7 years ago by gsathya

Component: Onionoopyonionoo
Owner: set to gsathya

comment:5 in reply to:  3 ; Changed 7 years ago by gsathya

Component: pyonionooOnionoo

Replying to karsten:

Replying to gsathya:

Spaces get encoded as a '+', Atlas and Onionoo need to be fixed to use that. Spaces aren't a legal part of the querystring(not safe either). I figured it might be better to stick to the RESTful API conventions which is using "multiple parameter values", but if it's going to be a pain to change, then we should at least use url encoded spaces.

We can do either '+' or multiple parameter values. I'm fine with either. If you say multiple parameter values conform more to the RESTful API conventions, let's do that. (Do you have a link stating that?)

Oh, this just comes to mind: it may be that we didn't use '+', because Tomcat didn't support it for some reason. That limitation will go away once we switch to Pyonionoo. Tomcat does support multiple parameter values, though.

We just need to keep supporting the current space-separated values for another two or three months in parallel to that, or we'll break existing applications without prior notice.

Actually with Pyonionoo, the '+' is automagically converted to space by Cyclone, so we can continue supporting both :) I don't think it's necessary to support multiple parameter value option. The '+' is a more accurate representation since we're actually doing an intersection operation.

I guess Onionoo should parse the '+'.

comment:6 in reply to:  5 ; Changed 7 years ago by karsten

Component: Onionoopyonionoo

Replying to gsathya:

Actually with Pyonionoo, the '+' is automagically converted to space by Cyclone, so we can continue supporting both :) I don't think it's necessary to support multiple parameter value option. The '+' is a more accurate representation since we're actually doing an intersection operation.

Okay, great! Now, that was easy. :)

I guess Onionoo should parse the '+'.

Turns out this is harder, because Tomcat doesn't like the "+" character in parameter values. But given that we're phasing out Onionoo's front-end quite soon, this doesn't matter much. Here's what I think we should do:

  1. Deploy Pyonionoo's front-end once it provides the same functionality as Onionoo's front-end.
  2. Add "+"-separated search terms to the specification, while still supporting space-separated search terms. Announce on the protocol page that space-separated searching will stop working two months later.
  3. Update Atlas (and any other Onionoo clients) to use "+"-separated search terms.
  4. Stop supporting space-separated search terms in Pyonionoo and update the protocol page to say so.

Does that make sense? Changing back to the pyonionoo component.

comment:7 in reply to:  6 Changed 7 years ago by gsathya

Status: newaccepted

Replying to karsten:

  1. Deploy Pyonionoo's front-end once it provides the same functionality as Onionoo's front-end.
  2. Add "+"-separated search terms to the specification, while still supporting space-separated search terms. Announce on the protocol page that space-separated searching will stop working two months later.
  3. Update Atlas (and any other Onionoo clients) to use "+"-separated search terms.
  4. Stop supporting space-separated search terms in Pyonionoo and update the protocol page to say so.

Does that make sense? Changing back to the pyonionoo component.

Sounds good :)

comment:8 Changed 6 years ago by karsten

Resolution: wontfix
Status: acceptedclosed

This ticket seems irrelevant now, because nobody has been working on pyonionoo for 16 months. Closing.

Note: See TracTickets for help on using tickets.