Skip to content

fix(rest): honor token property in catalog config#1330

Open
fallintoplace wants to merge 1 commit into
apache:mainfrom
fallintoplace:fix/rest-token-prop-auth
Open

fix(rest): honor token property in catalog config#1330
fallintoplace wants to merge 1 commit into
apache:mainfrom
fallintoplace:fix/rest-token-prop-auth

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

parse the REST catalog token property into the static OAuth token option
preserve that static token in the catalog's retained properties
add regression coverage for helper parsing/serialization and the catalog.Load(..., {"token": ...}) auth path

Why

The REST catalog already defines token as a first-class property key, and WithOAuthToken correctly wires a static bearer token into authentication. But the property path was incomplete: fromProps ignored token, so catalog.Load silently skipped static-token auth, and toProps dropped it from the catalog's retained properties.

This made property-driven REST catalog auth behave differently from the explicit option path.

Testing

go test ./catalog/rest -run 'Test(ToPropsSigv4RegionFallback|FromPropsReadsOAuthToken|TokenAuthenticationPriority|RestTLSCatalogSuite)$' -count=1
go test ./catalog/rest -count=1
go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.4 run --timeout=10m ./catalog/rest/...
git diff --check

@fallintoplace fallintoplace requested a review from zeroshade as a code owner June 27, 2026 14:43

@tanmayrauth tanmayrauth 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.

Clean fix, the property path was genuinely broken and this makes it symmetric with the explicit option. LGTM. A couple of notes inline.

Comment thread catalog/rest/rest.go
for k, v := range props {
switch k {
case keyOauthToken:
o.oauthToken = v

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.

Nice, this is the actual fix: before, token fell through to the default case and landed in additionalProps, so setupOAuthManager (which reads opts.oauthToken) never saw it and property-driven auth was silently a no-op. Wiring it into the typed field also means the very first /v1/config call is now authenticated.

Comment thread catalog/rest/rest.go
}
}

setIf(keyOauthToken, o.oauthToken)

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.

Good that you added this alongside the fromProps change — it's required now that token no longer lands in additionalProps (which toProps copies via maps.Copy), so without this the token would drop out of the catalog's retained props. Sits right next to the existing credential setIf, so it's consistent.

keyOauthToken: "static-token",
}, &opts)

assert.Equal(t, "static-token", opts.oauthToken)

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.

The assert.Nil(t, opts.additionalProps) here is the assertion I'd most want, it proves the token isn't double-stored in the free-form bucket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants