AR delete surface/Step 1: Client version and file delete writes

Fiona McCawleyrequested to merge
627391-client-version-and-file-deletes into master

What does this MR do and why?

This is Step 1 of 19 in the plan for the monolith artifact delete surface, tracked under #627172.

This MR is opened as a Draft. The plan merge request it implements, gitlab-org/ops/artifact-registry!2201 (merged), has not merged yet. Merging the plan is the approval signal for this step, so this MR stays in Draft until then.

The change adds two write methods to the Artifact Registry Ruby client at ee/lib/artifact_registry/client.rb:

  • #delete_version(slug:, repository_name:, format:, version_id:)
  • #delete_file(slug:, repository_name:, format:, file_id:)

Matching specs are added to ee/spec/lib/artifact_registry/client_spec.rb: 44 new examples, driven by one shared example group named "a single package artifact delete" so both methods are held to the same posture.

A few implementation notes:

  • No new path builder and no new private helper were added. Both routes address the top level of the format segment (.../{format}/versions/{version_id} and .../{format}/files/{file_id}), so they reach the existing private artifact_path builder as it stands.
  • Both methods guard against the PACKAGE_FORMATS constant (maven, npm), not the ARTIFACT_FORMATS union (maven, npm, docker, oci) used by the already-merged #delete_artifact. AR's committed OpenAPI contract types the format path parameter on these two routes as a package format only, so a container repository has no reachable versions or files under any format segment.
  • The id is guarded up front with the existing private guard_segments!, rather than resolved to a not-found the way the single reads (#version) do. Without that guard, an id-less delete would silently address the collection route instead of a member.
  • Every delete answers 202 with an empty body. Acceptance is not completion, so neither method returns a count or a job handle. Both return nil, and nothing is parsed from the response. A caller that needs to know whether the artifact is gone has to re-read. Any other 2xx status is treated as contract drift and refused through the existing private raise_unexpected_success, not read as an acceptance.
  • Neither method retries. The client's connection-wide RETRY_OPTIONS[:methods] already excludes :delete (narrowed in the S05 step, !252683 (merged)), and both new methods inherit that with no change here. A replayed DELETE is dangerous because AR answers 404 on a version or file the first attempt already removed, which would report a failure for a delete that actually worked. A spec asserts the DELETE is issued exactly once on a transport timeout.
  • Neither method takes a kind argument, and neither checks kind. AR reads kind off its own repository row and decides whether the request permanently deletes a hosted artifact or evicts a remote cached copy. Both arms answer 202, so the response does not distinguish them. AR is the authorization boundary here, not the monolith.

Two choices a reviewer might flag:

  • The two method bodies are near-identical, and no shared private helper was extracted. The plan states that Steps 1 through 5 all touch these same two files and must be able to merge in any order. A helper introduced here would make Steps 2 and 3 depend on this step's merge order. The two methods instead mirror the shape of the already-merged #delete_artifact sibling in the same file.
  • The already-merged #delete_artifact and #bulk_delete_artifacts keep their existing inline form. This step does not refactor them.

Test coverage adds, across both methods: a positive 202 per package format; an empty 202 body treated as success with nothing parsed; a 404 raising ApiError with the status rather than resolving to nil (there is no idempotence rescue here, unlike #delete_repository); 401 and 403 raising AuthorizationError rather than ApiError, with no error tracking; 500 and 503 raising UnavailableError with an echoed credential redacted in both the raised and the logged error; a 200 refused as contract drift; docker, oci, and an unserved format rejected before any request is made, asserted by request count; blank and bare dot-segment slug, repository name, and id rejected before any request; percent-encoding of every path segment so a value cannot smuggle extra path segments; the credential attached as a Bearer header and never in the request URI; and exactly one DELETE issued on a transport timeout.

This ships dark behind the artifact_registry_ui feature flag, which is disabled. There is no changelog entry, because the change sits entirely behind a disabled flag. The commit carries an EE: true trailer.

References

Screenshots or screen recordings

Not applicable. This is a client-layer change only, with no UI.

How to set up and validate locally

  1. Run the specs.

    bundle exec rspec ee/spec/lib/artifact_registry/client_spec.rb

    Confirmed result: 720 examples, 0 failures (44 of them new).

  2. Run a script against a local stub HTTP server. Save the script below as /tmp/ar_delete_check.rb and run it with bundle exec rails runner /tmp/ar_delete_check.rb.

    require 'socket'

seen = []
server = TCPServer.new('127.0.0.1', 0)
port = server.addr[1]

Thread.new do
  loop do
    socket = server.accept
    request_line = socket.gets
    headers = {}
    while (line = socket.gets) && line != "\r\n"
      key, value = line.split(': ', 2)
      headers[key.downcase] = value.to_s.strip
    end
    seen << "#{request_line.strip} | authorization=#{headers['authorization']} | content-length=#{headers['content-length'].inspect}"
    socket.print("HTTP/1.1 202 Accepted\r\nContent-Type: application/json\r\nContent-Length: 0\r\n\r\n")
    socket.close
  end
rescue IOError, Errno::EBADF
  nil
end

exchange = Object.new
exchange.define_singleton_method(:token_for) { |_user| 'stub-token' }

client = ArtifactRegistry::Client.new(
  base_url: "http://127.0.0.1:#{port}",
  current_user: Object.new,
  token_exchange: exchange
)

puts "delete_version(maven)      => #{client.delete_version(slug: 'my-group', repository_name: 'my-repo', format: 'maven', version_id: '11111111-2222-3333-4444-555555555555').inspect}"
puts "delete_file(npm, id 'a/b') => #{client.delete_file(slug: 'my-group', repository_name: 'my-repo', format: 'npm', file_id: 'a/b').inspect}"

[[:delete_version, :version_id, 'docker'], [:delete_file, :file_id, 'oci']].each do |method, arg, format|
  client.public_send(method, slug: 'my-group', repository_name: 'my-repo', format: format, arg => 'x')
rescue ArgumentError => e
  puts "#{method}(#{format}) => ArgumentError: #{e.message}"
end

[[:delete_version, :version_id], [:delete_file, :file_id]].each do |method, arg|
  client.public_send(method, slug: 'my-group', repository_name: 'my-repo', format: 'maven', arg => '')
rescue ArgumentError => e
  puts "#{method}(blank id) => ArgumentError: #{e.message}"
end

sleep 0.2
puts "\nrequests the stub server saw:"
seen.each { |line| puts "  #{line}" }
server.close

    Confirmed output:

    delete_version(maven)      => nil
delete_file(npm, id 'a/b') => nil
delete_version(docker) => ArgumentError: format must be one of: maven, npm
delete_file(oci) => ArgumentError: format must be one of: maven, npm
delete_version(blank id) => ArgumentError: version_id is required
delete_file(blank id) => ArgumentError: file_id is required

requests the stub server saw:
  DELETE /api/v1/my-group/repositories/my-repo/maven/versions/11111111-2222-3333-4444-555555555555 HTTP/1.1 | authorization=Bearer stub-token | content-length=nil
  DELETE /api/v1/my-group/repositories/my-repo/npm/files/a%2Fb HTTP/1.1 | authorization=Bearer stub-token | content-length=nil

    This confirms the two paths match AR's contracted routes. The a/b id is percent-encoded, so it cannot smuggle an extra path segment. The credential rides in the Authorization header, not the URI. No request body is sent, and the empty 202 resolves to nil rather than a parse error. A stub server is the right level for a client-layer change here, because AR's handler arms for these two routes are already released, so this is not a placeholder for unimplemented behavior on the AR side.

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.

Merge request reports