Opened 13 years ago

Closed 11 years ago

#216 closed defect (fixed)

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

Reported by: Christopher Allan Webber Owned by:
Priority: major Milestone: 0.3.3
Component: programming Keywords: bitesized review
Cc: sebastian@… Parent Tickets:

Description (last modified by 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.



Change History (27)

comment:1 by Will Kahn-Greene, 12 years ago

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

comment:2 by Jakob Kramer, 12 years ago

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…

Last edited 12 years ago by Jakob Kramer (previous) (diff)

comment:3 by Jakob Kramer, 12 years ago

Priority: trivialmajor

comment:4 by Christopher Allan Webber, 12 years ago

Description: modified (diff)

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?

comment:5 by Christopher Allan Webber, 12 years ago

Keywords: bitesized added

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

If someone wants to pick up this task, we can talk about solutions on IRC.

Last edited 12 years ago by Christopher Allan Webber (previous) (diff)

in reply to:  4 comment:6 by Jakob Kramer, 12 years ago

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.

comment:7 by Will Kahn-Greene, 12 years ago

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.

comment:8 by Jakob Kramer, 12 years ago

Milestone: 0.3.1
Owner: set to Jakob Kramer

comment:9 by Jakob Kramer, 12 years ago

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.

comment:10 by Will Kahn-Greene, 12 years ago

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?

comment:11 by Jakob Kramer, 12 years ago

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…

comment:12 by Will Kahn-Greene, 12 years ago

Milestone: 0.3.1

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.

comment:13 by Jakob Kramer, 11 years ago

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

comment:14 by spaetz, 11 years ago

Cc: sebastian@… added

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.

comment:15 by bigjust, 11 years ago

+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.

comment:16 by spaetz, 11 years ago

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.

comment:17 by bigjust, 11 years ago

sounds good to me.

comment:18 by Jakob Kramer, 11 years ago

Owner: Jakob Kramer removed
Status: acceptedassigned

comment:19 by spaetz, 11 years ago

Keywords: review added
Milestone: 0.3.3

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.

comment:20 by spaetz, 11 years ago

Keywords: review removed

comment:21 by spaetz, 11 years ago

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...

comment:22 by spaetz, 11 years ago

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

comment:23 by spaetz, 11 years ago

Keywords: review added

comment:24 by Christopher Allan Webber, 11 years ago

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. :)

comment:25 by Christopher Allan Webber, 11 years ago

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

I feel pretty good about that implementation!

comment:26 by Christopher Allan Webber, 11 years ago

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 :)

comment:27 by Christopher Allan Webber, 11 years ago

Resolution: fixed
Status: assignedclosed

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!

Note: See TracTickets for help on using tickets.