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.

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

OpenAPI Spec #470

Closed
WyriHaximus opened this issue May 2, 2021 · 21 comments
Closed

OpenAPI Spec #470

WyriHaximus opened this issue May 2, 2021 · 21 comments
Labels
Type: Feature New feature or request

Comments

@WyriHaximus
Copy link
Contributor

What’s missing?

An OpenAPI spec YAML file for the payload-schemas.

Why?

Because it will bring having the webhook schema definitions in the official OpenAPI spec one step over: github/rest-api-description#21

Alternatives you tried

Tried converting the JSON Schema file with several tools, but they all failed when getting into the second level of object properties.

@wolfy1339
Copy link
Member

wolfy1339 commented May 2, 2021

I don't think adding a YAML version would be in the plans. If someone needs it they can make one by converting the JSON.

This would require substantial changes. We currently have separate schemas for each event's actions, and then we merge them together as one single file (with some slight modifications) during the build process.

Would you be willing to help out if we decide to integrate OpenAPI?

@wolfy1339
Copy link
Member

@G-Rath @gr2m Any thoughts?

@WyriHaximus
Copy link
Contributor Author

I don't think adding a YAML version would be in the plans. If someone needs it they can make one by converting the JSON.

Tried that last night and got stuck at some point. If you have a way to get a correctly converted YAML I'm interested.

This would require substantial changes. We currently have separate schemas for each event's actions, and then we merge them together as one single file (with some slight modifications) during the build process.

Looking at the code and spec that comes out of it both JSON Schema and OpenAPI seems very similar.

Would you be willing to help out if we decide to integrate OpenAPI?

Was already looking at how the project is set up and works, so yes 👍 .

@G-Rath @gr2m Any thoughts?

@gr2m is the one pointing me over here: github/rest-api-description#21 (comment)

@wolfy1339
Copy link
Member

I don't think adding a YAML version would be in the plans. If someone needs it they can make one by converting the JSON.

Tried that last night and got stuck at some point. If you have a way to get a correctly converted YAML I'm interested.

You're asking for an OpenAPI YAML version. I'm saying that we could provide a JSON version, but I'm unsure if we would provide a YAML version

Looking at the code and spec that comes out of it both JSON Schema and OpenAPI seems very similar.

The schemas, as they are currently, can almost be used in OpenAPI but there are some small differences.

Would you be willing to help out if we decide to integrate OpenAPI?

Was already looking at how the project is set up and works, so yes 👍

I made a simple conversion, and it includes a YAML version here: https://github.com/octokit/webhooks/tree/openapi
It really isn't the best, and isn't spec compliant due to the fact that we are using a custom property that isn't prefixed with x-

@WyriHaximus
Copy link
Contributor Author

I don't think adding a YAML version would be in the plans. If someone needs it they can make one by converting the JSON.

Tried that last night and got stuck at some point. If you have a way to get a correctly converted YAML I'm interested.

You're asking for an OpenAPI YAML version. I'm saying that we could provide a JSON version, but I'm unsure if we would provide a YAML version

Ah that way, don't mind at all if a JSON or a YAML, sorry for the confusion.

Looking at the code and spec that comes out of it both JSON Schema and OpenAPI seems very similar.

The schemas, as they are currently, can almost be used in OpenAPI but there are some small differences.

That might explain some of my confusion 😂 .

Would you be willing to help out if we decide to integrate OpenAPI?

Was already looking at how the project is set up and works, so yes 👍

I made a simple conversion, and it includes a YAML version here: https://github.com/octokit/webhooks/tree/openapi
It really isn't the best, and isn't spec compliant due to the fact that we are using a custom property that isn't prefixed with x-

If and how can I help to improve it?

@wolfy1339
Copy link
Member

If and how can I help to improve it?

A proper implementation without all the hackyness that can validate against the OpenAPI spec.
My conversion is a proof-of-concept and should only be used as a base

@gr2m
Copy link
Contributor

gr2m commented May 2, 2021

@G-Rath @gr2m Any thoughts?

@gr2m is the one pointing me over here: github/rest-api-description#21 (comment)

Both a YAML output and OpenAPI spec is out of scope of this repository. This is a community effort, not driven by any GitHub employees. If you'd like to build upon this repository and create an OpenAPI spec in a YAML format on it, you'd need to do it yourself.

But the core part that is hard to create and maintain are the JSON Schemas for all the webhook requests, and that's what @wolfy1339 and the others invested a lot of time into already

@WyriHaximus
Copy link
Contributor Author

If and how can I help to improve it?

A proper implementation without all the hackyness that can validate against the OpenAPI spec.
My conversion is a proof-of-concept and should only be used as a base

vs.

@gr2m is the one pointing me over here: github/rest-api-description#21 (comment)

Both a YAML output and OpenAPI spec is out of scope of this repository. This is a community effort, not driven by any GitHub employees. If you'd like to build upon this repository and create an OpenAPI spec in a YAML format on it, you'd need to do it yourself.

Those two statements seem conflicting. My main intention is to bridge to time from now until GitHub will provide these schema's. I'm game for helping put, initial check uncovered I can't currently parse https://github.com/octokit/webhooks/blob/openapi/payload-schemas/openapi-schema.yml due to a missing request body (user). But if it's out of scope of this repository I also don't want to spent to much time on it.

But the core part that is hard to create and maintain are the JSON Schemas for all the webhook requests, and that's what @wolfy1339 and the others invested a lot of time into already

❤️ !

@wolfy1339
Copy link
Member

In #136 (comment)

For future reference, we might consider changing the format of the published assets to follow the OpenAPI 3.1 specification for webhooks.

There seemed to be a willingness in the past for changing to OpenAPI

Before GitHub published their own OpenAPI spec, there was @octokit/routes which was a community maintained OpenAPI spec for the REST api.

I just demonstrated a quick and easy solution to publishing an OpenAPI spec for those who need it. There's no need to change the input format or anything


due to a missing request body (user). But if it's out of scope of this repository I also don't want to spent to much time on it.

That is definitely a bug. I just fixed it

WyriHaximus added a commit to php-api-clients/github that referenced this issue May 29, 2021
@gr2m
Copy link
Contributor

gr2m commented Oct 25, 2022

fyi as of v2.1.0, GitHub's OpenAPI spec includes webhooks. I didn't look into it in detail, just sharing the announcement

https://github.com/github/rest-api-description/releases/tag/v2.1.0

@wolfy1339
Copy link
Member

🎉 That's amazing news!

How should we handle the transition in this repo, since there's now official openapi spec?

@gr2m
Copy link
Contributor

gr2m commented Oct 25, 2022

From my experience with transitioning https://github.com/octokit/routes where we generated an OpenAPI Spec to github.com/octokit/openapi where we utilize GitHub's OpenAPI spec, there will be some discrepancies, I'm sure the first version of the webhooks in the OpenAPI spec will lack some things that we have in our specs.

I'd start out by creating a setup where we load GitHub's new OpenAPI spec and compare them to ours, to find these discrepancies. Once they are compatible, I guess we can just switch? We could still allow for user-patches in order to fill gaps until problems are resolved on GitHub's side, I've imlemented something similar in octokit/openapi. But we can look into that later

@WyriHaximus
Copy link
Contributor Author

This is awesome news! Will transition my code to it, @wolfy1339 thank you for the branch, it has helped me a lot so far <3!

@wolfy1339
Copy link
Member

wolfy1339 commented Nov 16, 2022

I created a script to convert from the OpenAPI format to JSON schema. This should help for comparing the 2 side-by-side, and finding inaccuracies

/**
 * @license MIT
 * @author: @wolfy1339
 * Simple script to convert the GitHub OpenAPI webhooks spec to JSON Schema in the same format as octokit/webhooks
 * Written in ES2022 using ES Modules
 */

import { writeFile } from 'node:fs/promises';
import prettier from 'prettier';

const oasWebhooks = await fetch(
  'https://raw.githubusercontent.com/github/rest-api-description/main/descriptions-next/api.github.com/api.github.com.json'
).then((res) => res.json());
const JSONSchema = {
  $schema: 'http://json-schema.org/draft-07/schema',
  definitions: {},
  oneOf: []
};

for (let name of Object.keys(oasWebhooks.webhooks)) {
  const defn = oasWebhooks.webhooks[name].post;
  // Get the name of the component holding the webhook event schema
  const $ref = defn.requestBody.content['application/json'].schema.$ref
    .split('/')
    .at(-1);
  const oasWebhookDefn = oasWebhooks.components.schemas[$ref];
  const [webhookEvent, action] = defn.operationId
    .replaceAll('-', '_')
    .split('/');
  // Create the proper id for the webhook event action (eg: `issue.opened`) schema
  const JSONSchemaName = `${webhookEvent}$${action || 'event'}`;

  // Create the id for the webhook event, eg: `issue`
  const webhookEvent$ref =
    typeof action !== 'undefined' ? `${webhookEvent}_event` : JSONSchemaName;
  // Add the webhook event (eg: issue) to the global `oneOf`
  JSONSchema.oneOf.push({ $ref: `#/definitions/${webhookEvent$ref}` });

  // Create the webhook event `oneOf` containing all the schemas for the different actions
  if (typeof action !== 'undefined') {
    JSONSchema.definitions[webhookEvent$ref] ??= {
      oneOf: []
    };
    JSONSchema.definitions[webhookEvent$ref].oneOf.push({
      $ref: `#/definitions/${JSONSchemaName}`
    });
  }

  JSONSchema.definitions[JSONSchemaName] = oasWebhookDefn;

  // Add the `$id` and `title` properties so the type generation script can guess the proper name for the generated interface
  JSONSchema.definitions[JSONSchemaName].title =
    typeof action !== 'undefined'
      ? `${webhookEvent} ${action} event`
      : `${webhookEvent} event`;
  JSONSchema.definitions[JSONSchemaName].$id = JSONSchemaName;
}

const uniqueIds = [];

JSONSchema.oneOf = JSONSchema.oneOf.filter((element) => {
  const isDuplicate = uniqueIds.includes(element.$ref);

  if (!isDuplicate) {
    uniqueIds.push(element.$ref);

    return true;
  }

  return false;
});

// Check all instances of `$ref` in the JSON Schema and replace them with the correct path, and add them to the definitions
const handleRefs = (obj) => {
  if (typeof obj !== 'object' || obj === null) return obj;

  for (let key in obj) {
    if (key === '$ref') {
      // Skip events
      if (obj[key].includes('$')) continue;
      const ref = obj[key].split('/').at(-1);
      JSONSchema.definitions[ref] = oasWebhooks.components.schemas[ref];
      // Call the function with the new definition to handle any of it's $refs
      handleRefs(JSONSchema.definitions[ref]);
      obj[key] = `#/definitions/${ref}`;
    } else {
      obj[key] = handleRefs(obj[key]);
    }
  }
  return obj;
};
// Check all $ref properties and include them in the output
handleRefs(JSONSchema.definitions);

await writeFile(
  'schema.json',
  prettier.format(JSON.stringify(JSONSchema), { parser: 'json' })
);

@gr2m
Copy link
Contributor

gr2m commented Nov 18, 2022

Amazing work @wolfy1339!!! Thank you so much for putting in all the work

@wolfy1339
Copy link
Member

I have created a repository that can publish webhooks types from the openapi spec

https://github.com/wolfy1339/openapi-webhooks

We could create something similar in the Octokit org, until openapi-ts/openapi-typescript#984 is resolved, then we could publish the whole OpenAPI 3.1 spec

@oscard0m
Copy link
Member

oscard0m commented Apr 7, 2023

I have created a repository that can publish webhooks types from the openapi spec

https://github.com/wolfy1339/openapi-webhooks

We could create something similar in the Octokit org, until drwpow/openapi-typescript#984 is resolved, then we could publish the whole OpenAPI 3.1 spec

The issue is resolved so I think we can move forward, right?

Btw @wolfy1339 congrats once again. You have done an amazing job here. I really appreciate all the work you are doing. Thank you 🙏🏽

Is there a way I can support you on this?

@wolfy1339
Copy link
Member

Finding differences between the OpenAPI spec, the JSON schemas in this repository and the actual webhooks received is greatly appreciated

@wolfy1339
Copy link
Member

The issue is resolved so I think we can move forward, right?

I don't believe this issue is fully resolved just yet. Once we have the OpenAPI webhook spec and types published under the Octokit org then this should be resolved

@wolfy1339
Copy link
Member

wolfy1339 commented Feb 18, 2024

The next major release of the @octokit/webhooks package will include OpenAPI specs for the webhooks.

Take a look at https://github.com/octokit/openapi-webhooks
Once that is released, this repository will be archived, and any issues can be patched over in that repo

@github-project-automation github-project-automation bot moved this from 🔥 Backlog to ✅ Done in 🧰 Octokit Active Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
Archived in project
Development

No branches or pull requests

5 participants