Skip to content

Commit df52b3a

Browse files
authored
docs(1to1): declare the 49 project-group fields as a wrong route annotation (#688)
## Description The largest single block left in the sent backlog turned out to be no work at all, and the reason is worth recording: it was GitLab's route annotation that was wrong, not our type. `projects.ProjectGroupOutput` was held against the whole `Group` entity and reported missing 49 of its fields. It is already 1:1 with what the endpoints send. `lib/api/projects.rb` describes both `GET :id/share_locations` and `GET :id/invited_groups` as answering with `Entities::Group`, and both call `present_groups`, the helper defined a few hundred lines above them in the same file, which presents `Entities::PublicGroupDetails`. Their sibling `GET :id/groups` calls that same helper and is annotated `PublicGroupDetails`, correctly, so three adjacent routes share one helper and two of them disagree with it. `PublicGroupDetails` is `BasicGroupDetails` plus `avatar_url`, `full_name` and `full_path`. Six keys, against roughly seventy for `Group`. All three endpoints answer with exactly those six on GitLab.com, and `ProjectGroupOutput` publishes all six. I checked this against the live API rather than by reading the source, because this is the third time in this review that the annotation has been the thing that was wrong. ## Related Issue No single issue: this is the project-groups tranche of the continuing 1:1 field review, whose upstream half is tracked in <https://gitlab.com/gitlab-org/api/client-go/-/issues/2300>. The GitLab-side fix is [gitlab-org/gitlab!254699](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254699), opened today, alongside [gitlab-org/gitlab!254698](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254698) for three annotations of the same class in `lib/api/project_job_token_scope.rb`. ## Type of Change - [x] Documentation update - [x] Enhancement (improvement to existing functionality) ## Changes Made - One declaration in `cmd/audit_1to1/internal/paths/sent_declarations.go` under `documented-response-is-not-the-one-sent`, keyed by package and entity with the wildcard field, since every field read on the component has the same answer. - An entry in `docs/development/upstream-bugs.md` recording the finding, the evidence, and the merge request that fixes it. - README stats regenerated. ## Results | counter | before | after | | --- | --- | --- | | undeclared findings | 117 | **68** | | `typed_unsurfaced_declared` | 1129 | **1178** | | `stale_declarations` | 0 | 0 | `typed_unsurfaced_fields` stays 1246: nothing was published, one block was answered. What remains, for the next tranche: `projects.Output` vs `Project` 17, `groups.Output` vs `BasicProjectDetails` 13, `issues.Output` vs `EpicIssue` 10, `groups.Output` vs `Group` 10, `groups.Output` vs `GroupDetail` 8, and a tail of 10 across six types. ## How to Test 1. `RGO_DIR="$PWD" /root/.claude/bin/rgo 'go run ./cmd/audit_1to1/ -scope=paths'` and read the summary. The JSON goes to stderr. 2. `go test ./cmd/audit_1to1/... -count=1` passes, including the declaration table's own staleness test. 3. Against any instance, call all three endpoints and count the keys: six each. ## Breaking Changes / Migration Notes N/A. No runtime code changes. ## Checklist ### Code Quality - [x] `golangci-lint run ./cmd/audit_1to1/...` clean - [x] Follows the conventions in `CLAUDE.md` ### Testing - [x] `go test ./cmd/audit_1to1/... -count=1` passes - [x] `stale_declarations` stays 0, so the declaration matches something ### Documentation - [x] `docs/development/upstream-bugs.md` records the finding and the merge request - [x] `markdownlint-cli2` clean on the changed file - [x] README stats regenerated - [x] Commit message follows Conventional Commits ### Security - [x] No secrets, tokens or credentials
1 parent 9ae164b commit df52b3a

3 files changed

Lines changed: 60 additions & 3 deletions

File tree

README.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -467,10 +467,10 @@ and is never logged. Full details: [PRIVACY.md](PRIVACY.md).
467467

468468
| Category | Files | Lines |
469469
| ------------------------ | --------: | ----------: |
470-
| Source (`.go`, non-test) | 1,246 | 267,003 |
470+
| Source (`.go`, non-test) | 1,246 | 267,017 |
471471
| Unit tests (`_test.go`) | 735 | 437,473 |
472472
| End-to-end tests | 246 | 66,458 |
473-
| **Total** | **2,227** | **770,934** |
473+
| **Total** | **2,227** | **770,948** |
474474

475475
### Functions
476476

@@ -523,7 +523,7 @@ and is never logged. Full details: [PRIVACY.md](PRIVACY.md).
523523
| Fact | Value |
524524
| ------------------------------------ | ---------------------------------------------------------------------------------------------------- |
525525
| Source code printed at 55 lines/page | ~4,854 pages of A4 |
526-
| Source lines mentioning `"gitlab"` | 15,510 (impossible to avoid) |
526+
| Source lines mentioning `"gitlab"` | 15,512 (impossible to avoid) |
527527
| Longest function name in source | `assertDynamicCompatibilityPolicyOwnedByActionCompat` (51 chars) |
528528
| Longest test function name | `TestRequiredMissingAndUnknownParamNames_SchemaValidation_ReturnsSortedMissingAndUnknown` (87 chars) |
529529

cmd/audit_1to1/internal/paths/sent_declarations.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,20 @@ var declaredUnsurfaced = []sentDeclaration{ //nolint:gochecknoglobals // the adj
261261
"pending-invitation object is what the GET at the same path lists. The shape declaration for the other " +
262262
"direction of this join records the same thing.",
263263
},
264+
{
265+
Package: toolsDir + "/projects",
266+
Entity: "API::Entities::Group",
267+
Field: declaredSegment,
268+
Category: categoryDocumentedNotSent,
269+
Reason: "lib/api/projects.rb describes GET :id/share_locations and GET :id/invited_groups as answering with " +
270+
"Entities::Group, and both call present_groups, the helper defined in the same file, which presents " +
271+
"Entities::PublicGroupDetails. Their sibling GET :id/groups calls the same helper and is annotated " +
272+
"PublicGroupDetails, correctly. PublicGroupDetails is BasicGroupDetails plus avatar_url, full_name and " +
273+
"full_path, which is six keys, and all three endpoints answer with exactly those six on GitLab.com: id, " +
274+
"name, avatar_url, web_url, full_name, full_path. ProjectGroupOutput publishes all six, so the type is " +
275+
"already 1:1 and the 49 fields read against it are the whole Group entity arriving through the wrong " +
276+
"annotation. Recorded in docs/development/upstream-bugs.md; the fix is gitlab-org/gitlab!254699.",
277+
},
264278
// The nine membership keys the billable members list is read against.
265279
// Named one by one rather than with a splat: internal/tools/groupmembers
266280
// also publishes API::Entities::Member on its own Output, where a finding

docs/development/upstream-bugs.md

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,49 @@ connecting it to the output type.
199199
**Effort**: small. A documentation correction, though the deprecated POST's
200200
example needs to stay reachable for callers still using it.
201201

202+
### Two project group listings are annotated with the whole Group entity
203+
204+
- **Reported**: yes.
205+
- **In review**: yes,
206+
[gitlab-org/gitlab!254699](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/254699).
207+
- **Merged**: no.
208+
- **Blocking**: no. Our output type is already the right shape; only the audit
209+
was misled.
210+
- **Workaround**: yes, a declaration. `cmd/audit_1to1/internal/paths/sent_declarations.go`
211+
answers the 49 findings this raised against `projects.ProjectGroupOutput`
212+
under `documented-response-is-not-the-one-sent`.
213+
214+
**Where**: `lib/api/projects.rb`, the `desc` blocks for
215+
`GET :id/share_locations` and `GET :id/invited_groups`.
216+
217+
**What**: both descriptions say `success Entities::Group`, and both handlers
218+
call `present_groups`, defined a few hundred lines above in the same file,
219+
which presents `with: Entities::PublicGroupDetails`. `PublicGroupDetails` is
220+
`BasicGroupDetails` plus `avatar_url`, `full_name` and `full_path`, so six keys
221+
in total, against roughly seventy for `Group`.
222+
223+
Their sibling `GET :id/groups` calls the same helper and is annotated
224+
`Entities::PublicGroupDetails`, correctly, so three adjacent routes share one
225+
helper and two of them disagree with it.
226+
227+
**Confirmed against the API**, not by reading alone. All three endpoints answer
228+
with exactly six keys on GitLab.com: `id`, `name`, `avatar_url`, `web_url`,
229+
`full_name`, `full_path`.
230+
231+
**Root cause**: the same class as the three job token scope annotations
232+
recorded above. A `desc` block naming an entity the handler does not present is
233+
invisible to every test, because Grape uses it for documentation only.
234+
235+
**How we found it**: the R-PATH sent dimension read `Entities::Group` off the
236+
route annotation and reported all 49 of that entity's fields as missing from
237+
`ProjectGroupOutput`, which publishes the six the endpoint really sends. It was
238+
the largest single block left in the backlog, 49 of 117, and it was not work at
239+
all.
240+
241+
**Effort**: small. One line in `lib/api/projects.rb` plus the regenerated
242+
OpenAPI document, where the change is a single `$ref`, because
243+
`APIEntitiesPublicGroupDetails` is already a component of the document.
244+
202245
## GitLab client (`gitlab.com/gitlab-org/api/client-go`)
203246

204247
### Panic unmarshalling an issue with no id

0 commit comments

Comments
 (0)