Check if path is a Resolver before decorating it #1551

bquorning · 2016-02-11T12:11:53Z

When you prepend or append view paths, you can choose to do so with either a path (which may be looked up on the file system) or an instance of ActionView::Resolver which will then know how to do the lookup. We should not wrap ActionView::Resolver instances in an EmptyTemplateResolver.

I’m not sure where/whether to add a spec.

cc @pixeltrix (related to #1535)

JonRowe · 2016-02-12T05:45:18Z

lib/rspec/rails/view_rendering.rb

@@ -81,7 +81,11 @@ def append_view_path(new_path)
      private

        def _path_decorator(path)
-          EmptyTemplateResolver.new(path)
+          if path.is_a?(::ActionView::Resolver)
+            path


Why? If the point here is nullify view rendering surely it's better to continue to ignore this?

PR #1535 switched form using EmptyTemplatePathSetDecorator, a subclass of ActionView::Resolver, to using EmptyTemplateResolver, a subclass of ActionView::FileSystemResolver. This means that when I have called prepend_view_path with a Resolver instance instead of a string/path argument, I run into this error: path already is a Resolver class from FileSystemResolver#initialize: https://github.com/rails/rails/blob/v4.2.5.1/actionview/lib/action_view/template/resolver.rb#L324-L329

I must admit, I wasn’t entirely sure what EmptyTemplateResolver does. It seems to change the found templates to templates that look identical, but that always render an empty string when called.

I guess the same behavior can be applied if path is an instance of ActionView::Resolver. But we need to change the superclass of EmptyTemplateResolver to be ActionView::Resolver, right?

What we're trying to do is prevent the templates rendering, maybe this fix should be bypassing that check within empty template resolver?

fables-tales · 2016-02-13T14:56:29Z

/cc @pixeltrix is this actually something we want?

bquorning · 2016-02-15T09:35:01Z

I pushed another commit, which moves the type check into the EmptyTemplateResolver initializer. Its superclass ActionView::FileSystemResolver has a type check in the initializer for when path is a Resolver instance, so I guess the subclass should have the same.

Is super("") an acceptable approach?

@pixeltrix Your opinion would be very welcome.

(I will rebase, obviously, before a merge)

bquorning · 2016-02-15T11:14:55Z

lib/rspec/rails/view_rendering.rb

+          if path.is_a?(::ActionView::Resolver)
+            super("")
+          elsif ::Rails::VERSION::STRING < '3.1'
+            super(path)


Sigh… https://github.com/rails/rails/blob/v3.0.20/actionpack/lib/action_view/template/resolver.rb#L123-L124

fables-tales · 2016-02-15T14:38:15Z

@bquorning ok, I have some questions. Is this to solve the performance regression? If so, can you provide a benchmark which shows if this is faster? If not, does this fix some regression that was introduced? If so, can you provide a spec or other example for that case?

bquorning · 2016-02-15T14:49:14Z

@samphippen I meant to fix a regression. After #1535, it is no longer possible to call e.g. prepend_view_path with a Resolver instance.

I’m not really sure where to add the spec.

fables-tales · 2016-02-15T14:50:08Z

@bquorning could you write a spec literally anywhere in spec/ and push it up. Once I see it I can likely advise on where it should end up.

pixeltrix · 2016-02-16T04:15:25Z

The best thing to do is what we do in ActionView::PathSet - only typecast if it's a String or a Pathname. This means changing the _path_decorator method to this:

def _path_decorator(path)
  case path
  when Pathname, String
    EmptyTemplateResolver.new(path.to_s)
  else
    path
  end
end

That should minimise the risk of any breakage by us messing about with the internals.

@samphippen we fixed the performance regression in 3.4.2 AFAIK - if there's another please point me to it.

fables-tales · 2016-02-16T09:19:38Z

👍 thanks @pixeltrix, @bquorning can you update the implementation to follow @pixeltrix's advice. Thanks!

bquorning · 2016-02-16T10:44:29Z

Hmm, I don’t agree @pixeltrix. If the point is (as @JonRowe mentioned earlier) to nullify view rendering, then my custom resolver (here: path) needs to be wrapped in a resolver similar to EmptyTemplateResolver. My resolver will know how to find the template (like your super call in find_templates), but the templates found should still be handled with an EmptyTemplateHandler. Right?

bquorning · 2016-02-16T13:44:36Z

On the other hand, the current EmptyTemplateResolver probably solves more than 99% of the use cases. Let’s assume that users who prepend with a custom Resolver class are able to fix this problem themselves (perhaps using a null resolver), and I’m fine with the suggested solution.

When you prepend or append view paths, you can choose to do so with either a path (which may be looked up on the file system) or an instance of `ActionView::Resolver` which will then know how to do the lookup. We should only wrap paths (`String` or `Pathname` instances) in an `EmptyTemplateResolver`.

bquorning · 2016-02-16T13:48:03Z

lib/rspec/rails/view_rendering.rb

-      # Delegates find_all to the submitted path set and then returns templates
-      # with modified source
+      # Delegates find_templates to the submitted path set and then returns
+      # templates with modified source


I hope it’s ok that I fixed the documentation here, too?

bquorning · 2016-02-16T13:48:15Z

PR updated & rebased.

fables-tales · 2016-02-16T13:51:33Z

@bquorning I still think it'd be good to get a spec for this. If in doubt, just make a new spec file in the spec/ directory. Let me know if you need any other help with that 😄

pixeltrix · 2016-02-16T16:19:41Z

@bquorning I think burdening rspec-rails with the responsibility of nullifying the rendering of all custom template resolvers is a bit much to ask 😄

One possible option would be to look at somewhere else to nullify the rendering - I'll see if there's somewhere we can hook in.

bquorning · 2016-02-16T21:28:00Z

I’ve tried reading RSpec’s specs for Example and ExampleGroup, in order to add a spec to your controller_example_group_spec. But I cannot figure out how to get a before hook to be run. And I guess it’s the before block in ViewRendering we want to test.

I pushed what I’ve got so far.

JonRowe · 2016-02-16T23:13:01Z

To write a spec for this you need to do whatever behaviour causes a Resolver to be passed down to this method, hooks are irrelevant for that.

bquorning · 2016-02-19T12:23:49Z

While it would be nice to have spec coverage this change, I cannot seem to wrap my head around it. Testing a test framework is very meta :-)

bquorning · 2016-03-07T10:57:37Z

@samphippen If I remove EmptyTemplates#prepend_view_path or EmptyTemplates#append_view_path from view_rendering.rb, all current specs still pass. You’re asking me to write specs for an untested feature, and I have no idea where to start.

view_rendering worked for me before #1535, and it works for again after the change in this PR. Any change of merging it without test coverage?

JonRowe · 2016-03-07T11:01:02Z

I don't think that's a good idea, I'm convinced this breaks the default usage which is no view rendering.

bquorning · 2016-03-07T14:39:00Z

I'm convinced this breaks the default usage which is no view rendering.

How come? Isn’t the default usage to prepend with a Pathname or String?

JonRowe · 2016-03-07T20:00:35Z

How come? Isn’t the default usage to prepend with a Pathname or String?

This isn't accessed directly, but by your controllers when specs are setup so we don't know what we'll receive, but we know we want to return an EmptyTemplateResolver so as to nullify the lookup, this is the direct opposite. I worked up #1557 as an alternative which skips the check added by changing the superclass of the resolver.

bquorning · 2016-03-07T21:48:16Z

… we know we want to return an EmptyTemplateResolver so as to nullify the lookup, this is the direct opposite.

It is not. An EmptyTemplateResolver is still used for default usage. But if prepending/appending a custom resolver, yes, it is not nullified – following @pixeltrix’ advice:

I think burdening rspec-rails with the responsibility of nullifying the rendering of all custom template resolvers is a bit much to ask

I’ll take a look at #1557.

bquorning · 2016-03-07T21:48:55Z

By the way, the specs added in #1557 are all passing on this branch.

JonRowe · 2016-03-07T22:30:42Z

It is not. An EmptyTemplateResolver is still used for default usage. But if prepending/appending a custom resolver, yes, it is not nullified.

Which is an undesired change in the behaviour.

By the way, the specs added in #1557 are all passing on this branch.

Yes, the specs were regression checks to ensure errors weren't raised, it's actually quite hard to test this in isolation and I don't want to pollute the features for regression checks.

JonRowe · 2016-03-07T22:30:53Z

Closing in favour of #1557.

JonRowe reviewed Feb 12, 2016
View reviewed changes

bquorning reviewed Feb 15, 2016
View reviewed changes

bquorning force-pushed the prepend-resolvers branch from a440806 to b2d22e4 Compare February 16, 2016 13:47

bquorning reviewed Feb 16, 2016
View reviewed changes

bquorning force-pushed the prepend-resolvers branch from b17a13c to b2d22e4 Compare February 19, 2016 12:22

ramhoj mentioned this pull request Feb 22, 2016

append_view_path monkey patch breaks API #1556

Closed

JonRowe mentioned this pull request Feb 23, 2016

Fix empty view rendering #1557

Closed

JonRowe closed this Mar 7, 2016

bquorning deleted the prepend-resolvers branch March 8, 2016 08:25

bquorning mentioned this pull request Mar 22, 2016

Delegate properly to custom Resolver instances #1580

Merged

Uh oh!

Check if path is a Resolver before decorating it #1551

Check if path is a Resolver before decorating it #1551

Uh oh!

Conversation

bquorning commented Feb 11, 2016

Uh oh!

JonRowe Feb 12, 2016

Choose a reason for hiding this comment

Uh oh!

bquorning Feb 12, 2016

Choose a reason for hiding this comment

Uh oh!

bquorning Feb 12, 2016

Choose a reason for hiding this comment

Uh oh!

JonRowe Feb 13, 2016

Choose a reason for hiding this comment

Uh oh!

fables-tales commented Feb 13, 2016

Uh oh!

bquorning commented Feb 15, 2016

Uh oh!

bquorning Feb 15, 2016

Choose a reason for hiding this comment

Uh oh!

fables-tales commented Feb 15, 2016

Uh oh!

bquorning commented Feb 15, 2016

Uh oh!

fables-tales commented Feb 15, 2016

Uh oh!

pixeltrix commented Feb 16, 2016

Uh oh!

fables-tales commented Feb 16, 2016

Uh oh!

bquorning commented Feb 16, 2016

Uh oh!

bquorning commented Feb 16, 2016

Uh oh!

bquorning Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

bquorning commented Feb 16, 2016

Uh oh!

fables-tales commented Feb 16, 2016

Uh oh!

pixeltrix commented Feb 16, 2016

Uh oh!

bquorning commented Feb 16, 2016

Uh oh!

JonRowe commented Feb 16, 2016

Uh oh!

bquorning commented Feb 19, 2016

Uh oh!

bquorning commented Mar 7, 2016

Uh oh!

JonRowe commented Mar 7, 2016

Uh oh!

bquorning commented Mar 7, 2016

Uh oh!

JonRowe commented Mar 7, 2016

Uh oh!

bquorning commented Mar 7, 2016

Uh oh!

bquorning commented Mar 7, 2016

Uh oh!

JonRowe commented Mar 7, 2016

Uh oh!

JonRowe commented Mar 7, 2016

Uh oh!

Uh oh!