Conan download_urls properly checks channel

Fix bug where Conan download_urls was not filtering
packages by conan_channel sometimes resulting in the
download_urls for the incorrect channel to be returned.

app/presenters/packages/conan/package_presenter.rb

@@ -8,8 +8,8 @@ module Packages
 
       attr_reader :params
 
-      def initialize(recipe, user, project, params = {})
-        @recipe = recipe
+      def initialize(package, user, project, params = {})
+        @package = package
         @user = user
         @project = project
         @params = params
@@ -48,10 +48,10 @@ module Packages
       def build_recipe_file_url(package_file)
         expose_url(
           api_v4_packages_conan_v1_files_export_path(
-            package_name: package.name,
-            package_version: package.version,
-            package_username: package.conan_metadatum.package_username,
-            package_channel: package.conan_metadatum.package_channel,
+            package_name: @package.name,
+            package_version: @package.version,
+            package_username: @package.conan_metadatum.package_username,
+            package_channel: @package.conan_metadatum.package_channel,
             recipe_revision: package_file.conan_file_metadatum.recipe_revision,
             file_name: package_file.file_name
           )
@@ -61,10 +61,10 @@ module Packages
       def build_package_file_url(package_file)
         expose_url(
           api_v4_packages_conan_v1_files_package_path(
-            package_name: package.name,
-            package_version: package.version,
-            package_username: package.conan_metadatum.package_username,
-            package_channel: package.conan_metadatum.package_channel,
+            package_name: @package.name,
+            package_version: @package.version,
+            package_username: @package.conan_metadatum.package_username,
+            package_channel: @package.conan_metadatum.package_channel,
             recipe_revision: package_file.conan_file_metadatum.recipe_revision,
             conan_package_reference: package_file.conan_file_metadatum.conan_package_reference,
             package_revision: package_file.conan_file_metadatum.package_revision,
@@ -84,22 +84,9 @@ module Packages
       end
 
       def package_files
-        return unless package
+        return unless @package
 
-        @package_files ||= package.package_files.preload_conan_file_metadata
-      end
-
-      def package
-        strong_memoize(:package) do
-          name, version = @recipe.split('@')[0].split('/')
-
-          @project.packages
-                  .conan
-                  .with_name(name)
-                  .with_version(version)
-                  .order_created
-                  .last
-        end
+        @package_files ||= @package.package_files.preload_conan_file_metadata
       end
 
       def matching_reference?(package_file)

changelogs/unreleased/235653-conan-dependency-bug.yml

+---
+title: Fix bug where conan does not properly check package channel when returning
+  file download urls
+merge_request: 40029
+author:
+type: fixed

lib/api/conan_packages.rb

@@ -113,7 +113,7 @@ module API
           authorize!(:read_package, project)
 
           presenter = ::Packages::Conan::PackagePresenter.new(
-            recipe,
+            package,
             current_user,
             project,
             conan_package_reference: params[:conan_package_reference]
@@ -131,7 +131,7 @@ module API
         get do
           authorize!(:read_package, project)
 
-          presenter = ::Packages::Conan::PackagePresenter.new(recipe, current_user, project)
+          presenter = ::Packages::Conan::PackagePresenter.new(package, current_user, project)
 
           present presenter, with: ::API::Entities::ConanPackage::ConanRecipeSnapshot
         end

lib/api/helpers/packages/conan/api_helpers.rb

@@ -9,7 +9,7 @@ module API
             authorize!(:read_package, project)
 
             presenter = ::Packages::Conan::PackagePresenter.new(
-              recipe,
+              package,
               current_user,
               project,
               conan_package_reference: params[:conan_package_reference]
@@ -94,6 +94,7 @@ module API
           def package
             strong_memoize(:package) do
               project.packages
+                .conan
                 .with_name(params[:package_name])
                 .with_version(params[:package_version])
                 .with_conan_channel(params[:package_channel])

spec/presenters/packages/conan/package_presenter_spec.rb

@@ -4,34 +4,20 @@ require 'spec_helper'
 
 RSpec.describe ::Packages::Conan::PackagePresenter do
   let_it_be(:user) { create(:user) }
-  let_it_be(:project) { create(:project) }
+  let_it_be(:package) { create(:conan_package) }
+  let_it_be(:project) { package.project }
   let_it_be(:conan_package_reference) { '123456789'}
 
-  RSpec.shared_examples 'not selecting a package with the wrong type' do
-    context 'with a nuget package with same name and version' do
-      let_it_be(:wrong_package) { create(:nuget_package, name: 'wrong', version: '1.0.0', project: project) }
-
-      let(:recipe) { "#{wrong_package.name}/#{wrong_package.version}" }
-
-      it { is_expected.to be_empty }
-    end
-  end
-
   describe '#recipe_urls' do
-    subject { described_class.new(recipe, user, project).recipe_urls }
+    subject { described_class.new(package, user, project).recipe_urls }
 
     context 'no existing package' do
-      let(:recipe) { "my-pkg/v1.0.0/#{project.full_path}/stable" }
+      let(:package) { nil }
 
       it { is_expected.to be_empty }
     end
 
-    it_behaves_like 'not selecting a package with the wrong type'
-
     context 'existing package' do
-      let(:package) { create(:conan_package, project: project) }
-      let(:recipe) { package.conan_recipe }
-
       let(:expected_result) do
         {
           "conanfile.py" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/export/conanfile.py",
@@ -40,24 +26,26 @@ RSpec.describe ::Packages::Conan::PackagePresenter do
       end
 
       it { is_expected.to eq(expected_result) }
+
+      context 'when there are multiple channels for the same package' do
+        let(:conan_metadatum) { create(:conan_metadatum, package_channel: 'newest' ) }
+        let!(:newest_package) { create(:conan_package, name: package.name, version: package.version, project: project, conan_metadatum: conan_metadatum) }
+
+        it { is_expected.to eq(expected_result) }
+      end
     end
   end
 
   describe '#recipe_snapshot' do
-    subject { described_class.new(recipe, user, project).recipe_snapshot }
+    subject { described_class.new(package, user, project).recipe_snapshot }
 
     context 'no existing package' do
-      let(:recipe) { "my-pkg/v1.0.0/#{project.full_path}/stable" }
+      let(:package) { nil }
 
       it { is_expected.to be_empty }
     end
 
-    it_behaves_like 'not selecting a package with the wrong type'
-
     context 'existing package' do
-      let(:package) { create(:conan_package, project: project) }
-      let(:recipe) { package.conan_recipe }
-
       let(:expected_result) do
         {
           "conanfile.py" => '12345abcde',
@@ -74,22 +62,17 @@ RSpec.describe ::Packages::Conan::PackagePresenter do
 
     subject do
       described_class.new(
-        recipe, user, project, conan_package_reference: reference
+        package, user, project, conan_package_reference: reference
       ).package_urls
     end
 
     context 'no existing package' do
-      let(:recipe) { "my-pkg/v1.0.0/#{project.full_path}/stable" }
+      let(:package) { nil }
 
       it { is_expected.to be_empty }
     end
 
-    it_behaves_like 'not selecting a package with the wrong type'
-
     context 'existing package' do
-      let(:package) { create(:conan_package, project: project) }
-      let(:recipe) { package.conan_recipe }
-
       let(:expected_result) do
         {
           "conaninfo.txt" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/package/#{conan_package_reference}/0/conaninfo.txt",
@@ -131,7 +114,7 @@ RSpec.describe ::Packages::Conan::PackagePresenter do
 
         it 'returns empty if the reference does not exist' do
           result = described_class.new(
-            recipe, user, project, conan_package_reference: 'doesnotexist'
+            package, user, project, conan_package_reference: 'doesnotexist'
           ).package_urls
 
           expect(result).to eq({})
@@ -145,22 +128,17 @@ RSpec.describe ::Packages::Conan::PackagePresenter do
 
     subject do
       described_class.new(
-        recipe, user, project, conan_package_reference: reference
+        package, user, project, conan_package_reference: reference
       ).package_snapshot
     end
 
     context 'no existing package' do
-      let(:recipe) { "my-pkg/v1.0.0/#{project.full_path}/stable" }
+      let(:package) { nil }
 
       it { is_expected.to be_empty }
     end
 
-    it_behaves_like 'not selecting a package with the wrong type'
-
     context 'existing package' do
-      let(:package) { create(:conan_package, project: project) }
-      let(:recipe) { package.conan_recipe }
-
       let(:expected_result) do
         {
           "conaninfo.txt" => '12345abcde',

spec/requests/api/conan_packages_spec.rb

@@ -268,7 +268,7 @@ RSpec.describe API::ConanPackages do
       it 'returns not found' do
         allow(::Packages::Conan::PackagePresenter).to receive(:new)
           .with(
-            'aa/bb@%{project}/ccc' % { project: ::Packages::Conan::Metadatum.package_username_from(full_path: project.full_path) },
+            nil,
             user,
             project,
             any_args
@@ -284,6 +284,21 @@ RSpec.describe API::ConanPackages do
     end
   end
 
+  shared_examples 'not selecting a package with the wrong type' do
+    context 'with a nuget package with same name and version' do
+      let(:conan_username) { ::Packages::Conan::Metadatum.package_username_from(full_path: project.full_path) }
+      let(:wrong_package) { create(:nuget_package, name: "wrong", version: '1.0.0', project: project) }
+      let(:recipe_path) { "#{wrong_package.name}/#{wrong_package.version}/#{conan_username}/foo" }
+
+      it 'calls the presenter with a nil package' do
+        expect(::Packages::Conan::PackagePresenter).to receive(:new)
+          .with(nil, user, project, any_args)
+
+        subject
+      end
+    end
+  end
+
   shared_examples 'recipe download_urls' do
     let(:recipe_path) { package.conan_recipe_path }
 
@@ -299,6 +314,8 @@ RSpec.describe API::ConanPackages do
 
       expect(json_response).to eq(expected_response)
     end
+
+    it_behaves_like 'not selecting a package with the wrong type'
   end
 
   shared_examples 'package download_urls' do
@@ -317,6 +334,8 @@ RSpec.describe API::ConanPackages do
 
       expect(json_response).to eq(expected_response)
     end
+
+    it_behaves_like 'not selecting a package with the wrong type'
   end
 
   context 'recipe endpoints' do
@@ -327,7 +346,7 @@ RSpec.describe API::ConanPackages do
 
     before do
       allow(::Packages::Conan::PackagePresenter).to receive(:new)
-        .with(package.conan_recipe, user, package.project, any_args)
+        .with(package, user, package.project, any_args)
         .and_return(presenter)
     end