Re: Symlink redirection breaks FTP site re-organisation

On Wed, Nov 30, 2016 at 1:25 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Tuesday, November 29, 2016, Dave Page <dpage@pgadmin.org> wrote:
>>
>> On Thu, Jun 9, 2016 at 3:36 PM, Dave Page <dpage@pgadmin.org> wrote:
>> > On Tue, Jun 7, 2016 at 10:38 AM, Magnus Hagander <magnus@hagander.net>
>> > wrote:
>> >>
>> >>
>> >> On Mon, Jun 6, 2016 at 5:19 PM, Dave Page <dpage@pgadmin.org> wrote:
>> >>>
>> >>> On Mon, Jun 6, 2016 at 4:02 PM, Magnus Hagander <magnus@hagander.net>
>> >>> wrote:
>> >>> >
>> >>> >
>> >>> > On Mon, Jun 6, 2016 at 2:11 PM, Dave Page <dpage@pgadmin.org> wrote:
>> >>> >>
>> >>> >> Hi,
>> >>> >>
>> >>> >> Whilst re-organising the pgAdmin website in preparation for pgAdmin
>> >>> >> 4,
>> >>> >> it became clear to me that the way we currently handle symlinks on
>> >>> >> the
>> >>> >> website FTP browser is broken. For example, the current pgAdmin
>> >>> >> site
>> >>> >> has URLs like:
>> >>> >>
>> >>> >> https://www.postgresql.org/pgadmin3/release/v1.22.1/...
>> >>> >>
>> >>> >> I want to change this to:
>> >>> >>
>> >>> >> https://www.postgresql.org/pgadmin/pgadmin3/v1.22.1/...
>> >>> >>
>> >>> >> Thus I renamed/symlinked the pgadmin3 dir on Fendaus and the
>> >>> >> release
>> >>> >> dir on Paxsor (the pgAdmin master server), however, whilst when we
>> >>> >> encounter a symlink in the browser we re-write it to the target (so
>> >>> >> that: https://www.postgresql.org/pgadmin3/ is rewritten to
>> >>> >> https://www.postgresql.org/pgadmin/), we don't handle anything
>> >>> >> deeper
>> >>> >> than that, so https://www.postgresql.org/pgadmin/release is a 404,
>> >>> >> never mind anything below it.
>> >>> >>
>> >>> >> So, I'm thinking that we need to have the website stop rewriting
>> >>> >> the
>> >>> >> URLs, or at least generate the index pages under both paths, as you
>> >>> >> would see if you traversed the filesystem itself.
>> >>> >>
>> >>> >> In doing this, I think we should also use a different icon for
>> >>> >> symlinks, so users can see that they're following a path (and maybe
>> >>> >> display the filename as "pgadmin3 -> pgadmin" as well).
>> >>> >>
>> >>> >> Thoughts?
>> >>> >
>> >>> >
>> >>> > Every single one of those URLs is a 404... Which makes the
>> >>> > discussion a
>> >>> > bit
>> >>> > hard to follow. Is it correct to just inject a "/ftp/" in each case,
>> >>> > or
>> >>> > are
>> >>> > there other subtleties to consider?
>> >>>
>> >>> Err, yeah. Though I had to backout my changes of course as it broken
>> >>> links all over the place because of this problem.
>> >>>
>> >>> Here's a current example though:
>> >>>
>> >>> https://www.postgresql.org/ftp/pgadmin3/ - This is a "real" directory.
>> >>> https://www.postgresql.org/ftp/pgadmin/ - This is a symlink to
>> >>> pgadmin3 (i.e. the on-page link is to the URL above)
>> >>>
>> >>> On a filesystem, the following paths are effectively the same:
>> >>>
>> >>> /pub/pgadmin3/release/v1.22.1/
>> >>> /pub/pgadmin/release/v1.22.1/
>> >>>
>> >>> However, not on the website:
>> >>>
>> >>> https://www.postgresql.org/ftp/pgadmin3/release/v1.22.1/ (OK)
>> >>> https://www.postgresql.org/ftp/pgadmin/release/v1.22.1/ (404)
>> >>>
>> >>> This prevents me properly fixing the directory names, and using
>> >>> symlinks for backwards-compatibility.
>> >>>
>> >>
>> >> Ah, gotcha, no I'm following.
>> >>
>> >> I agree that a good way forward is to stop rewriting the URLs. However,
>> >> I
>> >> think the proper thing to do is to detect the case and generate a real
>> >> http
>> >> redirect response for those cases where there's a symlink in the path,
>> >> and
>> >> not generating the indexes in two different places.
>> >>
>> >> E.g. the link would still point to /ftp/pgadmin/, but hitting that
>> >> would
>> >> give you a 301 code sending you to /ftp/pgadmin3/.
>> >
>> > Yes, that seems reasonable.
>> >
>> >> There's no reason that can't fairly easily be made to work for deep
>> >> links as
>> >> well - and if we're going to change this around we should get both of
>> >> those
>> >> fixed at once.
>> >
>> > Yeah.
>>
>> OK, so here's a patch that implements that, and makes a couple of
>> other minor tweaks in passing. It does the following:
>>
>> 1) Checks each inbound path element to see if it exists as a directory
>> or symlink. If symlinks are present for any element, they are expanded
>> to their canonical paths. A temporary (not permanent - things may
>> move, which is kinda the point) redirect is then returned to the
>> resulting full canonical path.
>>
>> 2) Fixes sorting, so the parent directory link always appears at the top.
>>
>> 3) Shows symlinks in the file browser with a modified icon, and the
>> target - e.g. latest -> source/v9.6.1
>
>
> This seems like a good way to do it.
>
>
>>
>> It works nicely on my machine, and appears to resolve the problems I
>> had when trying to restructure with symlinks without breaking old
>> paths - but Magnus, please review :-)
>>
>
>
> Nitpicks:
>
> No need to do
> if x != '':
>    for y in x.split()
>
> you can just directly do for y in x.split() and it will run zero times if x
> is empty string.

Not according to my debugger - it will run once. If I do what you
suggest, /ftp/ becomes a 404. Leave it in and it works fine.

>
> We also don't generally use the django urlresolvers.reverse(). We probably
> should, but if so we should use it universally. Introducing it in just one
> place it may generate problems down the road. IIRC that happened at least
> once before, so I would suggest hardcoding the URL instead.

Urgh. That means it cannot be tested locally.

> Probably use directories.sort() rather than making a copy of it?

Yup.

> As said, those are mostly nitpicks :) Rest looks good. (haven't run any
> particular tests, but it looks fine)
>
> /Magnus
>
>
>
> --
>  Magnus Hagander
>  Me: http://www.hagander.net/
>  Work: http://www.redpill-linpro.com/



-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company