Use Array.join rather than URI.join

Add spec for cases where URI.join does not work as expected

3 files changed, 31 insertions, 17 deletions

diff --git a/app/models/environment.rb b/app/models/environment.rb
index 4d48c6c16d3..513427ac2c5 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -204,7 +204,7 @@ class Environment < ApplicationRecord
     public_path = project.public_path_for_source_path(path, commit_sha)
     return unless public_path
 
-    URI.join(external_url, public_path).to_s
+    [external_url.delete_suffix('/'), public_path.delete_prefix('/')].join('/')
   end
 
   def expire_etag_cache
diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb
index 8e0adcbbf9d..2b67f6d7e92 100644
--- a/spec/models/environment_spec.rb
+++ b/spec/models/environment_spec.rb
@@ -782,12 +782,9 @@ describe Environment, :use_clean_rails_memory_store_caching do
     let(:source_path) { 'source/file.html' }
     let(:sha) { RepoHelpers.sample_commit.id }
 
-    before do
-      environment.external_url = 'http://example.com'
-    end
-
     context 'when the public path is not known' do
       before do
+        environment.external_url = 'http://example.com'
         allow(project).to receive(:public_path_for_source_path).with(source_path, sha).and_return(nil)
       end
 
@@ -797,18 +794,25 @@ describe Environment, :use_clean_rails_memory_store_caching do
     end
 
     context 'when the public path is known' do
-      before do
-        allow(project).to receive(:public_path_for_source_path).with(source_path, sha).and_return('file.html')
-      end
-
-      it 'returns the full external URL' do
-        expect(environment.external_url_for(source_path, sha)).to eq('http://example.com/file.html')
-      end
-
-      it 'returns the proper url when the environment external_url ends in a slash' do
-        environment.external_url = 'http://example.com/sub/'
-
-        expect(environment.external_url_for(source_path, sha)).to eq('http://example.com/sub/file.html')
+      using RSpec::Parameterized::TableSyntax
+
+      where(:external_url, :public_path, :full_url) do
+        'http://example.com'          | 'file.html'         | 'http://example.com/file.html'
+        'http://example.com/'         | 'file.html'         | 'http://example.com/file.html'
+        'http://example.com'          | '/file.html'        | 'http://example.com/file.html'
+        'http://example.com/'         | '/file.html'        | 'http://example.com/file.html'
+        'http://example.com/subpath'  | 'public/file.html'  | 'http://example.com/subpath/public/file.html'
+        'http://example.com/subpath/' | 'public/file.html'  | 'http://example.com/subpath/public/file.html'
+        'http://example.com/subpath'  | '/public/file.html' | 'http://example.com/subpath/public/file.html'
+        'http://example.com/subpath/' | '/public/file.html' | 'http://example.com/subpath/public/file.html'
+      end
+      with_them do
+        it 'returns the full external URL' do
+          environment.external_url = external_url
+          allow(project).to receive(:public_path_for_source_path).with(source_path, sha).and_return(public_path)
+
+          expect(environment.external_url_for(source_path, sha)).to eq(full_url)
+        end
       end
     end
   end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 32fb0c0ff73..157103123ad 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2950,6 +2950,16 @@ describe Project do
           expect(project.public_path_for_source_path('file.html', sha)).to be_nil
         end
       end
+
+      it 'returns a public path with a leading slash unmodified' do
+        route_map = Gitlab::RouteMap.new(<<-MAP.strip_heredoc)
+          - source: 'source/file.html'
+            public: '/public/file'
+        MAP
+        allow(project).to receive(:route_map_for).with(sha).and_return(route_map)
+
+        expect(project.public_path_for_source_path('source/file.html', sha)).to eq('/public/file')
+      end
     end
 
     context 'when there is no route map' do