laskoviymishka commented on code in PR #1345:
URL: https://github.com/apache/iceberg-go/pull/1345#discussion_r3501966714
##########
catalog/rest/rest.go:
##########
@@ -462,11 +470,6 @@ func fromProps(props iceberg.Properties, o *options) error
{
case keyRestSigV4Service:
o.sigv4Service = v
case keyAuthUrl:
Review Comment:
This empty `case keyAuthUrl:` reads like dead code, but it's load-bearing —
it's the only thing keeping the auth URL from falling into `default` and
getting pushed into `additionalProps`. Someone skimming will see every other
key with a one-line assignment and "clean this up" by deleting it, which
silently regresses the URL into `additionalProps`.
I'd just fold `parseAuthURL` back into the case here instead of hoisting it
above the loop:
```go
case keyAuthUrl:
u, err := parseAuthURL(v)
if err != nil {
return err
}
o.authUri = u
```
That keeps the testable-helper win, drops the pre-loop lookup, and the case
is self-evident again. wdyt?
##########
catalog/rest/rest_internal_test.go:
##########
@@ -54,35 +54,101 @@ import (
func TestLoadRegisteredCatalogRejectsInvalidAuthURL(t *testing.T) {
t.Parallel()
- cat, err := catalog.Load(context.Background(), "rest",
iceberg.Properties{
- "uri": "http://example.com";,
- "rest.authorization-url": "http://[::1";,
- })
- require.Error(t, err)
- assert.Nil(t, cat)
- assert.ErrorContains(t, err, "invalid rest.authorization-url")
+ tests := []struct {
Review Comment:
Every case in both tables asserts an error — there's no positive case
proving a well-formed `rest.authorization-url` (say
`https://auth.example.com/oauth/token`) still parses and lands on `o.authUri`
as the token endpoint.
The thing we actually shipped is "reject broken, keep valid", and right now
a regression that over-tightens the check — rejecting good URLs too — wouldn't
be caught by anything here. I'd add a success case asserting
`Load`/`NewCatalog` succeeds and the URL ends up where it should.
##########
catalog/rest/rest_internal_test.go:
##########
@@ -54,35 +54,101 @@ import (
func TestLoadRegisteredCatalogRejectsInvalidAuthURL(t *testing.T) {
t.Parallel()
- cat, err := catalog.Load(context.Background(), "rest",
iceberg.Properties{
- "uri": "http://example.com";,
- "rest.authorization-url": "http://[::1";,
- })
- require.Error(t, err)
- assert.Nil(t, cat)
- assert.ErrorContains(t, err, "invalid rest.authorization-url")
+ tests := []struct {
+ name string
+ authURL string
+ wantErr string
+ }{
+ {
+ name: "malformed URL",
+ authURL: "http://[::1";,
+ wantErr: "invalid rest.authorization-url",
+ },
+ {
+ name: "missing scheme",
+ authURL: "example.com/auth",
+ wantErr: "missing scheme or host",
+ },
+ {
+ name: "missing host",
Review Comment:
`url.Parse("localhost:8080")` parses `localhost` as the scheme and `8080` as
`Opaque` — so `Host` is empty and the assertion passes, but for a different
reason than "missing host" implies. There's no case that actually exercises a
valid scheme with a genuinely empty host.
I'd rename this to something like `"scheme+opaque, no host"` and add a real
`"http://"` (or `"http:///path"`) case to cover the true
empty-host-with-valid-scheme path.
##########
catalog/rest/rest.go:
##########
@@ -500,6 +503,18 @@ func fromProps(props iceberg.Properties, o *options) error
{
return nil
}
+func parseAuthURL(raw string) (*url.URL, error) {
Review Comment:
Two small things here, neither blocking. The `"missing scheme or host"`
message doesn't say which one is actually missing — a touch more precision
would help whoever's debugging a bad config.
And this only covers the config-string path; `WithAuthURI` (options.go)
still lets a caller set a relative `*url.URL` directly, and `setupOAuthManager`
calls `.String()` on it unconditionally. Scoping this PR to the config path is
reasonable, but the invariant "if `authUri != nil` it's absolute" isn't
enforced catalog-wide. Fine to leave for a follow-up — flagging so it's a
deliberate choice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
