URL parse error when the name of a project ends in `.atom`

When a project name ends in .atom there is a URL generation error that causes the dashboard and the profile page of a user to give a 500 error.

This is the exact error being outputted.

ActionController::UrlGenerationError - No route matches {:action=>"show", :controller=>"projects", :id=>#<Project id: 117536, name: "a.atom", path: "a.atom", description: "", [...] missing required keys: [:id]:

Renaming the project to a-atom, in this case, solved the problem.

@yorickpeterse I believe you worked on the atom feeds recently. Can you have a look, please?

/cc @rspeicher @JobV

I suspect this is because Rails thinks anything with .extension is a format type:

http://stackoverflow.com/questions/2952235/rails-params-with-dot-e-g-google-com

The change I added for Atom feeds was related to how the timestamp of the latest event was retrieved, I didn't actually change anything in terms of request handling, routing, URL generation, etc. So sadly my guess is as good as any.

@JobV I don't suppose this was already fixed. Can you schedule it?

Milestone changed to 8.6

Reassigned to @jameslopez

@DouweM I think this started happening after https://gitlab.com/gitlab-org/gitlab-ce/commit/a6dfd065ca9de2e421425a5126c68bf8949ce110#e801163eb9182955dca3eb9817d59af634f6665d_299_298 - Undoing the change in L298 makes it work. What are your thoughts on this? Do we need that .atom for the #show action?

@jameslopez I think that without that (?<!\.atom) part, project atom feeds wouldn't work since the .atom extension would be regarded part of the project name instead of part of the feed route.

The only "solution" I can think of is to disallow the .atom extension in project paths, we already do that for .git for the same reason.

@DouweM thanks! That's ok, I am happy to do that change. An alternative would be to create a separate route for the atom feeds but wouldn't be very intuitive I guess...

@patricio is disabling the .extension for paths okay ?

@jameslopez

An alternative would be to create a separate route for the atom feeds but wouldn't be very intuitive I guess...

If we have two projects, user/proj and user/proj.atom, how do we know where gitlab.com/user/proj.atom should lead? That can't be solved with a separate route.

@DouweM

Ah, what I meant is user/proj and user/proj.atom would always lead to the project page. But something like user/proj/atom or user/proj.atom/atom will lead to the atom feed of the project, or some other rule. I didn't think this would be an option anyway as it's not very restfully plus wouldn't be consistent...

Another workaround would be to change the to_param method in models/project.rb and add some logic to check whether the project name ends with 1 ".atom" but I think we probably still need a new route in that case.

I think our best option is still disallowing the extension anyway...

@jameslopez We can't change the atom path to /atom since people will have this stored in their RSS readers.

Another workaround would be to change the to_param method in models/project.rb and add some logic to check whether the project name ends with 1 ".atom" but I think we probably still need a new route in that case.

That would still leave the gitlab.com/user/proj.atom ambiguity, wouldn't it?

I think our best option is still disallowing the extension anyway...

I agree. We need to migrate any existing .atom projects and note this in the release post and update doc.

@DouweM I assume that means creating a migration looking for projects ending in .atom in the DB - what do we do with them though? Renaming the project from a.atom to a-atom or something like is an option... but will mean also checking for the availability of that name, etc...

Are these projects actually functional/working though? as I assume the error would prevent people from using them...

@jameslopez You can check out http://gitlab.com/gitlab-org/gitlab-ce/blob/mr-merge-base/db/migrate/20150421120000_remove_periods_at_ends_of_usernames.rb for inspiration, where we did a similar thing for user names.

@DouweM thanks! I will have a look

mentioned in merge request !2651 (merged)

@DouweM Not sure I'm getting the business logic right here. Do I need to do the whole namespace thing? or can I just use gitlab_shell.mv_namespace(path_was, path) with the project path ? I've been digging into that for a bit but I am still not sure how the paths in the filesystem work for projects...

@jameslopez You're gonna want to look at the code for moving projects, not namespaces: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/models/project.rb#L705-736.

@DouweM ah! That's what I need. Thanks

Status changed to closed by commit ac923b4f

mentioned in commit ac923b4f

Milestone changed to 8.5

Status changed to reopened

mentioned in merge request !2712 (merged)

Missed a small bit (checking only for lowercase .atom, as .aTom should be fine - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2712

mentioned in merge request gitlab-com/www-gitlab-com!1434 (merged)

Status changed to closed by commit dblessing/gitlab-ce@ac923b4fb40da88d6c7907d08d4584d5d4a560e2

mentioned in commit a5c2e7a4

mentioned in issue #13742 (closed)

Mentioned in commit pfjason/gitlab-ce@ac923b4f

Mentioned in commit pfjason/gitlab-ce@a5c2e7a4

mentioned in merge request !14785