Re: Symlink redirection breaks FTP site re-organisation

On Tuesday, November 29, 2016, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> On Thu, Jun 9, 2016 at 3:36 PM, Dave Page <dpage(at)pgadmin(dot)org
> <javascript:;>> wrote:
> > On Tue, Jun 7, 2016 at 10:38 AM, Magnus Hagander <magnus(at)hagander(dot)net
> <javascript:;>> wrote:
> >>
> >>
> >> On Mon, Jun 6, 2016 at 5:19 PM, Dave Page <dpage(at)pgadmin(dot)org
> <javascript:;>> wrote:
> >>>
> >>> On Mon, Jun 6, 2016 at 4:02 PM, Magnus Hagander <magnus(at)hagander(dot)net
> <javascript:;>>
> >>> wrote:
> >>> >
> >>> >
> >>> > On Mon, Jun 6, 2016 at 2:11 PM, Dave Page <dpage(at)pgadmin(dot)org
> <javascript:;>> 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.

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.

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

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/