Re: pgarchives: strip angle brackets when checking for msgid search

On 10/05/2015 11:57 AM, Magnus Hagander wrote:
> 
> 
> On Sat, Oct 3, 2015 at 7:42 PM, Amir Rohan <amir.rohan@zoho.com
> <mailto:amir.rohan@zoho.com>> wrote:
> 
>     Hi Magnus,
> 
>     On 10/03/2015 07:31 PM, Magnus Hagander wrote:
>     > On Fri, Oct 2, 2015 at 7:29 PM, Amir Rohan <amir.rohan@zoho.com <mailto:amir.rohan@zoho.com>
>     > <mailto:amir.rohan@zoho.com <mailto:amir.rohan@zoho.com>>> wrote:
>     >
>     >     In some email-clients, when you copy the Message-ID to the clipboard
>     >     it surrounds in (or perhaps, doesn't strip) angle brackets.
>     >     When pasting this into the mail archive search, search fails to find
>     >     the message
>     >     and this happens often enough so as to be annoying. The fix is trivial.
>     >
>     >     Discussion of similar problem in the commitfest app:
>     >
>     >     <CABUevExRGr+Q5gzp-9cDLOqgntsOEcM0zZYg5VkADj3Z_NKz5A@mail.gmail.com
>     <mailto:CABUevExRGr%2BQ5gzp-9cDLOqgntsOEcM0zZYg5VkADj3Z_NKz5A@mail.gmail.com>>
>     >   
>      <mailto:CABUevExRGr+Q5gzp-9cDLOqgntsOEcM0zZYg5VkADj3Z_NKz5A@mail.gmail.com
>     <mailto:CABUevExRGr%2BQ5gzp-9cDLOqgntsOEcM0zZYg5VkADj3Z_NKz5A@mail.gmail.com>>
>     >
>     >     patch attached.
>     >
>     >
>     > Interesting - we also had a similar fix in 9b7e9b59 for the archives,
>     > but that one only deals with the URLs and not the search box. Makes much
>     > sense to do it in the search box as well, so applied. Thanks!
>     >
>     > Actually, before I sent this.
>     >It's also broken because it redirects to bad URLs if there are spaces
>     in it.
> 
>     What is "It"?
> 
>     > Or if there is just one of the two, > e.g. it has < but not >.
> 
>     I'm sorry, I lost you. Let's try and sync...
> 
>     Prior to this patch:
> 
>     "msgid" = works
>     " msgid " = fails
>     " msgid" = fails
>     "msgid " = fails
>     <msgid>" = fails
>     <msgid" = fails
>     "msgid>" = fails
>     " msgid>" = fails
>     etc'
> 
>     With this patch -- they should all work.
> 
>     If there's a search query that currently works and is broken by this
>     patch, can you give an example?
> 
> 
> 
> The search works. Meaning the API returns "this messageid exists", which
> causes the search client to send a redirect to an URL that still has the
> non-stripped version of the URL (including the < or space).
> 
> The regexp used for parsing URLs only allows either "msgid" or
> "<msgid>". Any other of those combinations will result in a 404 at the
> receiving end of that redirect.
>  
> 
>     > All those now generate 404s, which is not good.
>     >
> 
>     "Now" meaning before applying this patch?
> 
> 
> No, after. Today they render a 200 OK with "search returned no hits".
> 
>  
> 
> 
>     > This patch needs to be in sync with the callers of that API as well as
>     > the code that actually views entries in the archives before it can be
>     > safely applied.
> 
>     There is no API change. afaict nothing that currently works and uses
>     this endpoint should be affected. The only effect should be that
>     queries that now fail but should succeed, succeed.
> 
> 
> 
> That is exactly what I'm saying it doesn't. I deployed it and tested and
> it broke.
> 
> So either the API needs to be updated to return the actual URL, and the
> consumers (which at this point I believe are just the main website) need
> to be updated to actually use that URL, or the consumer also needs to be
> updated with the same "cleaning" rules, or the URL parsing in the
> archives code need to also deal with all those cases.
> 
> I think either the first, or a combination of first and third, of those
> are the way to go. The second one (doing the same changes in the pgweb
> code) seems to just put us in a position where we'll make the same
> mistake again next time we try to fix these rules.
> 

Ah, thanks for clearing that up, I understand now and I didn't think to
test that. I'll send an updated patch.

Amir