danielemoraschi commented on code in PR #8791: URL: https://github.com/apache/incubator-devlake/pull/8791#discussion_r3006064300
########## backend/plugins/github/tasks/http_client.go: ########## @@ -0,0 +1,89 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package tasks + +import ( + "net/http" + + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/plugin" + "github.com/apache/incubator-devlake/plugins/github/models" + "github.com/apache/incubator-devlake/plugins/github/token" +) + +func CreateAuthenticatedHttpClient( + taskCtx plugin.TaskContext, + connection *models.GithubConnection, + baseClient *http.Client, +) (*http.Client, errors.Error) { + + logger := taskCtx.GetLogger() + db := taskCtx.GetDal() + encryptionSecret := taskCtx.GetConfig(plugin.EncodeKeyEnvStr) + + if baseClient == nil { + baseClient = &http.Client{} + } + + // Inject TokenProvider for OAuth refresh or GitHub App installation tokens. + var tp *token.TokenProvider + if connection.RefreshToken != "" { + tp = token.NewTokenProvider(connection, db, baseClient, logger, encryptionSecret) + } else if connection.AuthMethod == models.AppKey && connection.InstallationID != 0 { + tp = token.NewAppInstallationTokenProvider(connection, db, baseClient, logger, encryptionSecret) + } + + baseTransport := baseClient.Transport + if baseTransport == nil { + baseTransport = http.DefaultTransport + } + + if tp != nil { + baseClient.Transport = token.NewRefreshRoundTripper(baseTransport, tp) + logger.Info( + "Installed token refresh round tripper for connection %d (authMethod=%s)", + connection.ID, + connection.AuthMethod, + ) + + } else if connection.Token != "" { Review Comment: `connection.Token` can contain multiple comma-separated tokens (e.g. tok1,tok2,tok3). The old code in `impl.go` split on comma and used only the first token (tokens[0]) via oauth2.StaticTokenSource. This passes `connection.Token` directly to `StaticRoundTripper`, which injects the entire unsplit string as `Authorization: Bearer tok1,tok2,tok3`. On the REST path, this also overwrites the correctly-rotated single token set by `SetupAuthentication` at the request level. ########## backend/helpers/pluginhelper/api/graphql_async_client.go: ########## @@ -218,3 +246,29 @@ func (apiClient *GraphqlAsyncClient) Wait() { func (apiClient *GraphqlAsyncClient) Release() { apiClient.cancel() } + +// WithFallbackRateLimit sets the initial/fallback rate limit used when +// rate limit information cannot be fetched dynamically. +// This value may be overridden later by getRateRemaining. +func WithFallbackRateLimit(limit int) GraphqlClientOption { + return func(c *GraphqlAsyncClient) { + if limit > 0 { + c.rateRemaining = limit + } + } +} + +// resolveRateLimit determines the rate limit for GraphQL requests using task configuration -> else default constant. +func resolveRateLimit(taskCtx plugin.TaskContext, logger log.Logger) int { Review Comment: `resolveRateLimit` reads env config at line 69, then opts (including WithFallbackRateLimit(5000)) are applied at lines 82-84, unconditionally overwriting the env value. Wouldn't a user setting `GRAPHQL_RATE_LIMIT=2000` be silently overridden by the code-level "fallback" of 5000 from graphql_client.go:69? ########## backend/plugins/github_graphql/tasks/graphql_client.go: ########## @@ -0,0 +1,71 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package tasks + +import ( + "context" + "fmt" + "github.com/apache/incubator-devlake/core/log" + "net/url" + "time" + + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/plugin" + helper "github.com/apache/incubator-devlake/helpers/pluginhelper/api" + "github.com/apache/incubator-devlake/plugins/github/models" + githubTasks "github.com/apache/incubator-devlake/plugins/github/tasks" + "github.com/merico-ai/graphql" +) + +func CreateGraphqlClient( + taskCtx plugin.TaskContext, + connection *models.GithubConnection, + getRateRemaining func(context.Context, *graphql.Client, log.Logger) (rateRemaining int, resetAt *time.Time, err errors.Error), +) (*helper.GraphqlAsyncClient, errors.Error) { + + // inject the shared auth layer + httpClient, err := githubTasks.CreateAuthenticatedHttpClient(taskCtx, connection, nil) Review Comment: Calling this with `nil` as the base client I believe will default to an empty `http.Client`. This ignores the proxy configuration (`connection.GetProxy()`) and abandons the HTTP/SOCKS5 Proxy logic associated with the connection. It might break enterprise proxy setups as the proxy configuration is never attached, while the previous implementation explicitly injected a custom proxy `http.Transport` in `backend/plugins/github_graphql/impl/impl.go` into the GraphQL HTTP client. This refactoring drops that setup for corporate proxy users. ########## backend/plugins/github_graphql/impl/impl.go: ########## @@ -230,8 +189,8 @@ func (p GithubGraphql) PrepareTaskData(taskCtx plugin.TaskContext, options map[s return 0, nil, errors.Default.Wrap(dataErrors[0], `query rate limit fail`) } if query.RateLimit == nil { - logger.Info(`github graphql rate limit are disabled, fallback to 5000req/hour`) - return 5000, nil, nil + logger.Info(`github graphql rate limit unavailable, using fallback rate limit`) + return 0, nil, errors.Default.New("rate limit unavailable") Review Comment: The old code returned `5000, nil, nil` for GitHub Enterprise with rate limiting disabled. This now returns an error instead of the previous 5000. The error propagates so the functionality is preserved, but every task run against such servers now produces a warning for what was previously a silently handled normal case. ########## backend/plugins/github/tasks/http_client.go: ########## @@ -0,0 +1,89 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package tasks + +import ( + "net/http" + + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/plugin" + "github.com/apache/incubator-devlake/plugins/github/models" + "github.com/apache/incubator-devlake/plugins/github/token" +) + +func CreateAuthenticatedHttpClient( + taskCtx plugin.TaskContext, + connection *models.GithubConnection, + baseClient *http.Client, +) (*http.Client, errors.Error) { + + logger := taskCtx.GetLogger() + db := taskCtx.GetDal() + encryptionSecret := taskCtx.GetConfig(plugin.EncodeKeyEnvStr) + + if baseClient == nil { + baseClient = &http.Client{} + } + + // Inject TokenProvider for OAuth refresh or GitHub App installation tokens. + var tp *token.TokenProvider + if connection.RefreshToken != "" { + tp = token.NewTokenProvider(connection, db, baseClient, logger, encryptionSecret) + } else if connection.AuthMethod == models.AppKey && connection.InstallationID != 0 { + tp = token.NewAppInstallationTokenProvider(connection, db, baseClient, logger, encryptionSecret) + } + + baseTransport := baseClient.Transport + if baseTransport == nil { + baseTransport = http.DefaultTransport + } + + if tp != nil { + baseClient.Transport = token.NewRefreshRoundTripper(baseTransport, tp) + logger.Info( + "Installed token refresh round tripper for connection %d (authMethod=%s)", + connection.ID, + connection.AuthMethod, + ) + + } else if connection.Token != "" { + baseClient.Transport = token.NewStaticRoundTripper( + baseTransport, + connection.Token, + ) + logger.Info( + "Installed static token round tripper for connection %d", + connection.ID, + ) + } + + // Persist the freshly minted token so the DB has a correctly encrypted value. + // PrepareApiClient (called by NewApiClientFromConnection) mints the token + // in-memory but does not persist it; without this, the DB may contain a stale + // or corrupted token that breaks GET /connections. + if connection.AuthMethod == models.AppKey && connection.Token != "" { + if err := token.PersistEncryptedTokenColumns(db, connection, encryptionSecret, logger, false); err != nil { + logger.Warn(err, "Failed to persist initial token for connection %d", connection.ID) + } else { + logger.Info("Persisted initial token for connection %d", connection.ID) + } + } + println("http client HIT3") Review Comment: Leftover development debug print statement deployed in production code. -- 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]
