Skip to content

Loading…

Fix Context#resolve for absolute paths #315

Closed
wants to merge 1 commit into from

3 participants

@route

Hi I've added checking for file existence when filename given as absolute path and also added tests for Context#resolve method.
This is backtrace for this issue:

ERROR NoMethodError: undefined method `mtime' for nil:NilClass
./sprockets/lib/sprockets/processed_asset.rb:128:in `block in build_dependency_paths'
./sprockets/lib/sprockets/processed_asset.rb:127:in `build_dependency_paths'
./sprockets/lib/sprockets/processed_asset.rb:17:in `initialize'
./sprockets/lib/sprockets/base.rb:340:in `new'
./sprockets/lib/sprockets/base.rb:340:in `block in build_asset'
./sprockets/lib/sprockets/base.rb:361:in `circular_call_protection'
./sprockets/lib/sprockets/base.rb:339:in `build_asset'
./sprockets/lib/sprockets/index.rb:93:in `block in build_asset'
./sprockets/lib/sprockets/caching.rb:51:in `cache_asset'
./sprockets/lib/sprockets/index.rb:92:in `build_asset'
./sprockets/lib/sprockets/base.rb:260:in `find_asset'
./sprockets/lib/sprockets/index.rb:60:in `find_asset'
./sprockets/lib/sprockets/bundled_asset.rb:16:in `initialize'
./sprockets/lib/sprockets/base.rb:343:in `new'
./sprockets/lib/sprockets/base.rb:343:in `build_asset'
./sprockets/lib/sprockets/index.rb:93:in `block in build_asset'
./sprockets/lib/sprockets/caching.rb:51:in `cache_asset'
./sprockets/lib/sprockets/index.rb:92:in `build_asset'
./sprockets/lib/sprockets/base.rb:260:in `find_asset'
./sprockets/lib/sprockets/index.rb:60:in `find_asset'
./sprockets/lib/sprockets/environment.rb:74:in `find_asset'
./sprockets/lib/sprockets/server.rb:47:in `call'
@route

Ow! I forgot to mention that this issue appears when you state depend_on for absolute path. BTW do we really need to raise exception if the file was removed when app is running and dependency for this file was stated?

@kirs

Several month ago I faced with this NoMethodError: undefined methodmtime' for nil:NilClass` exception and killed a lot of time to fix this issue. Today I think this checking is Must Have for Sprockets.

@josh

Something is fishy here.

@route

What do you mean?

@route

@josh I will explain the issue in details. I don't want to keep this PR opened if you aren't going to merge it. If you see code's defects I'll glad to fix it.

Case 1: You have depend_on directive in your manifest file (application.js for example) and it point to file with absolute path(less likely to be, because usually you're using relative paths)

Case 2: When you're using register_preprocessor and using context.depend_on(absolute_path_to_file) in the block. (more likely)

The result in both cases the same. If file exists when you declare depend_on and disappear later we'll see NoMethodError: undefined method `mtime' for nil:NilClass because this line https://github.com/sstephenson/sprockets/blob/master/lib/sprockets/context.rb#L83 hasn't any check.

@josh josh closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 7, 2012
  1. Fix Context#resolve for absolute paths

    Dmitriy Vorotilin committed
This page is out of date. Refresh to see the latest.
Showing with 45 additions and 19 deletions.
  1. +21 −19 lib/sprockets/context.rb
  2. +24 −0 test/test_context.rb
View
40 lib/sprockets/context.rb
@@ -77,32 +77,34 @@ def content_type
# # => "/path/to/app/javascripts/bar.js"
#
def resolve(path, options = {}, &block)
- pathname = Pathname.new(path)
- attributes = environment.attributes_for(pathname)
+ attributes = environment.attributes_for(path)
+ pathname = attributes.pathname
if pathname.absolute?
- pathname
-
- elsif content_type = options[:content_type]
- content_type = self.content_type if content_type == :self
-
- if attributes.format_extension
- if content_type != attributes.content_type
- raise ContentTypeMismatch, "#{path} is " +
- "'#{attributes.content_type}', not '#{content_type}'"
+ return pathname if pathname.exist?
+ else
+ if content_type = options[:content_type]
+ content_type = self.content_type if content_type == :self
+
+ if attributes.format_extension
+ if content_type != attributes.content_type
+ raise ContentTypeMismatch, "#{path} is " +
+ "'#{attributes.content_type}', not '#{content_type}'"
+ end
end
- end
- resolve(path) do |candidate|
- if self.content_type == environment.content_type_of(candidate)
- return candidate
+ resolve(path) do |candidate|
+ if self.content_type == environment.content_type_of(candidate)
+ return candidate
+ end
end
- end
- raise FileNotFound, "couldn't find file '#{path}'"
- else
- environment.resolve(path, {:base_path => self.pathname.dirname}.merge(options), &block)
+ else
+ return environment.resolve(path, {:base_path => self.pathname.dirname}.merge(options), &block)
+ end
end
+
+ raise FileNotFound, "couldn't find file '#{path}'"
end
# `depend_on` allows you to state a dependency on a file without
View
24 test/test_context.rb
@@ -8,6 +8,30 @@ def setup
@env.append_path(fixture_path('context'))
end
+ test "resolve logical path" do
+ logical_path = "bar.js"
+ path = File.join(fixture_path('context'), logical_path)
+ pathname = Pathname.new(path)
+ context = @env.context_class.new(@env, logical_path, pathname)
+
+ # absolute paths
+ assert_equal context.resolve(path), pathname
+ assert_raise Sprockets::FileNotFound do
+ context.resolve File.join(fixture_path('context'), "wrongname")
+ end
+
+ # with options
+ assert_equal context.resolve(logical_path, :content_type => "application/javascript"), pathname
+ assert_raise Sprockets::ContentTypeMismatch do
+ context.resolve logical_path, :content_type => "text/css"
+ end
+ assert_equal context.resolve(logical_path, :content_type => :self), pathname
+
+ # logical paths
+ assert_equal context.resolve(logical_path), pathname
+ assert_raise(Sprockets::FileNotFound) { context.resolve("wrongname") }
+ end
+
test "context environment is indexed" do
instances = @env["environment.js"].to_s.split("\n")
assert_match "Sprockets::Index", instances[0]
Something went wrong with that request. Please try again.