Skip to content

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

Open
JamBalaya56562 wants to merge 1 commit into
google:masterfrom
JamBalaya56562:refactor/3644-ldap-mapping-value-params
Open

refactor!: Pass LDAP mapping request bodies by value via new UpdateUserLDAPMappingRequest and UpdateTeamLDAPMappingRequest#4432
JamBalaya56562 wants to merge 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):

type UpdateUserLDAPMappingRequest struct {
	LDAPDN string `json:"ldap_dn"`
}

type UpdateTeamLDAPMappingRequest struct {
	LDAPDN string `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.

@jvm986 jvm986 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

codecov Bot 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.

@gmlewis gmlewis 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking API Change PR 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