Bash expressions do not fail 'single entry' check

nebhale · nebhale · commit 01303b8fa3a5 · 2014-03-20T14:06:38.000Z
Previously, if the value of a JAVA_OPTS was a bash expression (like in
the AppDynamics Framework), it would run afoul of the enforcement that
values in JAVA_OPTS must be a 'single entry'.  It would, however, only
cause a problem if a custom JAVA_OPT was specified as well (via the
java_opts.yml file). This change updates the detection logic for the
'single entry' so that it makes an exception for bash expressions and
ensures that the check is only run when Play applications are being
pushed.

[#67900270]
diff --git a/.idea/copyright/profiles_settings.xml b/.idea/copyright/profiles_settings.xml
diff --git a/.idea/runConfigurations/All_Tests.xml b/.idea/runConfigurations/All_Tests.xml
diff --git a/.idea/runConfigurations/Without_Integration_Tests.xml b/.idea/runConfigurations/Without_Integration_Tests.xml
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -11,7 +11,7 @@ GEM
     diff-lcs (1.2.5)
     docile (1.1.3)
     json (1.8.1)
-    multi_json (1.9.0)
+    multi_json (1.9.2)
     parser (2.1.7)
       ast (~> 1.1)
       slop (~> 3.4, >= 3.4.5)
@@ -31,7 +31,7 @@ GEM
     rspec-mocks (3.0.0.beta2)
       rspec-support (= 3.0.0.beta2)
     rspec-support (3.0.0.beta2)
-    rubocop (0.19.0)
+    rubocop (0.19.1)
       json (>= 1.7.7, < 2)
       parser (~> 2.1.7)
       powerpack (~> 0.0.6)
diff --git a/java-buildpack.iml b/java-buildpack.iml
@@ -277,7 +277,7 @@
     <orderEntry type="library" scope="PROVIDED" name="diff-lcs (v1.2.5, rbenv: 1.9.3-p545) [gem]" level="application" />
     <orderEntry type="library" scope="PROVIDED" name="docile (v1.1.3, rbenv: 1.9.3-p545) [gem]" level="application" />
     <orderEntry type="library" scope="PROVIDED" name="json (v1.8.1, rbenv: 1.9.3-p545) [gem]" level="application" />
-    <orderEntry type="library" scope="PROVIDED" name="multi_json (v1.9.0, rbenv: 1.9.3-p545) [gem]" level="application" />
+    <orderEntry type="library" scope="PROVIDED" name="multi_json (v1.9.2, rbenv: 1.9.3-p545) [gem]" level="application" />
     <orderEntry type="library" scope="PROVIDED" name="parser (v2.1.7, rbenv: 1.9.3-p545) [gem]" level="application" />
     <orderEntry type="library" scope="PROVIDED" name="powerpack (v0.0.9, rbenv: 1.9.3-p545) [gem]" level="application" />
     <orderEntry type="library" scope="PROVIDED" name="rainbow (v2.0.0, rbenv: 1.9.3-p545) [gem]" level="application" />
diff --git a/lib/java_buildpack/framework/java_opts.rb b/lib/java_buildpack/framework/java_opts.rb
@@ -39,22 +39,13 @@ def compile
 
       # @macro base_component_release
       def release
-        java_opts = @droplet.java_opts
-
-        check_single_options java_opts
-        java_opts.concat parsed_java_opts
+        @droplet.java_opts.concat parsed_java_opts
       end
 
       private
 
       CONFIGURATION_PROPERTY = 'java_opts'.freeze
 
-      def check_single_options(opts)
-        opts.each do |option|
-          fail "Invalid Java option contains more than one option: '#{option}'" if option.shellsplit.length > 1
-        end
-      end
-
       def memory_option?(option)
         option =~ /-Xms/ || option =~ /-Xmx/ || option =~ /-XX:MaxMetaspaceSize/ || option =~ /-XX:MaxPermSize/ ||
           option =~ /-Xss/ || option =~ /-XX:MetaspaceSize/ || option =~ /-XX:PermSize/
diff --git a/lib/java_buildpack/util/play/post22.rb b/lib/java_buildpack/util/play/post22.rb
@@ -17,6 +17,7 @@
 require 'java_buildpack/util/play'
 require 'java_buildpack/util/play/base'
 require 'java_buildpack/util/start_script'
+require 'shellwords'
 
 module JavaBuildpack
   module Util
@@ -39,7 +40,15 @@ def augment_classpath
 
         # @macro base_java_opts
         def java_opts
-          @droplet.java_opts.map { |java_opt| "-J#{java_opt}" }
+          java_opts = @droplet.java_opts
+
+          java_opts.each do |option|
+            if option.shellsplit.length > 1 && !bash_expression?(option)
+              fail "Invalid Java option contains more than one option: '#{option}'"
+            end
+          end
+
+          java_opts.map { |java_opt| "-J#{java_opt}" }
         end
 
         # @macro base_lib_dir
@@ -62,6 +71,12 @@ def root
           fail "Method 'root' must be defined"
         end
 
+        private
+
+        def bash_expression?(option)
+          option =~ /\$\(expr/
+        end
+
       end
 
     end
diff --git a/lib/java_buildpack/util/play/pre22.rb b/lib/java_buildpack/util/play/pre22.rb
@@ -25,13 +25,16 @@ class Pre22 < Base
 
         protected
 
+        # @macro base_java_opts
+        def java_opts
+          @droplet.java_opts
+        end
+
         # @macro base_start_script
         def start_script
           root ? root + 'start' : nil
         end
 
-        protected
-
         # @!macro [new] pre22_root
         #   Returns the root of the play application
         #
diff --git a/lib/java_buildpack/util/play/pre22_dist.rb b/lib/java_buildpack/util/play/pre22_dist.rb
@@ -38,11 +38,6 @@ def augment_classpath
           end
         end
 
-        # @macro base_java_opts
-        def java_opts
-          @droplet.java_opts
-        end
-
         # @macro base_lib_dir
         def lib_dir
           root + 'lib'
diff --git a/lib/java_buildpack/util/play/pre22_staged.rb b/lib/java_buildpack/util/play/pre22_staged.rb
@@ -30,11 +30,6 @@ def augment_classpath
           @droplet.additional_libraries.link_to lib_dir
         end
 
-        # @macro base_java_opts
-        def java_opts
-          @droplet.java_opts
-        end
-
         # @macro base_lib_dir
         def lib_dir
           root + 'staged'
diff --git a/spec/java_buildpack/framework/java_opts_spec.rb b/spec/java_buildpack/framework/java_opts_spec.rb
@@ -105,12 +105,4 @@
     end
   end
 
-  context do
-    let(:java_opts) { super() << '-Xmx30m -Xms30m' }
-
-    it 'should not allow multiple options in a single array entry' do
-      expect { component.release }.to raise_error(/Invalid Java option contains more than one option/)
-    end
-  end
-
 end
diff --git a/spec/java_buildpack/util/play/post22_dist_spec.rb b/spec/java_buildpack/util/play/post22_dist_spec.rb
@@ -21,11 +21,6 @@
 describe JavaBuildpack::Util::Play::Post22Dist do
   include_context 'component_helper'
 
-  before do
-    java_home
-    java_opts
-  end
-
   context do
 
     let(:trigger) { described_class.new(droplet).supports? }
diff --git a/spec/java_buildpack/util/play/post22_spec.rb b/spec/java_buildpack/util/play/post22_spec.rb
@@ -24,11 +24,6 @@
 
   let(:play_app) { described_class.new(droplet) }
 
-  before do
-    java_home
-    java_opts
-  end
-
   it 'should raise error if root method is unimplemented' do
     expect { play_app.send(:root) }.to raise_error "Method 'root' must be defined"
   end
@@ -55,6 +50,26 @@
                                        '-Jtest-opt-2 -Jtest-opt-1 -J-Dhttp.port=$PORT')
     end
 
+    context do
+      let(:java_opts) { super() << '-Xmx30m -Xms30m' }
+
+      it 'should not allow multiple options in a single JAVA_OPTS array entry' do
+        expect { play_app.release }.to raise_error(/Invalid Java option contains more than one option/)
+      end
+    end
+
+    context do
+      let(:java_opts) do
+        super() << '-Dappdynamics.agent.nodeName=$(expr "$VCAP_APPLICATION" : \'.*instance_id[": ]*"\([a-z0-9]\+\)".*\')'
+      end
+
+      it 'should allow options with expressions' do
+        play_app.release
+
+        expect(java_opts).to include('-Dappdynamics.agent.nodeName=$(expr "$VCAP_APPLICATION" : \'.*instance_id[": ]*"\([a-z0-9]\+\)".*\')')
+      end
+    end
+
   end
 
 end
diff --git a/spec/java_buildpack/util/play/post22_staged_spec.rb b/spec/java_buildpack/util/play/post22_staged_spec.rb
@@ -21,11 +21,6 @@
 describe JavaBuildpack::Util::Play::Post22Staged do
   include_context 'component_helper'
 
-  before do
-    java_home
-    java_opts
-  end
-
   context do
 
     let(:trigger) { described_class.new(droplet).supports? }
diff --git a/spec/java_buildpack/util/play/pre22_dist_spec.rb b/spec/java_buildpack/util/play/pre22_dist_spec.rb
@@ -21,11 +21,6 @@
 describe JavaBuildpack::Util::Play::Pre22Dist do
   include_context 'component_helper'
 
-  before do
-    java_home
-    java_opts
-  end
-
   context do
 
     let(:trigger) { described_class.new(droplet).supports? }
diff --git a/spec/java_buildpack/util/play/pre22_staged_spec.rb b/spec/java_buildpack/util/play/pre22_staged_spec.rb
@@ -21,11 +21,6 @@
 describe JavaBuildpack::Util::Play::Pre22Staged do
   include_context 'component_helper'
 
-  before do
-    java_home
-    java_opts
-  end
-
   context do
 
     let(:trigger) { described_class.new(droplet).supports? }
