Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(tools): openapi fixing and adding example #279

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(tools): openapi fixing and adding example #279

wants to merge 1 commit into from

Conversation

akihikokuroda
Copy link
Contributor

@akihikokuroda akihikokuroda commented Jan 13, 2025 •
edited
Loading

Which issue(s) does this pull-request address?

Closes: #275

Description

Checklist

  • I have read the contributor guide
  • I have signed off on my commit
  • Linting passes: yarn lint or yarn lint:fix
  • Formatting is applied: yarn format or yarn format:fix
  • Unit tests pass: yarn test:unit
  • E2E tests pass: yarn test:e2e
  • Tests are included
  • Documentation is changed or added
  • Commit messages and PR title follow conventional commits

@akihikokuroda akihikokuroda requested a review from a team as a code owner January 13, 2025 17:58
@akihikokuroda
Copy link
Contributor Author

akihikokuroda commented Jan 13, 2025 •
edited
Loading

So far some fixes are in commits:

  • name property is not propagated
  • description property is not propagated
  • httpProxyUrl is not used
  • example usage is missing
  • "$ref" doesn't work in the api schema
  • github schema seems too big.

Tomas2D
Tomas2D requested changes Jan 14, 2025
View reviewed changes
examples/tools/openapi.ts Outdated Show resolved Hide resolved
@akihikokuroda
Copy link
Contributor Author

Using the github OpenAPI schema from https://github.com/github/rest-api-description/blob/main/descriptions/api.github.com/api.github.com.json causes this error.
MemoryFatalError: Retrieved message (186568 tokens) cannot fit inside current memory (111412 tokens)

@akihikokuroda akihikokuroda changed the title [WIP] fix(tools): openapi fixing and adding example fix(tools): openapi fixing and adding example Jan 14, 2025
@Tomas2D
Copy link
Contributor

Tomas2D commented Jan 15, 2025

Try to find some smaller API or use it's subset.

@akihikokuroda
Copy link
Contributor Author

I was looking for some downloadable api schema from a site and downloading it in the sample but I couldn't find a good site that provides the api schema file and not required an API key or an access token for some of the API calls.
So I made a subset of github api schema and the example is using it.

Tomas2D
Tomas2D requested changes Jan 15, 2025
View reviewed changes
src/tools/openapi.ts Outdated Show resolved Hide resolved
src/tools/openapi.ts Outdated Show resolved Hide resolved
src/tools/openapi.ts Outdated
Comment on lines 158 to 160
this.url = options.url;
this.apiKey = options.apiKey;
this.httpProxyUrl = options.httpProxyUrl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong. You don't need to set those properties because they are already set in this.options

src/tools/openapi.ts Outdated
Comment on lines 205 to 207
apiKey: undefined,
httpProxyUrl: this.httpProxyUrl,
url: this.url,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove because they are already propagated from ...super.createSnapshot()

Tomas2D
Tomas2D requested changes Jan 20, 2025
View reviewed changes
src/tools/openapi.ts Outdated Show resolved Hide resolved
src/tools/openapi.ts Outdated
Comment on lines 168 to 171
const headers: { [key: string]: string } = { Accept: "application/json" };
if (this.apiKey) {
headers["Authorization"] = `Bearer ${this.apiKey}`;
if (this.options.apiKey) {
headers["Authorization"] = `Bearer ${this.options.apiKey}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

create body in advance, pass it to the emitter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what to do here. Would you explain a little more? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

 const options = {
    ...this.option.fetchOptions,
    body: !isEmpty(input.body) ? input.body : undefined,
    method: input.method.toLowerCase(),
    headers: { Accept: "application/json", ...this.option.fetchOptions?.headers, },
    
  }
 await this.emitter.emit("beforeFetch", { url, optionsl });
    try {
      const response = await fetch(
        url.toString(),
        options,
      );
      const text = await response.text();
      const output = new OpenAPIToolOutput(response.status, response.statusText, text);
      await this.emitter.emit("afterFetch", { data: output, url });
      return output;
    }

and change type of fetchOptions from any to RequestInit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!!

xjacka
xjacka requested changes Jan 20, 2025
View reviewed changes
src/tools/openapi.ts Outdated
if (this.apiKey) {
headers["Authorization"] = `Bearer ${this.apiKey}`;
if (this.options.apiKey) {
headers["Authorization"] = `Bearer ${this.options.apiKey}`;
Copy link

Choose a reason for hiding this comment

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

We can eliminate the 'apiKey' and the 'httpProxyUrl' parameters and allow the user to provide fetch options in the constructor. This solution will be more flexible.

src/tools/openapi.ts
) {
let path: string = input.path || "";
const url = new URL(this.openApiSchema.servers[0].url);
const url = new URL(this.options.url || this.openApiSchema.servers[0].url);
Copy link

Choose a reason for hiding this comment

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

The 'url' parameter is a duplication of the openApiSchema.servers.url parameter which is required in the constructor. Remove this check in the constructor and either make the URL parameter required or check that at least one of the server's definitions is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The servers object seems optional. https://swagger.io/specification/

xjacka
xjacka reviewed Jan 20, 2025
View reviewed changes
src/tools/openapi.ts Outdated
@@ -30,15 +30,20 @@ import { ValueError } from "@/errors.js";
import { SchemaObject } from "ajv";
import { parse } from "yaml";
import { isEmpty } from "remeda";
import { HttpsProxyAgent } from "https-proxy-agent";
import fetch from "node-fetch";

export interface OpenAPIToolOptions extends BaseToolOptions {
name: string;
Copy link

Choose a reason for hiding this comment

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

'Name' and the 'description' are still not propagated to the object.

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

@Tomas2D Tomas2D Tomas2D requested changes

@xjacka xjacka xjacka requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpenAPI Tool
3 participants
@akihikokuroda @Tomas2D @xjacka