Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

deeply nested shallow routes broken from 4.1.2 fix #15783

Closed
smdern opened this Issue · 22 comments

5 participants

@smdern

This commit e10f26f breaks deeply nested routes. Specifically the deeply nested still requires the top most id for index/create

e.g.

namespace :asdf do
    resources :foo do
      resources :bar, shallow: true do
          resources :baz
      end
    end
end

produces:
image

the baz create and index path requires :foo_id

@rafaelfranca rafaelfranca added this to the 4.1.2 milestone
@pixeltrix
Owner

:cry: will look at it today - does it work if you place the shallow: true on resources :foo ?

@smdern

does it work if you place the shallow: true on resources :foo ?

Yes. If the top-most resource has shallow: true then it'll work.

@pixeltrix
Owner

Are you okay with that as a resolution - it's the way I intended things when I made the fix

@aaronjensen

@pixeltrix if that's so then the docs should change. The problem is that setting it on the top-most one sets it for all children. The idea before was the children could opt in, but they can't anymore.

Example:

    resources :foo do
      resources :bar, shallow: true do
          resources :baz
      end
      resources :deep
    end
@aaronjensen

Thinking about it, the docs probably should change. The above example is somewhat confusing because the shallow: true on :bar is saying you want :bar's routes to be shallow. It's ALSO saying that you want all of its children resources to have shallow routes. There doesn't seem to be a way to say you want :bar to be shallow but not :baz. Either two options are needed or something else...

@pixeltrix
Owner

You can set shallow: false on the children, can't you?

@smdern

You can set shallow: false on the children, can't you?

I tried doing that w/ no change in output.

@smdern

There should probably be some spec coverage to document the expected output and to ensure the behavior matches the rails docs.

@pixeltrix
Owner

I tried doing that w/ no change in output.

I'd consider that a bug - I'll check that out

There should probably be some spec coverage to document the expected output and to ensure the behavior matches the rails docs

The problem is getting the coverage without massively expanding the number of tests - the number of places where you can set :shallow, :shallow_prefix and :shallow_path quickly gets out of hand.

I'd prefer that they went away altogether in Rails 5.

@aaronjensen

You can set shallow: false on the children, can't you?

Even w/ that there's still the ambiguity, the problem is the option applies to self and children. It should apply to one or the other. If you want each of your children to be shallow you could maybe do:

resources :foo do
  shallow do
    resources :bar
  end
end

Which would save shallow: true as applying to only that single resources declaration it is declared on. Which would also mean that shallow: true on a top level node would be meaningless/an error.

I'd prefer that they went away altogether in Rails 5.

You'd prefer the :shallow option entirely would go away? would it be replaced by something else? or the default?

@pixeltrix
Owner

You'd prefer the :shallow option entirely would go away?

Use the standard methods to define the urls, e.g.

resources :posts do
  resources :comments, only: %i[index, create]
end

resources :comments, except: %i[index create]

It's clear then what you want

@smdern

but isn't that what :shallow is supposed to do for you?

@pixeltrix
Owner

The problem is that :shallow_path and :shallow_prefix got documented as public options you can pass to :scope so that makes the code convoluted - we'll see how the GSoC project to refactor Mapper helps to improve things and then make a decision.

@pixeltrix
Owner

It may be that we just deprecate those options in 4.2 and remove them in 5 and make the distinction between the shallow method and the :shallow option as you've described. At least that way it limits the number of code paths to test.

@pixeltrix
Owner

@rafaelfranca whilst there's a bug regarding the setting of shallow: false it's not a regression so isn't a blocker on release of 4.1.2

@pixeltrix pixeltrix modified the milestone: 4.1.3, 4.1.2
@aaronjensen

@pixeltrix so is the original issue then not a regression? It seems like the bug fix caused another bug (or a documentation bug?) We had to change code to upgrade to 4.1.2.

@pixeltrix
Owner

@aaronjensen unfortunately due to the way the mapper is implemented as soon as you create a nested scope the options are merged together so unless shallow is true when the scope is created the :shallow_path option is merged and can't be unpicked because some of that path may have come from a namespace and some from the parent resource. If the docs indicate otherwise then they should be fixed, yes.

However you should be able to set shallow: false and I'll fix that in the next release - I checked that it's broken in 3.2.x, 4.0.x, 4.1.x and master so it's not a blocker for the release.

Sorry to be the bearer of bad news but there's a sequence of fixes here and if I try and undo this then I'll just be introducing another bug. I did go through the various shallow scenarios to make sure that they were consistent - I just didn't think that someone would want to set shallow: false in a nested resource.

@pixeltrix pixeltrix self-assigned this
@aaronjensen

@pixeltrix no worries, we've made the change already, I was just mentioning it for the sake of others. Thanks for the explanation.

@rafaelfranca

Ok. Thank you for moving it from the release. I'll do another RC today.

@sebjacobs sebjacobs referenced this issue from a commit in sebjacobs/rails
@sebjacobs sebjacobs Generate shallow paths for all children of shallow resources.
Prior to this commit shallow resources would only generate paths for
non-direct children (with a nested depth greater than 1).

Take the following routes file.

    resources :blogs do
      resources :posts, shallow: true do
        resources :comments do
          resources :tags
        end
      end
    end

This would generate shallow paths for `tags` nested under `posts`,
e.g `/posts/:id/tags/`, however it would not generate shallow paths
for `comments` nested under `posts`, e.g `/posts/:id/comments/new`.

This commit changes the behaviour of the route mapper so that it
generate paths for direct children of shallow resources, for example
if you take the previous routes file, this will now generate
shallow paths for `comments` nested under `posts`, .e.g
`posts/:id/comments/new`.

This was the behaviour in Rails `4.0.4` however this was broken in
@jcoglan's fix for another routes related issue[1].

This also fixes an issue[2] reported by @smdern.

[1] rails@d0e5963
[2] rails#15783
3f92091
@pixeltrix pixeltrix referenced this issue from a commit
@sebjacobs sebjacobs Generate shallow paths for all children of shallow resources.
Prior to this commit shallow resources would only generate paths for
non-direct children (with a nested depth greater than 1).

Take the following routes file.

    resources :blogs do
      resources :posts, shallow: true do
        resources :comments do
          resources :tags
        end
      end
    end

This would generate shallow paths for `tags` nested under `posts`,
e.g `/posts/:id/tags/`, however it would not generate shallow paths
for `comments` nested under `posts`, e.g `/posts/:id/comments/new`.

This commit changes the behaviour of the route mapper so that it
generate paths for direct children of shallow resources, for example
if you take the previous routes file, this will now generate
shallow paths for `comments` nested under `posts`, .e.g
`posts/:id/comments/new`.

This was the behaviour in Rails `4.0.4` however this was broken in
@jcoglan's fix for another routes related issue[1].

This also fixes an issue[2] reported by @smdern.

[1] d0e5963
[2] #15783

(cherry picked from commit e972d34)

Conflicts:
	actionpack/CHANGELOG.md
67436b1
@pixeltrix pixeltrix referenced this issue from a commit
@sebjacobs sebjacobs Generate shallow paths for all children of shallow resources.
Prior to this commit shallow resources would only generate paths for
non-direct children (with a nested depth greater than 1).

Take the following routes file.

    resources :blogs do
      resources :posts, shallow: true do
        resources :comments do
          resources :tags
        end
      end
    end

This would generate shallow paths for `tags` nested under `posts`,
e.g `/posts/:id/tags/`, however it would not generate shallow paths
for `comments` nested under `posts`, e.g `/posts/:id/comments/new`.

This commit changes the behaviour of the route mapper so that it
generate paths for direct children of shallow resources, for example
if you take the previous routes file, this will now generate
shallow paths for `comments` nested under `posts`, .e.g
`posts/:id/comments/new`.

This was the behaviour in Rails `4.0.4` however this was broken in
@jcoglan's fix for another routes related issue[1].

This also fixes an issue[2] reported by @smdern.

[1] d0e5963
[2] #15783

(cherry picked from commit e972d34)

Conflicts:
	actionpack/CHANGELOG.md
1d94fa0
@pixeltrix pixeltrix referenced this issue from a commit
@sebjacobs sebjacobs Generate shallow paths for all children of shallow resources.
Prior to this commit shallow resources would only generate paths for
non-direct children (with a nested depth greater than 1).

Take the following routes file.

    resources :blogs do
      resources :posts, shallow: true do
        resources :comments do
          resources :tags
        end
      end
    end

This would generate shallow paths for `tags` nested under `posts`,
e.g `/posts/:id/tags/`, however it would not generate shallow paths
for `comments` nested under `posts`, e.g `/posts/:id/comments/new`.

This commit changes the behaviour of the route mapper so that it
generate paths for direct children of shallow resources, for example
if you take the previous routes file, this will now generate
shallow paths for `comments` nested under `posts`, .e.g
`posts/:id/comments/new`.

This was the behaviour in Rails `4.0.4` however this was broken in
@jcoglan's fix for another routes related issue[1].

This also fixes an issue[2] reported by @smdern.

[1] d0e5963
[2] #15783
e972d34
@chancancode
Owner

Closing this as it is fixed by #16033, if you think this is incorrect feel free to comment here and I'll reopen it for you!

cc @pixeltrix @rafaelfranca

@pixeltrix
Owner

@chancancode thanks - I forgot to close when I merged the fix.

@ttosch ttosch referenced this issue from a commit
@sebjacobs sebjacobs Generate shallow paths for all children of shallow resources.
Prior to this commit shallow resources would only generate paths for
non-direct children (with a nested depth greater than 1).

Take the following routes file.

    resources :blogs do
      resources :posts, shallow: true do
        resources :comments do
          resources :tags
        end
      end
    end

This would generate shallow paths for `tags` nested under `posts`,
e.g `/posts/:id/tags/`, however it would not generate shallow paths
for `comments` nested under `posts`, e.g `/posts/:id/comments/new`.

This commit changes the behaviour of the route mapper so that it
generate paths for direct children of shallow resources, for example
if you take the previous routes file, this will now generate
shallow paths for `comments` nested under `posts`, .e.g
`posts/:id/comments/new`.

This was the behaviour in Rails `4.0.4` however this was broken in
@jcoglan's fix for another routes related issue[1].

This also fixes an issue[2] reported by @smdern.

[1] d0e5963
[2] #15783

(cherry picked from commit e972d34)

Conflicts:
	actionpack/CHANGELOG.md
1f0b9c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.