What happens when you set a slug to an existing objectid? Anything bad?

[Christopher Allan Webber]

Probably not, though that seems a bit nasty / tricky!

We might want to prevent someone from doing that though it probably
isn't a big deal.

[Will Kahn-Greene]

The original url for this bug was ​http://bugs.foocorp.net/issues/514 .

[Jakob Kramer]

Yes, something bad. If you set the slug of Media #2 to '1', you can't edit or delete Media #1 anymore. Maybe the uploader of Media #1 can even delete Media #2 like that…

[Christopher Allan Webber]

I have been thinking the "optionally look up by slug OR id" is a mistake. We should just require slugs (even if the same as the id, who cares) and look up via that only. Does that seem like a reasonable solution?

[Christopher Allan Webber]

Unless nobody objects, let's assume that. I'm marking this bug as bitesized.

[Jakob Kramer]

Replying to cwebber:

I have been thinking the "optionally look up by slug OR id" is a mistake. We should just require slugs (even if the same as the id, who cares) and look up via that only. Does that seem like a reasonable solution?

I am totally fine with this.

[Will Kahn-Greene]

I think that's prudent, too. I don't think an instance has to get very big for slug collisions. Heck, you only need one person to name their photo "untitled" and then everyone else is screwed.

[Jakob Kramer]

A problem with that solution is media without slugs. You could always have a default slug (the _id), but Elrond does not want that.

Another simple solution is to disallow all-digit slugs, but I wonder if this would be a good solution.

[Will Kahn-Greene]

Gandaro: How're you doing with this bug? It looks like it missed the 0.3.1 milestone. Do you think you could get this done in the next two weeks so it can make the 0.3.2 milestone?

[Jakob Kramer]

I am not able to finish this issue… Please also make sure that automatically created slugs don’t use all-digits, and write a migration that renames all “all-digit” slugs to something else… Maybe “-%d”? ​My work yet…

[Will Kahn-Greene]

Given that, bumping it out of the 0.3.2 milestone.

If someone wants to finish this up and can do it in the next two weeks, then please assign it to yourself and add it to the 0.3.2 milestone.

[Jakob Kramer]

The things that have to be done are:

• Writing a test for the “slugs must not be numbers” change
• Writing a migration for moving old slugs to some others (which format?)
• Make sure slugs are not named after numbers automatically

[spaetz]

Hi, I believe this bug should be solved rather sooner than later as its resolution will require the changing of medias permanent URLs and we don't want to do that once MediaGoblin has achieved widespread usage.

Here's what I learned so far:
Currently a media's permanent home can be reached via ID ('/u/spaetz/m/5/') or slug ('/u/spaetz/m/picofspaetz/'). The static images URL will contain the ID and the filename: '/mgoblin_media/media_entries/5/spaetz_small.png'.

Currently if a slug contains only numbers you might end up on a wrong page.

The resolution is: a) to forbid digit-only slugs and migrate already existing ones or b) to not care and simply use slugs _or_ ID numbers on a media home page. I agree that we might want to go for slugs. The problem with missing slugs can be solved by having a function akin to unique_filename (or somesuch) and make sure we always return a deterministic slug (what exactly is bad in the _ID scheme?).

Secondly, we need to decide whether the media URLs should still contain the ID as they do now or whether they also should be using a slug-based URL.

In any case, we should make the call asap to not break all permalinks that are out there when we change URLs.

[bigjust]

+1 for only allowing slug based urls. If we really want to allow IDs, it should be in a different path, so that there is no conflicts/confusion.

[spaetz]

How about ID ('/u/spaetz/i/5/') or slug ('/u/spaetz/m/picofspaetz/') type of URLs? This way there can be no ambiguity and we can deal with "slugless" media just fine by providing the id-based URL.

[bigjust]

sounds good to me.

[spaetz]

Please review and merge my branch 216_force_url_by_slug. It forces URLS to be by slug. Adding /u/USERNAME/m/id/IDNUM as additional media homepage can be done as a separate step.

[spaetz]

OK, ignore the branch. As discussed, this will need more work. The steps as outlined by Elrond are:

1) Enforce clean slugs without ":" and "/", currently they can be set to anything.
2) Enforce existance of slugs when uploading media
3) Migrate all slugs to clean slugs and set empty slugs to the media ID, so that m/IDN/ continues to work
4) Only make slug-based URLs work for at least user-facing pages
4b) If admin pages (delete, edit) should be slug based or id-based is still open for discussion

Does this sound like a plan?
Each step can be done one at a time...

[spaetz]

Please review branch 216_force_url_by_slug. It does these steps:

1) Enforce clean slugs without ":" and "/", currently they can be set to anything when editing media
2) Enforce existance of slugs when uploading media

[Christopher Allan Webber]

Okay, reviewed the branch. It looks good. (Note, I haven't tested it, just code reviewed it; I'll do a test when all the features are in place.)

I guess the main debate now is about whether or not we should be "forcing" a slug to exist when it doesn't have one yet. I have been for it, but Elrond has made a really good point: now that we're avoiding the "conflict" of both accepting an id or a slug for the same-formed URL. So basically, maybe instead of:

        #Is already a slug assigned? Check if it is valid
        if self.slug:
            self.slug = slugify(self.slug)
        elif self.title:
            #assign slug based on title
            self.slug = slugify(self.title)
        elif self.id:
            # Does the object already have an ID? (after adding to the session)
            self.slug = unicode(self.id)
        else:
            # Everything else failed, just use random garbage
            self.slug = unicode(uuid4())[1:4]

... well I wrote a branch with an alternate solution: :)

​https://gitorious.org/~cwebber/mediagoblin/cwebbers-mediagoblin/commits/216_cwebber_style_unique_slugs

I am going to make unit tests for this and actually test it and push that up. The good news is that everyone on IRC seems to agree it's a good idea! :)

The other thing is that we agreed on IRC that we should switch this from using a colon as a separator to an equals sign; so this branch should move to doing /id=foo/ instead of /id:foo/. Keeping colons out of the slug regexp though is still fine and good. :)

[Christopher Allan Webber]

Yahoo! Testing framework now works, pushed to my forementioned branch!

I feel pretty good about that implementation!

[Christopher Allan Webber]

At elrond's encouragement, 216_cwebber_style_unique_slugs is now merged!

Still plenty left to do on this ticket though, re: migrations and URLs at least :)

[Christopher Allan Webber]

Annnd it's all merged! Migration and new urls! We did switch back from equals to colon basically because the urls ended up looking nicer (the = was being escaped by urlgen or something).

We can close this out now! Woohoo!