Add the repository packages connection on a Artifact registry detail type
What does this MR do and why?
Adds the packages keyset connection for Maven and npm repositories, over the
merged client packages read, plus the organization seam the connection needs
to reach that client.
This change is behind the feature flag :artifact_registry_ui (dark,
default_enabled: false). It is monolith/S05 Step 3, and the first AR slice
to mount a child connection that issues its own Artifact Registry request.
Important
This MR deviates from the merged monolith/S05 spec and needs a spec
author's decision before it merges. The spec places both artifact
connections on the shared ArtifactRegistryRepository type. This MR mounts
packages on a new ArtifactRegistryRepositoryDetails type instead. The
reasoning is in Why the connection is on a detail type
below. If that reasoning is rejected, the last commit
(61617eb4bab3) can be dropped on its own and the rest of the MR stands.
The elements are a union, not one type with nullable fields
Artifact Registry answers Maven and npm packages with structurally different
rows (ADR-009).
A single element type would make every Maven field nullable-but-set and every
npm field nullable-but-set in alternation, erasing that split. So this adds
ArtifactRegistryPackage, a BaseUnion over ArtifactRegistryMavenPackage
(id, groupId, artifactId) and ArtifactRegistryNpmPackage (id, name,
nullable scope, versionsCount), mirroring the FOSS PackageMetadata union.
resolve_type fails closed: a package class with no member type raises
rather than resolving to whichever branch happens to be last. That is asserted.
The organization seam
ArtifactRegistry::RepositoryPresenter carries the organization a repository
was read through. The value object holds no back-reference, so a connection
mounted on the repository type has no other way to reach the slug and the
memoized client its own read needs.
Rejected: context.scoped_set!, which works but hides the data flow in mutable
context where the wrapper keeps it on the object graph the type system already
documents.
Why the connection is on a detail type
packages issues one Artifact Registry request per repository, and nothing
batches it. With the connection on the shared type, this was a valid query:
organization {
artifactRegistryRepositories(first: 100) { nodes { packages { nodes { id } } } }
}That is 1 + 100 sequential AR round trips from a single GraphQL request.
Nothing in the codebase prevents it: GitlabSchema sets
default_max_page_size 100, neither the field nor the resolvers declare
max_page_size or complexity, and static complexity cannot see this cost
because it scores the query shape rather than the row count. A 100-row fan-out
scores like a cheap query.
graphql-ruby emits a subclass as an unrelated object type with no shared
interface, so a client holding an ArtifactRegistryRepository from the list
cannot select packages on it even through an inline fragment. The query above
now fails validation. That is the property a numeric guard cannot provide,
and it is asserted directly in
ee/spec/graphql/types/artifact_registry/repository_details_type_spec.rb.
The spec already accepts this reasoning one level down: it caps artifact pages at 20 rows because "an uncapped page would multiply AR requests per query", then mounts the connection on a list that defaults to 100 wide. Once a child connection lands on the package elements, the two shapes compose and that cap does not survive the level above it.
The concrete cost: fragment-based selections break
Worth naming explicitly in the spec decision, because it is a stronger break
than merely losing access to packages. organization.artifactRegistryRepository
moves from ArtifactRegistryRepository to ArtifactRegistryRepositoryDetails,
and since graphql-ruby emits the subclass as an unrelated type with no shared
interface, a query that spreads a fragment declared on
ArtifactRegistryRepository under that field now fails validation.
That is acceptable here only because the field is experiment behind a disabled
flag, so it has no clients to break. It is the cost the spec author is being
asked to accept, not a detail of the implementation.
Precedent for the pattern in this codebase: VirtualRegistries UpstreamType
/ UpstreamDetailsType (same group, current milestones, expensive child
connection on the detail type only), plus ContainerRepositoryDetailsType,
Packages::PackageDetailsType, and
GoogleCloud::ArtifactRegistry::DockerImageDetailsType.
The split closes the fan-out across repositories. Two bounds close what it does not, both added in review:
max_page_size: 20on the field.ArtifactRegistry::PaginatesListsreads the cap off the field and falls back to the schema default of 100 without it, sofirst: 100was forwardinglimit: 100to Artifact Registry.FieldCallCount, limit: 1on the resolver. One operation could still select the connection under several aliases on the same repository, each resolution its own AR request. The extension raises before the resolve body runs, so the second selection costs no client call.
Complexity covers neither: it scores the query shape, not the row count or the per-resolution HTTP cost.
Two things fell out of the split rather than being engineered:
RepositoriesResolverno longer wraps its nodes in a presenter. That wrap existed only so a child connection on a listed repository could reach the organization, and the list mounts no child connection now.Repositories::CreateandUpdatereturn the plain type, which no longer carriespackages, so selecting it through a mutation payload can no longer raiseNoMethodError.
Alternative considered and rejected:
Gitlab::Graphql::Limit::FieldCallCount with limit: 1, which is one line and
needs no spec change. It yields a runtime error rather than a validation error,
leaves packages documented on every list node before rejecting it at runtime,
and fixes neither the presenter wrap nor the mutation payload.
The local typedefs stopped redeclaring the schema
The second commit fixes generate-apollo-graphql-schema, which the first
commit broke. The frontend's local @client typedefs declared the package
types, the union, their connection, and ArtifactRegistryRepository.packages
while no server backing existed. Adding those to the real schema made each one
a duplicate definition, and Apollo refuses to build such a schema.
The typedefs now reference what the schema owns instead of defining it: the two
package types became extend type blocks carrying only versions, which is
still local. What stays local is otherwise unchanged.
The shared possible types were regenerated
The fourth commit fixes graphql-verify, which broke at the
graphql_possible_types_extraction.js --check step. That step asserts
app/assets/javascripts/graphql_shared/possible_types.json matches the union
and interface members the schema declares, and adding ArtifactRegistryPackage
left the committed file stale. Apollo needs those members to match an inline
fragment, so a stale file renders the Maven and npm cells blank rather than
erroring.
Notes for review
Three behaviors are worth a reviewer's eye. Each is a monolith convention the plan's design collided with, and each was established empirically:
- The resolver short-circuits a non-package format instead of calling the
client.
ArtifactRegistry::Client#packagesguards onPACKAGE_FORMATSand raisesArgumentErrorbefore issuing a request, so a container repository could never earn the404the spec says resolves this connection null. The short-circuit produces that same null, with no client call and no error. - The presenter authorizes through
declarative_policy_subject, notdeclarative_policy_delegate. The delegate form picks the policy class from the delegate but instantiates it with the original subject, soOrganizationPolicyruns against the presenter and fails on@subject.user?. Resolvers::BaseResolver#objectunwraps a presenter to its subject, which would drop the organization beforePackagesResolvercould read it. The resolver reads the pre-unwrap object via a privatepresented_repositoryand leavesobjectmeaning what it means everywhere else.
Two further notes:
- The package element types carry no
authorizeand noskip_type_authorization. They are policy-less value objects, and declaring the ability makes the union redactor raise.RepositoryDetailsTypeinheritsauthorize :read_artifact_registryfrom its parent and carries aGraphql/AuthorizeTypesdisable, because that cop reads only the class body. field :idneedsresolver_method:, notmethod:.resolver_methoddefaults to the field name and is checked on the type instance first, somethod:would still route the read throughBaseObject#idand encode a global ID these value objects cannot produce. That reasoning now lives once, inArtifactRegistry::ExposesElementId, which both package types include.
Known follow-ups, deliberately not in this MR
imageswill hit the same Apollo collision.ArtifactRegistryImageandArtifactRegistryImageConnectionare still local typedef definitions, so Step 4 needs the sameextend typeedit. It should also mountimageson the detail type.- The
packagesdocument still carries@client. The schema answers the connection now, butget_repository_packages.query.graphqlkeeps the directive andmock_resolvers.jskeeps answering it. Removing both makes the page render server data, which needs handler data inrepository_detail_spec.jsand the feature spec that goes with it, so it is its own change.mock_resolvers_spec.jscarries a named exception for it (LOCALLY_ANSWERED_SCHEMA_FIELDS) so the local-versus-schema invariant still fails loudly everywhere else. cache_config.jsstill exports a localpossibleTypesfor the union. It is now redundant: bothlib/graphql.jsandmock_apollo_helper.jsread the generatedpossible_types.json, which carries the union as of this MR. It is left in place to keep this diff to the CI fix; the note inrepositories/index.jsabout dropping it with the mock can go one step earlier now.
Multiversion compatibility
Danger flags this MR because it touches GraphQL backend and frontend code
together. The rolling-deployment hazard it describes does not arise here,
though the reasoning changed when monolith/S14 landed on master during
review and this branch was rebased onto it. What follows is re-derived from the
current diff.
The hazard is a frontend that requests a new field from a backend which has not finished rolling out. This MR adds no selection to any document:
- No query or mutation document changed.
git diff --name-only origin/master...HEAD -- '*.graphql'returnsgraphql/typedefs.graphqlalone, and that change only removes local declarations the schema now owns and retargets the rest ontoArtifactRegistryRepositoryDetails. get_repository_detail.query.graphqldoes send server fields, as of S14 — but every one of them already exists onmaster. Renaming the type the field returns does not add a selection, and an older backend answering that field answers the same fields it answers today.- The remaining frontend changes are cache and local-resolver wiring
(
cache_config.js,cache_update.js,mock_resolvers.js,constants.js,repositories_edit_form.vue) plus a generatedpossible_types.json. None of them reaches the server.
What is a genuine ordering constraint is the type rename itself, and it is
satisfied by construction: ArtifactRegistryRepositoryDetails ships in this
same MR as the field's return type, so no frontend revision ever asks an older
backend for it.
On top of that, the whole surface sits behind the disabled
artifact_registry_ui flag, which is the "gate the frontend behind a feature
flag" option the warning offers.
@gl_introduced marks fields the frontend newly consumes from the server. This
MR introduces no such field, so it does not apply.
Diff size
1064 reviewable LOC, past the 500 the development model asks me to justify:
| group | LOC |
|---|---|
| production Ruby (9 files) | +216 / -3 |
| frontend typedefs (1 file) | +11 / -20 |
| specs (10 files) | +837 / -5 |
| generated artifacts, schema + possible types (excluded from review) | +539 / -2 |
Danger's line count (1633) includes the three generated files.
On Danger's commit-body warnings: rewrapping those bodies to 72 columns
means rewriting four already-pushed commits and force-pushing, which would
re-anchor the review threads already sitting on this branch. Squash is enabled
on this MR instead, so the individual bodies never reach master.
Production is 216 LOC across nine files. The bulk is the spec suite, which does not split usefully: separating the connection from its request spec would land untested code, and the union, the resolver, and the presenter are one seam a reviewer reads together. The three commits are separable, and the detail-type commit can be dropped alone if its reasoning is rejected.
How to set up and validate locally
-
Enable the flag:
Feature.enable(:artifact_registry_ui)inrails c. -
Query an organization's repository and select the connection:
query { organization(id: "gid://gitlab/Organizations::Organization/1") { artifactRegistryRepository(name: "my-maven-repo") { name packages(first: 5) { pageInfo { hasNextPage endCursor } nodes { __typename ... on ArtifactRegistryMavenPackage { id groupId artifactId } ... on ArtifactRegistryNpmPackage { id name scope versionsCount } } } } } } -
Confirm a
dockerorocirepository resolvespackagesnull without an error, and that disabling the flag resolves it null with no AR call. -
Confirm the guard: selecting
packagesinsideartifactRegistryRepositoriesis rejected as an invalid query.
Verification performed
rspecover the Artifact Registry GraphQL surface (requests, resolvers, resolver concerns, types, presenters, mutations): 182 examples, 0 failures.jest ee/spec/frontend/packages_and_registries/artifact_registry: 40 suites, 980 tests, all passing.rake gitlab:graphql:validate: passes.rubocopclean on every touched file.- Schema and reference docs regenerated with
bundle exec rake gitlab:graphql:update_all; regenerating again after theExposesElementIdextraction produced a byte-identical result, confirming that commit is a refactor only. - The Apollo CLI lives only in a dedicated CI image, so the schema-collision
fix was verified mechanically instead: every typedef definition and every
extendfield was cross-checked againstintrospection_result.json, with zero collisions. - The
graphql-verifysteps were run locally and pass:gitlab:graphql:validate,gitlab:graphql:check_docs,gitlab:graphql:check_introspection_sync, andgraphql_possible_types_extraction.js --check.
MR acceptance checklist
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.