Store each externally stored field in its own payload and expose hasContent

Dennis Meisterrequested to merge
gitlab-community/gitlab-org/gitlab:616229-agent-plan-filename-column into master

What does this MR do and why?

Gitlab::ExternallyStoredField stores some model fields as a JSON document in object storage instead of Postgres. Two models use it: WorkItems::AgentPlan (content, readiness_score_feedback) and Ci::Catalog::BundledResources::Version (readme).

The goal was to answer "does this work item have a plan?" without reading object storage. The first attempt added one file column per model, holding the document's filename, so its presence meant a payload exists.

That broke in review, when readiness_score_feedback moved into the content payload. One column for a shared document says something is stored, not what, so a plan with only feedback reported content.

Two reviewers agreed on the fix: one document and one column pair per field. See !251354 (comment 3846581473) and !251354 (comment 3862419390)

Framework

In lib/gitlab/externally_stored_field.rb and lib/gitlab/markdown_cache/external_storage/extension.rb:

  • Fields register against a named payload: externally_stored_field(field, payload: field). externally_stored_payloads maps payloads to fields.
  • mount_external_storage mounts one CarrierWave uploader per payload, with <payload>_file and <payload>_file_store columns.
  • Loading, writing, clearing and destroy cleanup are per payload, so content no longer touches readiness_score_feedback.
  • external_payload_filename(payload) defaults to <payload>.json. Writes reuse the name already in the column, so backfilled rows keep pointing at the document they already have. No blob moves.
  • cache_markdown_field keeps a field's _html in its markdown field's payload.

FileStoreMounter

app/models/concerns/file_store_mounter.rb held an allowlist of mount column names (file, signed_file, predicate_file), so every new field had to be added there. It now checks the file / <prefix>_file convention it was guarding, as suggested in review.

Consumers

  • Ci::Catalog::BundledResources::Version drops its external_payload_filename override; the new default already returns readme.json. The unused ReadmeUploader::FILENAME constant is removed.
  • WorkItems::AgentPlan gains has_content?, which reads content_file alone. It overrides the filename for content.
  • payloadStored becomes hasContent on the agent plan widget. That field was never released, so there is no deprecation cycle.
  • Both models add ignore_columns :file_store with remove_with: '19.7', since each payload now carries its own store column.

Database

Two migrations add columns: content_file, content_file_store, readiness_score_feedback_file, readiness_score_feedback_file_store on work_item_agent_plans, and readme_file, readme_file_store on catalog_bundled_resource_versions. The earlier file column and its migrations are removed from this MR; they never shipped.

Four more backfill the pointer columns, along with the store each document was written to. content_file gets the name the combined document already used, so nothing moves in object storage, and readme_file gets readme.json, which is both the existing name and the new default.

Two run pre-deploy, because the code that reads these columns ships in the same deployment. The other two run the same statements again post-deploy, for rows the old code writes while it is still rolling out. Those read back empty, and editing one in that state overwrites the document that was there. Asked for in !251354 (comment 3959253894).

readiness_score_feedback is dropped, not migrated: splitting it out would mean reading every document, and it is unreleased and unread. Agreed in !251354 (comment 3874258348)

A known false positive in hasContent

The backfill sets content_file on every existing work_item_agent_plans row, including rows that only ever held the blank document written by the bug fixed in !257562 (merged). SQL cannot tell those apart, so they report hasContent: true until content is actually written. New rows are correct, because that bug is fixed.

This is accepted rather than worked around. Not backfilling makes hasContent exact but loses the content of existing beta plans, and the alternative of checking object storage from a migration was tried and dropped: the uploader that builds the storage key is EE only while db/post_migrate is shared with FOSS, and a wrong key deletes pointers to real content. Settled in !251354 (comment 3920505682) and confirmed again in !251354 (comment 3978526316).

References

Screenshots or screen recordings

No UI change, so no screenshots apply.

How to set up and validate locally

  1. Run the concern and model specs:

    bundle exec rspec spec/lib/gitlab/externally_stored_field_spec.rb ee/spec/models/work_items/agent_plan_spec.rb spec/models/ci/catalog/bundled_resources/version_spec.rb

  2. In a Rails console, find a work item with an agent plan. Check that work_item.reset.get_widget(:agent_plan).has_content_for_widget? returns true without calling read_external_payload, while agent_plan.content does call it.

Test results: 319 examples, 0 failures across the concern, both models, the widget, the GraphQL type, both uploaders, the callbacks and the catalog services. RuboCop is clean. Request specs cannot run in this local environment: a missing sass gem fails every request spec, so ee/spec/requests/api/graphql/work_item_spec.rb is covered by CI.

GraphQL

New field on WorkItemWidgetAgentPlan:

  • Field: hasContent
  • Type: Boolean!
  • Token scopes: api, read_api, ai_workflows
  • Resolves from: content_file column, no object storage read

content, contentHtml, readinessScoreFeedback and readinessScoreFeedbackHtml carry FieldCallCount with a limit of 1, because reading them costs an object storage round trip. So a list of work items can only resolve them for one item. hasContent reads a column, so it can be asked for every item in a list. The widget requires :update_work_item.

Single work item:

query {
  workItem(id: "gid://gitlab/WorkItem/1") {
    features {
      agentPlan {
        hasContent
      }
    }
  }
}

Across a list, which content cannot do:

query {
  group(fullPath: "my-group") {
    workItems {
      nodes {
        id
        widgets {
          ... on WorkItemWidgetAgentPlan {
            hasContent
          }
        }
      }
    }
  }
}

The generated reference entry is in doc/api/graphql/reference/_index.md.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist.

Edited by Dennis Meister

Merge request reports