Opened 13 years ago
Closed 12 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 )
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 , 13 years ago
comment:2 by , 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…
comment:3 by , 12 years ago
Priority: | trivial → major |
---|
follow-up: 6 comment:4 by , 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 , 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.
comment:6 by , 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 , 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 , 12 years ago
Milestone: | → 0.3.1 |
---|---|
Owner: | set to |
comment:9 by , 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 , 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 , 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 , 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 , 12 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 , 12 years ago
Cc: | 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 , 12 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 , 12 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:18 by , 12 years ago
Owner: | removed |
---|---|
Status: | accepted → assigned |
comment:19 by , 12 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 , 12 years ago
Keywords: | review removed |
---|
comment:21 by , 12 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 , 12 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 , 12 years ago
Keywords: | review added |
---|
comment:24 by , 12 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: :)
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 , 12 years ago
Yahoo! Testing framework now works, pushed to my forementioned branch!
I feel pretty good about that implementation!
comment:26 by , 12 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 , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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!
The original url for this bug was http://bugs.foocorp.net/issues/514 .