[Schema Inaccuracy] <webhook-pull-request-closed lack some properties>

[windqin]

Schema Inaccuracy

In https://github.com/github/rest-api-description/blob/main/descriptions/ghec/ghec.2022-11-28.json commit 6510f7e, webhook-pull-request-closed -> properties -> pull_request -> allOf[0] refers to #/components/schemas/pull-request, but this schema pull-request -> base -> repo lack some properties:

                                    "allow_auto_merge": {
                                        "default": false,
                                        "description": "Whether to allow auto-merge for pull requests.",
                                        "type": "boolean"
                                    },
                                    "allow_update_branch": {
                                        "description": "Whether to allow updating the pull request's branch.",
                                        "type": "boolean"
                                    },
                                    "delete_branch_on_merge": {
                                        "default": false,
                                        "description": "Whether to delete head branches when pull requests are merged.",
                                        "type": "boolean"
                                    },
                                    "merge_commit_message": {
                                        "description": "The default value for a merge commit message.\n- `PR_TITLE` - default to the pull request's title.\n- `PR_BODY` - default to the pull request's body.\n- `BLANK` - default to a blank commit message.",
                                        "enum": [
                                            "PR_BODY",
                                            "PR_TITLE",
                                            "BLANK"
                                        ],
                                        "maxLength": 1024,
                                        "type": "string"
                                    },
                                    "merge_commit_title": {
                                        "description": "The default value for a merge commit title.\n- `PR_TITLE` - default to the pull request's title.\n- `MERGE_MESSAGE` - default to the classic title for a merge message (e.g., \"Merge pull request #123 from branch-name\").",
                                        "enum": [
                                            "PR_TITLE",
                                            "MERGE_MESSAGE"
                                        ],
                                        "maxLength": 1024,
                                        "type": "string"
                                    },
                                    "squash_merge_commit_message": {
                                        "description": "The default value for a squash merge commit message:\n- `PR_BODY` - default to the pull request's body.\n- `COMMIT_MESSAGES` - default to the branch's commit messages.\n- `BLANK` - default to a blank commit message.",
                                        "enum": [
                                            "PR_BODY",
                                            "COMMIT_MESSAGES",
                                            "BLANK"
                                        ],
                                        "maxLength": 1024,
                                        "type": "string"
                                    },
                                    "squash_merge_commit_title": {
                                        "description": "The default value for a squash merge commit title:\n- `PR_TITLE` - default to the pull request's title.\n- `COMMIT_OR_PR_TITLE` - default to the commit's title (if only one commit) or the pull request's title (when more than one commit).",
                                        "enum": [
                                            "PR_TITLE",
                                            "COMMIT_OR_PR_TITLE"
                                        ],
                                        "maxLength": 1024,
                                        "type": "string"
                                    },
                                    "use_squash_pr_title_as_default": {
                                        "default": false,
                                        "description": "Whether a squash merge commit can use the pull request title as default. **This property has been deprecated. Please use `squash_merge_commit_title` instead.**",
                                        "type": "boolean"
                                    }

The current schema tries to use allOf under pull_request to define them, but they are under pull_request -> base -> repo, so the allOf[1] does not work.

Expected

Define the above properties under #/components/schemas/pull-request -> properties -> base -> properties -> repo -> properties

Reproduction Steps

In GHEC repo settings -> Webhooks -> Manage webhook, setup a webhook with "Pull requests" selected, close a pull request, then check the "Recent Deliveries" in the webhook setting page, you can see the webhook event pull_request.closed payload contains those properties under pull_request -> base -> repo.

[Lukeghenco]

👋 from PRs FR this week. We are not entirely sure what the true issue is here and what the ask is for this task.

cc @github/c2c-actions-first-responder as you were pinged on a similiar issue here: https://github.com/github/ecosystem-events/issues/2672

Wondering if you have found anything meaningful for actionable work here?

[windqin]

@Lukeghenco , the issue is:

Either the definition of components -> schemas -> pull_request -> properties -> base -> properties -> repo -> properties is wrong, it lacks above mentioned properties, as components -> schemas -> webhook-pull-request-closed -> properties -> pull_request refers to #/components/schemas/pull-request, but the real webhook payload contains those properties mentioned above.

Or the definition of components -> schemas -> webhook-pull-request-closed -> properties -> pull_request is wrong. If the pull_request under webhook-pull-request-closed is different with #/components/schemas/pull-request, please don't refer to it, but use a dedicate definition for webhook only.

[katiem0]

Hi @Lukeghenco Just wanting to elaborate what @windqin is referencing with an example for the schema in question.

I see that for webhook-pull-request-closed, the following properties are identified for the webhook event:

webhook-pull-request-closed

"webhook-pull-request-closed": {
        "title": "pull_request closed event",
        "type": "object",
        "properties": {
          "action": {
            "type": "string",
            "enum": [
              "closed"
            ]
          },
          "enterprise": {
            "$ref": "#/components/schemas/enterprise-webhooks"
          },
          "installation": {
            "$ref": "#/components/schemas/simple-installation"
          },
          "number": {
            "description": "The pull request number.",
            "type": "integer"
          },
          "organization": {
            "$ref": "#/components/schemas/organization-simple-webhooks"
          },
          "pull_request": {
            "allOf": [
              {
                "$ref": "#/components/schemas/pull-request"
              },
              {
                "type": "object",
                "properties": {
                  "allow_auto_merge": {
                    "description": "Whether to allow auto-merge for pull requests.",
                    "type": "boolean",
                    "default": false
                  },
                  "allow_update_branch": {
                    "description": "Whether to allow updating the pull request's branch.",
                    "type": "boolean"
                  },
                  "delete_branch_on_merge": {
                    "description": "Whether to delete head branches when pull requests are merged.",
                    "type": "boolean",
                    "default": false
                  },
                  "merge_commit_message": {
                    "description": "The default value for a merge commit message.\n- `PR_TITLE` - default to the pull request's title.\n- `PR_BODY` - default to the pull request's body.\n- `BLANK` - default to a blank commit message.",
                    "type": "string",
                    "enum": [
                      "PR_BODY",
                      "PR_TITLE",
                      "BLANK"
                    ]
                  },
                  "merge_commit_title": {
                    "description": "The default value for a merge commit title.\n- `PR_TITLE` - default to the pull request's title.\n- `MERGE_MESSAGE` - default to the classic title for a merge message (e.g., \"Merge pull request #123 from branch-name\").",
                    "type": "string",
                    "enum": [
                      "PR_TITLE",
                      "MERGE_MESSAGE"
                    ]
                  },
                  "squash_merge_commit_message": {
                    "description": "The default value for a squash merge commit message:\n- `PR_BODY` - default to the pull request's body.\n- `COMMIT_MESSAGES` - default to the branch's commit messages.\n- `BLANK` - default to a blank commit message.",
                    "type": "string",
                    "enum": [
                      "PR_BODY",
                      "COMMIT_MESSAGES",
                      "BLANK"
                    ]
                  },
                  "squash_merge_commit_title": {
                    "description": "The default value for a squash merge commit title:\n- `PR_TITLE` - default to the pull request's title.\n- `COMMIT_OR_PR_TITLE` - default to the commit's title (if only one commit) or the pull request's title (when more than one commit).",
                    "type": "string",
                    "enum": [
                      "PR_TITLE",
                      "COMMIT_OR_PR_TITLE"
                    ]
                  },
                  "use_squash_pr_title_as_default": {
                    "description": "Whether a squash merge commit can use the pull request title as default. **This property has been deprecated. Please use `squash_merge_commit_title` instead.**",
                    "type": "boolean",
                    "default": false
                  }
                }
              }
            ]
          },

Specifically, want to make note of

"pull_request": {
  "allOf": [
    {
      "$ref": "#/components/schemas/pull-request"
    },
    {
      "type": "object",
      "properties": {
        "allow_auto_merge": {
          "description": "Whether to allow auto-merge for pull requests.",
          "type": "boolean",
          "default": false
        },
...

Which is also supported in our docs that the fields outlined in the main part of this issue as far as the json schema, are not included in #/components/schemas/pull-request but are supplemental fields added to the pull_request.allOf[] object when the action is closed.

However, when the webhook payload event is sent, the fields (i.e. allow_auto_merge etc) are not direct properties of pull_request.allOf[1] but are instead nested in the pull-request.base.repo object, which technically should be then included in the pull_request.allOf[0] object:

Sample Webhook Payload

{
  "action": "closed",
  "pull_request": {
    "state": "closed",
    "locked": false,
    "title": "Bump actions/github-script from 6 to 7",
    "created_at": "2023-11-14T04:50:12Z",
    "updated_at": "2023-11-28T17:50:05Z",
    "closed_at": "2023-11-28T17:50:05Z",
    "merged_at": "2023-11-28T17:50:05Z",
    "base": {
      "label": "Avocado-Extra-Charge:main",
      "ref": "main",
      "repo": {
        ...
        "default_branch": "main",
        "allow_squash_merge": true,
        "allow_merge_commit": true,
        "allow_rebase_merge": true,
        "allow_auto_merge": false,
        "delete_branch_on_merge": false,
        "allow_update_branch": false,
        "use_squash_pr_title_as_default": false,
        "squash_merge_commit_message": "COMMIT_MESSAGES",
        "squash_merge_commit_title": "COMMIT_OR_PR_TITLE",
        "merge_commit_message": "PR_TITLE",
        "merge_commit_title": "MERGE_MESSAGE"
      }
    },
}

[skalnik]

I've gone ahead and thrown this on the Pull Request board to take care of!

[dzader]

A fix for this has been deployed