Skip to content

refactor!: Pass LDAP mapping request bodies by value via new UpdateUserLDAPMappingRequest and UpdateTeamLDAPMappingRequest - #4432

Merged
gmlewis merged 1 commit into
google:masterfrom
JamBalaya56562:refactor/3644-ldap-mapping-value-params
Aug 5, 2026
Merged

refactor!: Pass LDAP mapping request bodies by value via new UpdateUserLDAPMappingRequest and UpdateTeamLDAPMappingRequest#4432
gmlewis merged 1 commit into
google:masterfrom
JamBalaya56562:refactor/3644-ldap-mapping-value-params

Conversation

@JamBalaya56562

Copy link
Copy Markdown
Contributor

Continues the request-body-by-value work in #3644, this time for the two LDAP mapping endpoints on AdminService.

Both methods reused their response types as request bodies, but per the GHES docs each endpoint accepts exactly one body parameter, ldap_dn, and it's required:

  • UpdateUserLDAPMapping took *UserLDAPMapping — 17 all-pointer fields, most of them server-generated (avatar_url, events_url, …).
  • UpdateTeamLDAPMapping took *TeamLDAPMapping — 10 all-pointer fields, same story.

The existing tests show this: both build the request with only LDAPDN set, while the response expectations populate more fields. So this adds dedicated request types, following the same approach as PullRequestSubmitReviewRequest (#4406) and UpdateConnectedExternalGroupRequest (#4425):

typeUpdateUserLDAPMappingRequeststruct {
LDAPDNstring`json:"ldap_dn"`
}
typeUpdateTeamLDAPMappingRequeststruct {
LDAPDNstring`json:"ldap_dn"`
}

LDAPDN is a non-pointer string since it's required, both bodies are passed by value, and both old entries are removed from the body-allowed-pointer-types allowlist. The two methods live in the same file and follow the identical pattern, so they're converted together.

Notes:

  • UserLDAPMapping and TeamLDAPMapping are unchanged — they stay the response types, so their fields keep pointer semantics.
  • The method verbs already match the docs operation names ("Update LDAP mapping for a user/team"), so no method rename.
  • The team int64 parameter is renamed to teamID for clarity (non-breaking).

Verified with go build ./..., go vet -tags integration ./test/integration/, gofmt, the full ./github/ test suite (both methods and the generated GetLDAPDN accessors at 100%), and custom-gcl (no paramcheck findings after removing the allowlist entries).

Updates #3644

BREAKING CHANGE: AdminService.UpdateUserLDAPMapping and UpdateTeamLDAPMapping now take new UpdateUserLDAPMappingRequest and UpdateTeamLDAPMappingRequest (with non-pointer LDAPDN) by value instead of *UserLDAPMapping and *TeamLDAPMapping.

cc @jvm986 — flagging for #3644 coordination; this is in the admin service, so it shouldn't overlap with the Issues work.

…serLDAPMappingRequest` and `UpdateTeamLDAPMappingRequest`
UpdateUserLDAPMapping and UpdateTeamLDAPMapping reused the UserLDAPMapping
and TeamLDAPMapping response types as their request bodies, but ldap_dn is
the only parameter either endpoint accepts, and it is required. The new
request types model that schema exactly, with a non-pointer LDAPDN, and are
passed by value.
The response types stay unchanged, the teamID parameter is renamed for
clarity, and both old entries are removed from the .golangci.yml allowlist.
BREAKING CHANGE: AdminService.UpdateUserLDAPMapping and UpdateTeamLDAPMapping now take new UpdateUserLDAPMappingRequest and UpdateTeamLDAPMappingRequest (with non-pointer LDAPDN) by value instead of *UserLDAPMapping and *TeamLDAPMapping.

@jvm986jvm986 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@codecov

codecovBot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.54%. Comparing base (40d29ba) to head (07656f5).

Additional details and impacted files
@@ Coverage Diff @@## master #4432 +/- ##
=======================================
Coverage 97.54% 97.54% =======================================
Files 194 194 Lines 19836 19836 =======================================
Hits 19350 19350 Misses 268 268 Partials 218 218 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gmlewisgmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Aug 5, 2026

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @JamBalaya56562 and @jvm986!
LGTM.
Merging.

@gmlewis
gmlewis merged commit 30c65e0 into google:masterAug 5, 2026
15 checks passed
@JamBalaya56562
JamBalaya56562 deleted the refactor/3644-ldap-mapping-value-params branch August 5, 2026 23:27
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking API ChangePR will require a bump to the major version num in next release. Look here to see the change(s).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@JamBalaya56562@gmlewis@jvm986