feat(openapitoolset): add OpenAPI specification toolset - #451
Conversation
- Add AuthConfig, AuthCredential, AuthScheme types - Add AuthHandler for OAuth2 flow orchestration - Add CredentialManager and CredentialService for token management - Add OAuth2 token exchange helpers - Add unit tests for auth package
Summary of ChangesHello @cpunion, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's ability to interact with external APIs by introducing a dedicated toolset for OpenAPI specifications. It allows developers to define API interactions using standard OpenAPI documents, which are then automatically converted into executable tools. This streamlines the process of integrating new services and ensures consistent API consumption patterns, complete with flexible authentication mechanisms. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new toolset for generating tools from OpenAPI specifications. The implementation covers parsing the spec, creating RestApiTool instances, and handling various authentication schemes. The code is well-structured and includes a good set of unit tests. I've identified a few areas for improvement, including a correctness issue in parameter parsing, a missing feature in authentication handling, and a misleading function signature. Overall, this is a great addition.
| var params []Parameter | ||
|
|
||
| // Parse path-level parameters | ||
| if pathParams, ok := pathItem["parameters"].([]any); ok { | ||
| params = append(params, parseParameterList(pathParams)...) | ||
| } | ||
|
|
||
| // Parse operation-level parameters (override path-level) | ||
| if opParams, ok := op["parameters"].([]any); ok { | ||
| params = append(params, parseParameterList(opParams)...) | ||
| } |
There was a problem hiding this comment.
The current implementation appends both path-level and operation-level parameters. According to the OpenAPI specification, parameters defined at the operation level should override any parameters with the same name and location (in) defined at the path level. Simply appending them can lead to duplicate parameters and incorrect API requests.
To fix this, you can use a map to collect parameters, which will naturally handle the overriding, and then convert the map back to a slice.
paramMap := make(map[string]Parameter)
// Parse path-level parameters
if pathParams, ok := pathItem["parameters"].([]any); ok {
for _, p := range parseParameterList(pathParams) {
paramMap[p.Name+":"+p.In] = p
}
}
// Parse operation-level parameters (overrides path-level)
if opParams, ok := op["parameters"].([]any); ok {
for _, p := range parseParameterList(opParams) {
paramMap[p.Name+":"+p.In] = p
}
}
params := make([]Parameter, 0, len(paramMap))
for _, p := range paramMap {
params = append(params, p)
}| parsed, err := parseOperation(path, method, op, baseURL, pathItemMap) | ||
| if err != nil { | ||
| continue // Skip invalid operations | ||
| } |
There was a problem hiding this comment.
The parseOperation function is declared to return an error, but it never actually returns one. This makes the error handling here redundant and the function's signature misleading. Since the parsing logic is very lenient and designed to skip invalid parts rather than fail, it would be clearer to remove the error return type from parseOperation altogether.
This would also require changing the signature of parseOperation on line 121 and its return statement on line 156.
| parsed, err := parseOperation(path, method, op, baseURL, pathItemMap) | |
| if err != nil { | |
| continue // Skip invalid operations | |
| } | |
| parsed := parseOperation(path, method, op, baseURL, pathItemMap) |
| case auth.AuthCredentialTypeAPIKey: | ||
| if t.authCredential.APIKey != "" && t.authScheme != nil { | ||
| if apiKeyScheme, ok := t.authScheme.(*auth.APIKeyScheme); ok { | ||
| switch apiKeyScheme.In { | ||
| case auth.APIKeyInHeader: | ||
| headers[apiKeyScheme.Name] = t.authCredential.APIKey | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation for API key authentication only supports placing the key in the request header. The OpenAPI specification also allows API keys to be passed in query parameters (in: query). This functionality is missing.
To add support for this, you would need to:
- Update
buildURLto check if the auth scheme is an API key in a query parameter, and if so, add it to the query values. - Ensure this function,
getAuthHeaders, only handles thein: headercase for API keys.
9f2e0c9 to
22ebdb7
Compare
Add support for tools to request authentication credentials during execution: - Add RequestedAuthConfigs field to EventActions for storing auth requests - Implement GenerateAuthEvent to convert auth requests to adk_request_credential function calls - Add RequestAuthConfig method to ToolContext for tools to request authentication - Add AuthConfig field to Tool interface for tools to declare auth requirements - Integrate auth event generation into the LLM base flow Also fix test comparisons to ignore RequestedAuthConfigs field initialization and add UTC timezone setup in database tests for consistent timestamp formatting.
22ebdb7 to
4a3116a
Compare
4a3116a to
d6d4100
Compare
Add a new toolset that automatically generates tools from OpenAPI/Swagger specifications, enabling agents to interact with REST APIs. Key components: - OpenAPI spec parser supporting v3.0/v3.1 with JSON Schema conversion - RestApiTool for executing HTTP requests with path/query/body parameters - Toolset for loading specs from file/URL and generating tools - OAuth2 authentication support with credential request flow - Example demonstrating GitHub API integration with device auth flow
d6d4100 to
3d1bdc7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-structured feature: an OpenAPI toolset with comprehensive authentication support, including OAuth2. The new auth package is robust, with a clear separation of concerns for schemes, credentials, and lifecycle management. The use of registries for exchangers and refreshers is a good design choice for extensibility. The OpenAPI parser and the RestApiTool provide a powerful way to generate tools from specifications. I've identified a critical issue in the example's custom OAuth2 code exchange logic that should be refactored to use the standard library for better security and simplicity. Additionally, there are a couple of medium-severity issues related to parsing and key generation that could be improved for robustness and clarity. Overall, this is an excellent addition to the ADK.
|
Thank you for this, and I am sorry it took us eight months to reply. That delay is on us. An OpenAPI toolset is genuinely missing from adk-go and we do want one. This PR cannot be the vehicle, for three reasons. The auth half conflicts with a decision we have since made. This PR is stacked on #455 and #457. We closed #455 because adk-go had by then landed its own The parser does not resolve The branch has moved out from under itself. Main is 322 commits ahead, with 16 conflicting files including So we are closing this one. We would very much welcome a smaller PR covering only |
|
Tracking issue for the scoped version, as promised above: #1464. It records the constraints (credentials through @cpunion, you have first refusal on it if you want it. |
2 participants
Base on:
#455 feat(auth): add OAuth2/OIDC authentication package
#457 Feature/auth flow