Packages to use composition, not inheritance

GraphQL does not have inheritance, so it is an anti-pattern to use
it to model polymorphism in our models.

The correct approach is to model polymorphism as implementation of
interfaces. Here there is a single `PackageType` interface, and
then different implementations of that.

Since the package types only differ in their metadata, we move the
polymorphism down to the metadata fields, which an empty interface,
implemented by metadata types.

One thing that had to change was the `Query.package` field,
which was previously `Query.package_composer_details`. This must change
since the implementation of the lookup does not perform any type
checking, and thus we cannot type the return value as a composer
package. This would be an illegal and ill-typed down-cast. A good
analogy for this is having a Java collection of

```java
// yes, Cucumbers are fruits - look it up
List<Fruit> bowl = List.of(new Apple(), new Banana(), new Cucumber())
```

And then expecting to get an apple without a cast:

```java
Apple fruit = bowl.get(0) // bad
```

This code would fail to compile in Java, and it is equally illegal
in GraphQL, without the appropriate casting.

We mark the composer metadata type as an orphan since it is only refered
to as an implementation of the broader metadata type.

We also split the type of packages into a top-level one, which is able
to refer to versions, and a leaf node which may not. This prevents
unbounded mutual cyclic recursion.

Return successfully for all packages

We can gradually add more specific package types, but it is important
to always succeed with the data we can return.

app/graphql/resolvers/package_details_resolver.rb

 # frozen_string_literal: true
 
 module Resolvers
-  # No return types defined because they can be different.
-  # rubocop: disable Graphql/ResolverType
   class PackageDetailsResolver < BaseResolver
+    type ::Types::Packages::PackageType, null: true
+
     argument :id, ::Types::GlobalIDType[::Packages::Package],
       required: true,
       description: 'The global ID of the package.'

app/graphql/resolvers/packages_resolver.rb

@@ -2,7 +2,7 @@
 
 module Resolvers
   class PackagesResolver < BaseResolver
-    type Types::Packages::PackageType, null: true
+    type Types::Packages::PackageType.connection_type, null: true
 
     def resolve(**args)
       return unless packages_available?

app/graphql/types/packages/composer/details_type.rb

-# frozen_string_literal: true
-
-module Types
-  module Packages
-    module Composer
-      class DetailsType < Types::Packages::PackageType
-        graphql_name 'PackageComposerDetails'
-        description 'Details of a Composer package'
-
-        authorize :read_package
-
-        field :composer_metadatum, Types::Packages::Composer::MetadatumType, null: false, description: 'The Composer metadatum.'
-      end
-    end
-  end
-end

app/graphql/types/packages/composer/metadatum_type.rb

@@ -4,8 +4,8 @@ module Types
   module Packages
     module Composer
       class MetadatumType < BaseObject
-        graphql_name 'PackageComposerMetadatumType'
-        description 'Composer metadatum'
+        graphql_name 'ComposerMetadata'
+        description 'Composer metadata'
 
         authorize :read_package
 

app/graphql/types/packages/metadata_type.rb

+# frozen_string_literal: true
+
+module Types
+  module Packages
+    class MetadataType < BaseUnion
+      graphql_name 'PackageMetadata'
+      description 'Represents metadata associated with a Package'
+
+      possible_types ::Types::Packages::Composer::MetadatumType
+
+      def self.resolve_type(object, context)
+        case object
+        when ::Packages::Composer::Metadatum
+          ::Types::Packages::Composer::MetadatumType
+        else
+          # NOTE: This method must be kept in sync with `PackageWithoutVersionsType#metadata`,
+          # which must never produce data that this discriminator cannot handle.
+          raise 'Unsupported metadata type'
+        end
+      end
+    end
+  end
+end

app/graphql/types/packages/package_type.rb

@@ -2,26 +2,13 @@
 
 module Types
   module Packages
-    class PackageType < BaseObject
+    class PackageType < PackageWithoutVersionsType
       graphql_name 'Package'
       description 'Represents a package in the Package Registry'
-
       authorize :read_package
 
-      field :id, GraphQL::ID_TYPE, null: false, description: 'The ID of the package.'
-      field :name, GraphQL::STRING_TYPE, null: false, description: 'The name of the package.'
-      field :created_at, Types::TimeType, null: false, description: 'The created date.'
-      field :updated_at, Types::TimeType, null: false, description: 'The updated date.'
-      field :version, GraphQL::STRING_TYPE, null: true, description: 'The version of the package.'
-      field :package_type, Types::Packages::PackageTypeEnum, null: false, description: 'The type of the package.'
-      field :tags, Types::Packages::PackageTagType.connection_type, null: true, description: 'The package tags.'
-      field :project, Types::ProjectType, null: false, description: 'Project where the package is stored.'
-      field :pipelines, Types::Ci::PipelineType.connection_type, null: true, description: 'Pipelines that built the package.'
-      field :versions, Types::Packages::PackageType.connection_type, null: true, description: 'The other versions of the package.'
-
-      def project
-        Gitlab::Graphql::Loaders::BatchModelLoader.new(Project, object.project_id).find
-      end
+      field :versions, ::Types::Packages::PackageWithoutVersionsType.connection_type, null: true,
+        description: 'The other versions of the package.'
     end
   end
 end

app/graphql/types/packages/package_without_versions_type.rb

+# frozen_string_literal: true
+
+module Types
+  module Packages
+    class PackageWithoutVersionsType < ::Types::BaseObject
+      graphql_name 'PackageWithoutVersions'
+      description 'Represents a version of a package in the Package Registry'
+
+      authorize :read_package
+
+      field :id, ::Types::GlobalIDType[::Packages::Package], null: false,
+            description: 'ID of the package.'
+
+      field :name, GraphQL::STRING_TYPE, null: false, description: 'Name of the package.'
+      field :created_at, Types::TimeType, null: false, description: 'Date of creation.'
+      field :updated_at, Types::TimeType, null: false, description: 'Date of most recent update.'
+      field :version, GraphQL::STRING_TYPE, null: true, description: 'Version string.'
+      field :package_type, Types::Packages::PackageTypeEnum, null: false, description: 'Package type.'
+      field :tags, Types::Packages::PackageTagType.connection_type, null: true, description: 'Package tags.'
+      field :project, Types::ProjectType, null: false, description: 'Project where the package is stored.'
+      field :pipelines, Types::Ci::PipelineType.connection_type, null: true,
+        description: 'Pipelines that built the package.'
+      field :metadata, Types::Packages::MetadataType, null: true,
+        description: 'Package metadata.'
+
+      def project
+        Gitlab::Graphql::Loaders::BatchModelLoader.new(Project, object.project_id).find
+      end
+
+      # NOTE: This method must be kept in sync with `MetadataType.resolve_type`.
+      # This method must never produce data that the discriminator cannot handle.
+      def metadata
+        case object.package_type
+        when 'composer'
+          object.composer_metadatum
+        else
+          nil
+        end
+      end
+    end
+  end
+end

app/graphql/types/project_type.rb

@@ -179,7 +179,7 @@ module Types
           description: 'A single issue of the project',
           resolver: Resolvers::IssuesResolver.single
 
-    field :packages, Types::Packages::PackageType.connection_type, null: true,
+    field :packages,
          description: 'Packages of the project',
          resolver: Resolvers::PackagesResolver
 

app/graphql/types/query_type.rb

@@ -58,9 +58,8 @@ module Types
             argument :id, ::Types::GlobalIDType[::ContainerRepository], required: true, description: 'The global ID of the container repository'
           end
 
-    field :package_composer_details, Types::Packages::Composer::DetailsType,
-          null: true,
-          description: 'Find a composer package',
+    field :package,
+          description: 'Find a package',
           resolver: Resolvers::PackageDetailsResolver
 
     field :user, Types::UserType,

app/services/security/ci_configuration/sast_parser_service.rb

@@ -94,7 +94,6 @@ module Security
       def gitlab_ci_yml_attributes
         @gitlab_ci_yml_attributes ||= begin
           config_content = @project.repository.blob_data_at(@project.repository.root_ref_sha, ci_config_file)
-
           return {} unless config_content
 
           build_sast_attributes(config_content)

spec/fixtures/api/schemas/graphql/packages/package_composer_details.json

-{
-  "type": "object",
-  "allOf": [{ "$ref": "./package_details.json" }],
-  "properties": {
-    "target_sha": {
-      "type": "string"
-    },
-    "composer_json": {
-      "type": "object"
-    }
-  }
-}

spec/fixtures/api/schemas/graphql/packages/package_composer_metadata.json

+{
+  "type": "object",
+  "additionalProperties": false,
+  "required": ["targetSha", "composerJson"],
+  "properties": {
+    "targetSha": {
+      "type": "string"
+    },
+    "composerJson": {
+      "type": "object",
+      "additionalProperties": false,
+      "required": ["name", "type", "license", "version"],
+      "properties": {
+        "name": { "type": "string" },
+        "type": { "type": "string" },
+        "license": { "type": "string" },
+        "version": { "type": "string" }
+      }
+    }
+  }
+}

spec/fixtures/api/schemas/graphql/packages/package_details.json

 {
   "type": "object",
+  "additionalProperties": false,
+  "required": [
+    "id", "name", "createdAt", "updatedAt", "version", "packageType",
+    "project", "tags", "pipelines", "versions", "metadata"
+  ],
   "properties": {
     "id": {
       "type": "string"
@@ -16,21 +21,46 @@
     "version": {
       "type": ["string", "null"]
     },
-    "package_type": {
+    "packageType": {
       "type": ["string"],
       "enum": ["MAVEN", "NPM", "CONAN", "NUGET", "PYPI", "COMPOSER", "GENERIC", "GOLANG", "DEBIAN"]
     },
     "tags": {
-      "type": "object"
+      "type": "object",
+      "additionalProperties": false,
+      "properties": {
+        "pageInfo": { "type": "object" },
+        "edges": { "type": "array" },
+        "nodes": { "type": "array" }
+      }
     },
     "project": {
       "type": "object"
     },
     "pipelines": {
-      "type": "object"
+      "type": "object",
+      "additionalProperties": false,
+      "properties": {
+        "pageInfo": { "type": "object" },
+        "count": { "type": "integer" },
+        "edges": { "type": "array" },
+        "nodes": { "type": "array" }
+      }
     },
     "versions": {
-      "type": "object"
+      "type": "object",
+      "additionalProperties": false,
+      "properties": {
+        "pageInfo": { "type": "object" },
+        "edges": { "type": "array" },
+        "nodes": { "type": "array" }
+      }
+    },
+    "metadata": {
+      "anyOf": [
+        { "$ref": "./package_composer_metadata.json" },
+        { "type": "null" }
+      ]
     }
   }
 }

spec/graphql/resolvers/package_details_resolver_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Resolvers::PackageDetailsResolver do
   include GraphqlHelpers
+  include ::Gitlab::Graphql::Laziness
 
   let_it_be_with_reload(:project) { create(:project) }
   let_it_be(:user) { project.owner }
@@ -11,10 +12,10 @@ RSpec.describe Resolvers::PackageDetailsResolver do
 
   describe '#resolve' do
     let(:args) do
-      { id: package.to_global_id.to_s }
+      { id: global_id_of(package) }
     end
 
-    subject { resolve(described_class, ctx: { current_user: user }, args: args).sync }
+    subject { force(resolve(described_class, ctx: { current_user: user }, args: args)) }
 
     it { is_expected.to eq(package) }
   end

spec/graphql/types/packages/composer/metadatum_type_spec.rb

@@ -2,9 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe GitlabSchema.types['PackageComposerMetadatumType'] do
-  it { expect(described_class.graphql_name).to eq('PackageComposerMetadatumType') }
-
+RSpec.describe GitlabSchema.types['ComposerMetadata'] do
   it 'includes composer metadatum fields' do
     expected_fields = %w[
      target_sha composer_json

spec/graphql/types/packages/package_type_spec.rb

@@ -3,11 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe GitlabSchema.types['Package'] do
-  it { expect(described_class.graphql_name).to eq('Package') }
-
   it 'includes all the package fields' do
     expected_fields = %w[
-        id name version created_at updated_at package_type tags project pipelines versions
+      id name version package_type
+      created_at updated_at
+      project
+      tags pipelines versions
     ]
 
     expect(described_class).to include_graphql_fields(*expected_fields)

spec/graphql/types/packages/composer/details_type_spec.rb → spec/graphql/types/packages/package_without_versions_type_spec.rb

@@ -2,20 +2,10 @@
 
 require 'spec_helper'
 
-RSpec.describe GitlabSchema.types['PackageComposerDetails'] do
-  it { expect(described_class.graphql_name).to eq('PackageComposerDetails') }
-
+RSpec.describe GitlabSchema.types['PackageWithoutVersions'] do
   it 'includes all the package fields' do
     expected_fields = %w[
-      id name version created_at updated_at package_type tags project pipelines versions
-    ]
-
-    expect(described_class).to include_graphql_fields(*expected_fields)
-  end
-
-  it 'includes composer specific files' do
-    expected_fields = %w[
-      composer_metadatum
+      id name version created_at updated_at package_type tags project pipelines
     ]
 
     expect(described_class).to include_graphql_fields(*expected_fields)

spec/graphql/types/query_type_spec.rb

@@ -95,9 +95,9 @@ RSpec.describe GitlabSchema.types['Query'] do
     it { is_expected.to have_graphql_type(Types::ContainerRepositoryDetailsType) }
   end
 
-  describe 'package_composer_details field' do
-    subject { described_class.fields['packageComposerDetails'] }
+  describe 'package field' do
+    subject { described_class.fields['package'] }
 
-    it { is_expected.to have_graphql_type(Types::Packages::Composer::DetailsType) }
+    it { is_expected.to have_graphql_type(Types::Packages::PackageType) }
   end
 end

spec/requests/api/graphql/packages/package_composer_details_spec.rb → spec/requests/api/graphql/packages/package_spec.rb

 # frozen_string_literal: true
 require 'spec_helper'
 
-RSpec.describe 'package composer details' do
+RSpec.describe 'package details' do
   using RSpec::Parameterized::TableSyntax
   include GraphqlHelpers
 
   let_it_be(:project) { create(:project) }
   let_it_be(:package) { create(:composer_package, project: project) }
+  let_it_be(:composer_json) { { name: 'name', type: 'type', license: 'license', version: 1 } }
   let_it_be(:composer_metadatum) do
     # we are forced to manually create the metadatum, without using the factory to force the sha to be a string
     # and avoid an error where gitaly can't find the repository
-    create(:composer_metadatum, package: package, target_sha: 'foo_sha', composer_json: { name: 'name', type: 'type', license: 'license', version: 1 })
+    create(:composer_metadatum, package: package, target_sha: 'foo_sha', composer_json: composer_json)
   end
 
+  let(:depth) { 3 }
+  let(:excluded) { ['metadata'] }
+
   let(:query) do
-    graphql_query_for(
-      'packageComposerDetails',
-      { id: package_global_id },
-      all_graphql_fields_for('PackageComposerDetails', max_depth: 2)
-    )
+    graphql_query_for(:package, { id: package_global_id }, <<~FIELDS)
+    #{all_graphql_fields_for('Package', max_depth: depth, excluded: excluded)}
+    metadata {
+      #{query_graphql_fragment('ComposerMetadata')}
+    }
+    FIELDS
   end
 
   let(:user) { project.owner }
-  let(:package_global_id) { package.to_global_id.to_s }
-  let(:package_composer_details_response) { graphql_data.dig('packageComposerDetails') }
+  let(:package_global_id) { global_id_of(package) }
+  let(:package_details) { graphql_data_at(:package) }
 
   subject { post_graphql(query, current_user: user) }
 
@@ -33,7 +38,43 @@ RSpec.describe 'package composer details' do
     end
 
     it 'matches the JSON schema' do
-      expect(package_composer_details_response).to match_schema('graphql/packages/package_composer_details')
+      expect(package_details).to match_schema('graphql/packages/package_details')
+    end
+
+    it 'includes the fields of the correct package' do
+      expect(package_details).to include(
+        'id' => package_global_id,
+        'metadata' => {
+          'targetSha' => 'foo_sha',
+          'composerJson' => composer_json.transform_keys(&:to_s).transform_values(&:to_s)
+        }
+      )
+    end
+  end
+
+  context 'there are other versions of this package' do
+    let(:depth) { 3 }
+    let(:excluded) { %w[metadata project tags pipelines] } # to limit the query complexity
+
+    let_it_be(:siblings) { create_list(:composer_package, 2, project: project, name: package.name) }
+
+    it 'includes the sibling versions' do
+      subject
+
+      expect(graphql_data_at(:package, :versions, :nodes)).to match_array(
+        siblings.map { |p| a_hash_including('id' => global_id_of(p)) }
+      )
+    end
+
+    context 'going deeper' do
+      let(:depth) { 6 }
+
+      it 'does not create a cycle of versions' do
+        subject
+
+        expect(graphql_data_at(:package, :versions, :nodes, :version)).to be_present
+        expect(graphql_data_at(:package, :versions, :nodes, :versions)).not_to be_present
+      end
     end
   end
 end

spec/requests/api/graphql/project/packages_spec.rb

@@ -5,16 +5,27 @@ require 'spec_helper'
 RSpec.describe 'getting a package list for a project' do
   include GraphqlHelpers
 
-  let_it_be(:project) { create(:project) }
+  let_it_be(:project) { create(:project, :repository) }
   let_it_be(:current_user) { create(:user) }
+
   let_it_be(:package) { create(:package, project: project) }
-  let(:packages_data) { graphql_data['project']['packages']['edges'] }
+  let_it_be(:maven_package) { create(:maven_package, project: project) }
+  let_it_be(:debian_package) { create(:debian_package, project: project) }
+  let_it_be(:composer_package) { create(:composer_package, project: project) }
+  let_it_be(:composer_metadatum) do
+    create(:composer_metadatum, package: composer_package,
+           target_sha: 'afdeh',
+           composer_json: { name: 'x', type: 'y', license: 'z', version: 1 })
+  end
+
+  let(:package_names) { graphql_data_at(:project, :packages, :edges, :node, :name) }
 
   let(:fields) do
     <<~QUERY
     edges {
       node {
-        #{all_graphql_fields_for('packages'.classify)}
+        #{all_graphql_fields_for('packages'.classify, excluded: ['project'])}
+        metadata { #{query_graphql_fragment('ComposerMetadata')} }
       }
     }
     QUERY
@@ -37,7 +48,17 @@ RSpec.describe 'getting a package list for a project' do
     it_behaves_like 'a working graphql query'
 
     it 'returns packages successfully' do
-      expect(packages_data[0]['node']['name']).to eq package.name
+      expect(package_names).to contain_exactly(
+        package.name,
+        maven_package.name,
+        debian_package.name,
+        composer_package.name
+      )
+    end
+
+    it 'deals with metadata' do
+      target_shas = graphql_data_at(:project, :packages, :edges, :node, :metadata, :target_sha)
+      expect(target_shas).to contain_exactly(composer_metadatum.target_sha)
     end
   end
 
@@ -53,7 +74,7 @@ RSpec.describe 'getting a package list for a project' do
     end
   end
 
-  context 'when the user is not autenthicated' do
+  context 'when the user is not authenticated' do
     before do
       post_graphql(query)
     end

spec/support/graphql/field_selection.rb

@@ -47,7 +47,7 @@ module Graphql
     NO_SKIP = ->(_name, _field) { false }
 
     def self.select_fields(type, skip = NO_SKIP, parent_types = Set.new, max_depth = 3)
-      return new if max_depth <= 0
+      return new if max_depth <= 0 || !type.respond_to?(:fields) # could be a union!
 
       new(type.fields.flat_map do |name, field|
         next [] if skip[name, field]

spec/support/helpers/graphql_helpers.rb

@@ -237,6 +237,10 @@ module GraphqlHelpers
     query_graphql_path([[name, args], node_selection], fields)
   end
 
+  def query_graphql_fragment(name)
+    "... on #{name} { #{all_graphql_fields_for(name)} }"
+  end
+
   # e.g:
   #   query_graphql_path(%i[foo bar baz], all_graphql_fields_for('Baz'))
   #   => foo { bar { baz { x y z } } }