Skip to content

safe GraphQL client for non-CLI calling scenarios#224

Merged
kezhenxu94 merged 2 commits into
apache:masterfrom
CodePrometheus:safe-graphql
Jun 4, 2025
Merged

safe GraphQL client for non-CLI calling scenarios#224
kezhenxu94 merged 2 commits into
apache:masterfrom
CodePrometheus:safe-graphql

Conversation

@CodePrometheus
Copy link
Copy Markdown
Contributor

To avoid panic when the caller comes from a non-cli scenario, parameters such as username may not have value, this pr adds relevant checks and default value.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the GraphQL client to safely handle missing context parameters in non-CLI scenarios by adding type checks and default values.

  • Use safe context.Value extraction for Insecure, BaseURL, Username, Password, and Authorization
  • Provide a hard-coded default baseURL when none is supplied
  • Construct Basic auth header only when both username and password are present
Comments suppressed due to low confidence (1)

pkg/graphql/client/client.go:58

  • Add unit tests for scenarios where context values are missing to verify default baseURL, insecure flag handling, and authorization header construction.
if baseURL == "" {

Comment thread pkg/graphql/client/client.go Outdated
Comment thread pkg/graphql/client/client.go Outdated
Comment thread pkg/graphql/client/client.go Outdated
@wu-sheng wu-sheng added this to the 0.15.0 milestone Jun 2, 2025
@wu-sheng wu-sheng added the enhancement New feature or request label Jun 2, 2025
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Actually this GraphQL client is not coupled with CLI, if you are using this client in your MCP server, you should pass the context.Context with the contextkey.Insecure{} set to false

@kezhenxu94
Copy link
Copy Markdown
Member

Example:

ctx := context.Background()
ctx = context.WithValue(ctx, contextkey.Insecure{}, true)
// pass the ctx to the graphql client

@CodePrometheus
Copy link
Copy Markdown
Contributor Author

@kezhenxu94 Yeah, I could default Insecure to true/false or make it configurable, I'll revert changes for this parameter. But for the other four parameters validation, let's keep this PR's approach, does this make sense to us?

@kezhenxu94 kezhenxu94 merged commit 77b4c49 into apache:master Jun 4, 2025
5 checks passed
@CodePrometheus CodePrometheus deleted the safe-graphql branch June 4, 2025 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants