diff --git a/.github/workflows/linter.yml b/.github/workflows/linter.yml index 6ba9dde478b..f38bbbea515 100644 --- a/.github/workflows/linter.yml +++ b/.github/workflows/linter.yml @@ -44,7 +44,9 @@ jobs: go-version: stable cache-dependency-path: "**/go.sum" - name: Check OpenAPI - run: ./script/metadata.sh update-openapi --validate + run: | + ./script/metadata.sh update-openapi --validate + ./script/metadata.sh check-schema-fields env: CHECK_GITHUB_OPENAPI: 1 GITHUB_TOKEN: ${{ github.token }} diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3cdb417b766..0a68f4cb6b8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -652,6 +652,40 @@ Its subcommands are: - `unused` - lists operations from `openapi_operations.yaml` that are not mapped from any methods. +- `check-schema-fields` - checks Go struct JSON field optionality against + GitHub's OpenAPI schemas. A struct opts in by carrying one or more + `//meta:schema` annotations in its doc comment, each naming the operation + whose request or response body schema the struct must match: + + ```go + // IssueCommentRequest represents a request to create or update an issue comment. + // + //meta:schema request POST /repos/{owner}/{repo}/issues/{issue_number}/comments + //meta:schema request PATCH /repos/{owner}/{repo}/issues/comments/{comment_id} + type IssueCommentRequest struct { + ``` + + For every annotation the command verifies that required, non-nullable schema + fields are non-pointer fields without `omitempty` or `omitzero`, that optional + schema fields remain omittable, and that the field sets line up. Unannotated + structs are not checked, and an annotation that does not resolve to an + operation in the OpenAPI descriptions is itself reported as an issue. Run it + with: + + ```sh + script/metadata.sh check-schema-fields + ``` + + When adding a new request type (or converting one to pass by value), add a + `//meta:schema request ` line per operation that uses it as a + body, reusing the method's `//meta:operation` value. + + A few Go fields may intentionally deviate from the OpenAPI schema. These are + listed as `Struct.Field` entries in + `tools/metadata/schema_field_exceptions.yaml`, and their diagnostics are + suppressed; each is a known deviation to fix and remove over time. Update that + file (rather than the Go source) to add or remove an exception. + [OpenAPI descriptions of their API]: https://github.com/github/rest-api-description ## Scripts diff --git a/github/gists_comments.go b/github/gists_comments.go index d2e769604fa..e0bd6fcd4fb 100644 --- a/github/gists_comments.go +++ b/github/gists_comments.go @@ -24,12 +24,16 @@ func (g GistComment) String() string { } // CreateGistCommentRequest represents the input for creating a gist comment. +// +//meta:schema request POST /gists/{gist_id}/comments type CreateGistCommentRequest struct { // Body is the comment text. Body string `json:"body"` } // UpdateGistCommentRequest represents the input for updating a gist comment. +// +//meta:schema request PATCH /gists/{gist_id}/comments/{comment_id} type UpdateGistCommentRequest struct { // Body is the comment text. Body string `json:"body"` diff --git a/github/issues_comments.go b/github/issues_comments.go index 23bf0e1c7b3..10fc2d79acd 100644 --- a/github/issues_comments.go +++ b/github/issues_comments.go @@ -51,6 +51,9 @@ func (i IssueComment) String() string { } // IssueCommentRequest represents a request to create or update an issue comment. +// +//meta:schema request POST /repos/{owner}/{repo}/issues/{issue_number}/comments +//meta:schema request PATCH /repos/{owner}/{repo}/issues/comments/{comment_id} type IssueCommentRequest struct { Body string `json:"body"` } diff --git a/github/issues_milestones.go b/github/issues_milestones.go index d863f5c6ad8..9a98c2b514e 100644 --- a/github/issues_milestones.go +++ b/github/issues_milestones.go @@ -35,6 +35,8 @@ func (m Milestone) String() string { } // CreateMilestoneRequest represents a request to create a milestone. +// +//meta:schema request POST /repos/{owner}/{repo}/milestones type CreateMilestoneRequest struct { Title string `json:"title"` State *string `json:"state,omitempty"` @@ -43,6 +45,8 @@ type CreateMilestoneRequest struct { } // UpdateMilestoneRequest represents a request to update a milestone. +// +//meta:schema request PATCH /repos/{owner}/{repo}/milestones/{milestone_number} type UpdateMilestoneRequest struct { Title *string `json:"title,omitempty"` State *string `json:"state,omitempty"` diff --git a/github/orgs_custom_repository_roles.go b/github/orgs_custom_repository_roles.go index c98785398cc..6538c18448f 100644 --- a/github/orgs_custom_repository_roles.go +++ b/github/orgs_custom_repository_roles.go @@ -31,6 +31,8 @@ type CustomRepoRoles struct { } // CreateCustomRepoRoleRequest represents the parameters to create a custom repository role. +// +//meta:schema request POST /orgs/{org}/custom-repository-roles type CreateCustomRepoRoleRequest struct { Name string `json:"name"` Description *string `json:"description,omitempty"` @@ -39,6 +41,8 @@ type CreateCustomRepoRoleRequest struct { } // UpdateCustomRepoRoleRequest represents the parameters to update a custom repository role. +// +//meta:schema request PATCH /orgs/{org}/custom-repository-roles/{role_id} type UpdateCustomRepoRoleRequest struct { Name *string `json:"name,omitempty"` Description *string `json:"description,omitempty"` diff --git a/github/pulls_comments.go b/github/pulls_comments.go index 7bd2c970e96..9dec47d1da6 100644 --- a/github/pulls_comments.go +++ b/github/pulls_comments.go @@ -144,6 +144,8 @@ func (s *PullRequestsService) GetComment(ctx context.Context, owner, repo string // CreatePullRequestCommentRequest represents a request to create a review // comment on a pull request. +// +//meta:schema request POST /repos/{owner}/{repo}/pulls/{pull_number}/comments type CreatePullRequestCommentRequest struct { Body string `json:"body"` CommitID string `json:"commit_id"` @@ -160,6 +162,8 @@ type CreatePullRequestCommentRequest struct { // UpdatePullRequestCommentRequest represents a request to update a review // comment on a pull request. +// +//meta:schema request PATCH /repos/{owner}/{repo}/pulls/comments/{comment_id} type UpdatePullRequestCommentRequest struct { Body string `json:"body"` } diff --git a/github/pulls_reviews.go b/github/pulls_reviews.go index f901f32ce5e..106fe298273 100644 --- a/github/pulls_reviews.go +++ b/github/pulls_reviews.go @@ -94,6 +94,8 @@ func (r *PullRequestReviewRequest) isComfortFadePreview() (bool, error) { } // PullRequestDismissReviewRequest represents a request to dismiss a review. +// +//meta:schema request PUT /repos/{owner}/{repo}/pulls/{pull_number}/reviews/{review_id}/dismissals type PullRequestDismissReviewRequest struct { Message string `json:"message"` Event *string `json:"event,omitempty"` @@ -104,6 +106,8 @@ func (r PullRequestDismissReviewRequest) String() string { } // PullRequestSubmitReviewRequest represents a request to submit a review. +// +//meta:schema request POST /repos/{owner}/{repo}/pulls/{pull_number}/reviews/{review_id}/events type PullRequestSubmitReviewRequest struct { Body *string `json:"body,omitempty"` Event string `json:"event"` diff --git a/github/repos.go b/github/repos.go index 588b0c66fbe..f5d9c1eba06 100644 --- a/github/repos.go +++ b/github/repos.go @@ -619,6 +619,8 @@ func (s *RepositoriesService) Create(ctx context.Context, org string, repo *Repo } // TemplateRepoRequest represents a request to create a repository from a template. +// +//meta:schema request POST /repos/{template_owner}/{template_repo}/generate type TemplateRepoRequest struct { Name string `json:"name"` Owner *string `json:"owner,omitempty"` diff --git a/github/repos_autolinks.go b/github/repos_autolinks.go index c097f601504..1982a805370 100644 --- a/github/repos_autolinks.go +++ b/github/repos_autolinks.go @@ -11,6 +11,8 @@ import ( ) // CreateAutolinkRequest specifies parameters for RepositoriesService.CreateAutolink method. +// +//meta:schema request POST /repos/{owner}/{repo}/autolinks type CreateAutolinkRequest struct { KeyPrefix string `json:"key_prefix"` URLTemplate string `json:"url_template"` diff --git a/github/repos_deployment_branch_policies.go b/github/repos_deployment_branch_policies.go index 20d176c5926..13143aab872 100644 --- a/github/repos_deployment_branch_policies.go +++ b/github/repos_deployment_branch_policies.go @@ -25,12 +25,16 @@ type DeploymentBranchPolicyResponse struct { } // CreateDeploymentBranchPolicyRequest represents a request to create a deployment branch policy. +// +//meta:schema request POST /repos/{owner}/{repo}/environments/{environment_name}/deployment-branch-policies type CreateDeploymentBranchPolicyRequest struct { Name string `json:"name"` Type *string `json:"type,omitempty"` } // UpdateDeploymentBranchPolicyRequest represents a request to update a deployment branch policy. +// +//meta:schema request PUT /repos/{owner}/{repo}/environments/{environment_name}/deployment-branch-policies/{branch_policy_id} type UpdateDeploymentBranchPolicyRequest struct { Name string `json:"name"` } diff --git a/github/repos_keys.go b/github/repos_keys.go index 1b43379ae0d..521bf26aa12 100644 --- a/github/repos_keys.go +++ b/github/repos_keys.go @@ -61,6 +61,8 @@ func (s *RepositoriesService) GetKey(ctx context.Context, owner, repo string, id } // CreateDeployKeyRequest represents a request to create a deploy key. +// +//meta:schema request POST /repos/{owner}/{repo}/keys type CreateDeployKeyRequest struct { Title *string `json:"title,omitempty"` Key string `json:"key"` diff --git a/github/repos_merging.go b/github/repos_merging.go index a1979542574..13410ab4759 100644 --- a/github/repos_merging.go +++ b/github/repos_merging.go @@ -12,6 +12,8 @@ import ( // RepositoryMergeRequest represents a request to merge a branch in a // repository. +// +//meta:schema request POST /repos/{owner}/{repo}/merges type RepositoryMergeRequest struct { Base string `json:"base"` Head string `json:"head"` @@ -20,6 +22,8 @@ type RepositoryMergeRequest struct { // RepoMergeUpstreamRequest represents a request to sync a branch of // a forked repository to keep it up-to-date with the upstream repository. +// +//meta:schema request POST /repos/{owner}/{repo}/merge-upstream type RepoMergeUpstreamRequest struct { Branch string `json:"branch"` } diff --git a/github/repos_releases.go b/github/repos_releases.go index 3361a5d79d1..f474bd733a8 100644 --- a/github/repos_releases.go +++ b/github/repos_releases.go @@ -57,6 +57,8 @@ type RepositoryReleaseNotes struct { } // GenerateNotesRequest represents the request to generate release notes. +// +//meta:schema request POST /repos/{owner}/{repo}/releases/generate-notes type GenerateNotesRequest struct { TagName string `json:"tag_name"` PreviousTagName *string `json:"previous_tag_name,omitempty"` @@ -83,6 +85,8 @@ type ReleaseAsset struct { } // UpdateReleaseAssetRequest represents the request to update a release asset. +// +//meta:schema request PATCH /repos/{owner}/{repo}/releases/assets/{asset_id} type UpdateReleaseAssetRequest struct { Name *string `json:"name,omitempty"` Label *string `json:"label,omitempty"` @@ -185,6 +189,8 @@ func (s *RepositoriesService) getSingleRelease(ctx context.Context, url string) } // CreateReleaseRequest represents a request to create a release in a repository. +// +//meta:schema request POST /repos/{owner}/{repo}/releases type CreateReleaseRequest struct { TagName string `json:"tag_name"` TargetCommitish *string `json:"target_commitish,omitempty"` @@ -199,6 +205,8 @@ type CreateReleaseRequest struct { } // UpdateReleaseRequest represents a request to update a release in a repository. +// +//meta:schema request PATCH /repos/{owner}/{repo}/releases/{release_id} type UpdateReleaseRequest struct { TagName *string `json:"tag_name,omitempty"` TargetCommitish *string `json:"target_commitish,omitempty"` diff --git a/github/teams.go b/github/teams.go index 63ffbe6e1a3..8c3333f52ad 100644 --- a/github/teams.go +++ b/github/teams.go @@ -995,6 +995,8 @@ type ExternalGroupList struct { // UpdateConnectedExternalGroupRequest represents a request to update the connection // between an external group and a team. +// +//meta:schema request PATCH /orgs/{org}/teams/{team_slug}/external-groups type UpdateConnectedExternalGroupRequest struct { GroupID int64 `json:"group_id"` } diff --git a/github/users_keys.go b/github/users_keys.go index a7a54b44e16..a7a1eb30199 100644 --- a/github/users_keys.go +++ b/github/users_keys.go @@ -30,6 +30,8 @@ func (k Key) String() string { // CreateUserKeyRequest represents a request to create a public SSH key for the // authenticated user. +// +//meta:schema request POST /user/keys type CreateUserKeyRequest struct { Title *string `json:"title,omitempty"` Key string `json:"key"` diff --git a/github/users_ssh_signing_keys.go b/github/users_ssh_signing_keys.go index 8df9ac5dcd0..14480816cc7 100644 --- a/github/users_ssh_signing_keys.go +++ b/github/users_ssh_signing_keys.go @@ -24,6 +24,8 @@ func (k SSHSigningKey) String() string { // CreateSSHSigningKeyRequest represents a request to create an SSH signing key // for the authenticated user. +// +//meta:schema request POST /user/ssh_signing_keys type CreateSSHSigningKeyRequest struct { Title *string `json:"title,omitempty"` Key string `json:"key"` diff --git a/script/lint.sh b/script/lint.sh index ca4000647a8..599e0c6a0c9 100755 --- a/script/lint.sh +++ b/script/lint.sh @@ -1,7 +1,7 @@ #!/bin/sh #/ [ CHECK_GITHUB_OPENAPI=1 ] script/lint.sh runs linters and validates generated files. -#/ When CHECK_GITHUB is set, it validates that openapi_operations.yaml is consistent with the -#/ descriptions from github.com/github/rest-api-description. +#/ When CHECK_GITHUB_OPENAPI is set, it validates OpenAPI metadata and schema fields +#/ against descriptions from github.com/github/rest-api-description. set -e @@ -96,6 +96,14 @@ if [ -n "$CHECK_GITHUB_OPENAPI" ]; then printf "${RED}✘ openapi_operations.yaml validation failed${NC}\n" fail fi + + print_header "Validating OpenAPI schema fields" + if script/metadata.sh check-schema-fields; then + printf "${GREEN}✔ OpenAPI schema fields are valid${NC}\n" + else + printf "${RED}✘ OpenAPI schema field validation failed${NC}\n" + fail + fi fi print_header "Validating generated files" diff --git a/tools/metadata/main.go b/tools/metadata/main.go index 09de07cd9d5..52de5aa013f 100644 --- a/tools/metadata/main.go +++ b/tools/metadata/main.go @@ -35,9 +35,16 @@ Update go source code to be consistent with openapi_operations.yaml. "format_help": `Format white space in openapi_operations.yaml and sort its operations.`, "unused_help": `List operations in openapi_operations.yaml that aren't used by any service methods.`, + "check_schema_fields_help": ` +Check Go struct JSON field optionality against GitHub's OpenAPI schemas. Only structs whose doc comment +carries one or more "//meta:schema " annotations are checked; each +annotation names the operation whose request or response body schema the struct must match. An annotation +that does not resolve to an operation in the OpenAPI descriptions is itself reported as an issue. +`, - "working_dir_help": `Working directory. Should be the root of the go-github repository.`, - "openapi_ref_help": `Git ref to pull OpenAPI descriptions from.`, + "working_dir_help": `Working directory. Should be the root of the go-github repository.`, + "openapi_ref_help": `Git ref to pull OpenAPI descriptions from.`, + "openapi_ref_default_help": `Git ref to pull OpenAPI descriptions from. Defaults to openapi_commit from openapi_operations.yaml.`, "openapi_validate_help": ` Instead of updating, make sure that the operations in openapi_operations.yaml's "openapi_operations" field are @@ -54,6 +61,7 @@ type rootCmd struct { UpdateGo updateGoCmd `kong:"cmd,help=${update_go_help}"` Format formatCmd `kong:"cmd,help=${format_help}"` Unused unusedCmd `kong:"cmd,help=${unused_help}"` + CheckSchema checkSchemaCmd `kong:"cmd,name=check-schema-fields,help=${check_schema_fields_help}"` WorkingDir string `kong:"short=C,default=.,help=${working_dir_help}"` @@ -182,6 +190,62 @@ func (c *unusedCmd) Run(root *rootCmd, k *kong.Context) error { return nil } +type checkSchemaCmd struct { + Ref string `kong:"help=${openapi_ref_default_help}"` + Verbose bool `kong:"help='Print each checked annotation.'"` +} + +func (c *checkSchemaCmd) Run(root *rootCmd, k *kong.Context) error { + ctx := context.Background() + _, opsFile, err := root.opsFile() + if err != nil { + return err + } + ref := c.Ref + if ref == "" { + ref = opsFile.GitCommit + if ref == "" { + return errors.New("openapi_operations.yaml does not have an openapi_commit field") + } + } + + client, err := githubClient(root.GithubURL, root.UploadURL) + if err != nil { + return err + } + descriptions, err := getDescriptions(ctx, client, ref) + if err != nil { + return err + } + exceptions, err := loadSchemaFieldExceptions(root.WorkingDir) + if err != nil { + return err + } + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: descriptions, + githubDir: filepath.Join(root.WorkingDir, "github"), + exceptions: exceptions, + }) + if err != nil { + return err + } + + fmt.Fprintf(k.Stdout, "Found %v schema field issues\n", len(result.Diagnostics)) + fmt.Fprintf(k.Stdout, "Checked %v annotations on %v annotated structs\n", result.Summary.Checked, result.Summary.AnnotatedStructs) + if c.Verbose { + for _, checked := range result.Checked { + fmt.Fprintf(k.Stdout, "checked: %v -> %v\n", checked.GoStruct, checked.Annotation) + } + } + for _, diag := range result.Diagnostics { + fmt.Fprintln(k.Stdout, diag.String()) + } + if len(result.Diagnostics) > 0 { + return fmt.Errorf("found %v schema field issues", len(result.Diagnostics)) + } + return nil +} + func main() { err := run(os.Args[1:], nil) if err != nil { diff --git a/tools/metadata/schema_field_exceptions.yaml b/tools/metadata/schema_field_exceptions.yaml new file mode 100644 index 00000000000..55fa9db22ea --- /dev/null +++ b/tools/metadata/schema_field_exceptions.yaml @@ -0,0 +1,4 @@ +# The file lists "Struct.Field" entries whose JSON field optionality intentionally deviates from the OpenAPI schema, +# so that their check-schema-fields diagnostics are suppressed. Add an entry only for an annotated struct whose +# deviation is deliberate; unannotated structs are not checked at all. +exceptions: [] diff --git a/tools/metadata/schema_fields.go b/tools/metadata/schema_fields.go new file mode 100644 index 00000000000..092a60218ed --- /dev/null +++ b/tools/metadata/schema_fields.go @@ -0,0 +1,643 @@ +// Copyright 2026 The go-github AUTHORS. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "cmp" + "errors" + "fmt" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "maps" + "os" + "path/filepath" + "reflect" + "regexp" + "slices" + "strings" + + "github.com/getkin/kin-openapi/openapi3" + "go.yaml.in/yaml/v3" +) + +type schemaFieldCheckOptions struct { + descriptions []*openapiFile + githubDir string + // exceptions holds "Struct.Field" entries whose diagnostics are suppressed. It is loaded from + // schema_field_exceptions.yaml by the command; see loadSchemaFieldExceptions. + exceptions []string +} + +type schemaFieldCheckResult struct { + Summary schemaFieldCheckSummary + Checked []*schemaFieldChecked + Diagnostics []*schemaFieldDiagnostic +} + +type schemaFieldCheckSummary struct { + GoStructs int + AnnotatedStructs int + Checked int + Diagnostics int +} + +type schemaFieldChecked struct { + Annotation string + GoStruct string + OpenAPIFile string +} + +type schemaFieldDiagnostic struct { + Annotation string + GoStruct string + Field string + JSONName string + Message string + Filename string + Line int + OpenAPIFile string +} + +func (d schemaFieldDiagnostic) String() string { + loc := diagLocation(d.Filename, d.Line) + if loc != "" { + loc += ": " + } + source := "" + if d.OpenAPIFile != "" { + source = fmt.Sprintf(" [%v]", d.OpenAPIFile) + } + subject := d.GoStruct + if d.Field != "" { + subject += "." + d.Field + } + if d.JSONName != "" && d.Annotation != "" { + subject += fmt.Sprintf(" (%v from %v)", d.JSONName, d.Annotation) + } + return fmt.Sprintf("%v%v: %v%v", loc, subject, d.Message, source) +} + +func diagLocation(filename string, line int) string { + if filename == "" { + return "" + } + if line == 0 { + return filename + } + return fmt.Sprintf("%v:%v", filename, line) +} + +// schemaAnnotation is one "//meta:schema " line from a struct doc comment. It names +// the operation whose request or response body schema the annotated struct must match. +type schemaAnnotation struct { + role string // "request" or "response" + method string + path string + filename string + line int +} + +func (a schemaAnnotation) String() string { + return fmt.Sprintf("%v %v %v", a.role, a.method, a.path) +} + +// schemaAnnotationProblem is a malformed "//meta:schema" line that could not be parsed into a +// schemaAnnotation. +type schemaAnnotationProblem struct { + text string + message string + filename string + line int +} + +type goStructInfo struct { + name string + filename string + line int + fields map[string]goStructField + annotations []*schemaAnnotation + annotationProblems []*schemaAnnotationProblem +} + +type goStructField struct { + goStruct string + field string + jsonName string + hasOmitOption bool + isPointer bool + canBeOmitted bool + filename string + line int +} + +type openapiSchemaFields struct { + annotation string + openapiFile string + required []string + properties map[string]openapiSchemaProperty +} + +type openapiSchemaProperty struct { + nullable bool + readOnly bool + writeOnly bool +} + +type schemaFieldMatch struct { + schema *openapiSchemaFields + goStruct *goStructInfo +} + +// schemaFieldExceptionsFile is the on-disk format of schema_field_exceptions.yaml: a list of "Struct.Field" +// entries whose JSON field optionality intentionally deviates from the OpenAPI schema, so their diagnostics +// are suppressed. Each entry is a known deviation awaiting cleanup. +type schemaFieldExceptionsFile struct { + Exceptions []string `yaml:"exceptions"` +} + +// loadSchemaFieldExceptions reads the "Struct.Field" exception entries from the exceptions file under +// workingDir and returns them. A missing file yields no exceptions and no error, so callers do not need +// the file to exist. +func loadSchemaFieldExceptions(workingDir string) ([]string, error) { + filename := filepath.Join(workingDir, "tools/metadata/schema_field_exceptions.yaml") + b, err := os.ReadFile(filename) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil, nil + } + return nil, err + } + var exceptionsFile schemaFieldExceptionsFile + if err := yaml.Unmarshal(b, &exceptionsFile); err != nil { + return nil, fmt.Errorf("%v: %w", filename, err) + } + return exceptionsFile.Exceptions, nil +} + +// filterAllowedSchemaFieldDiagnostics removes diagnostics whose "Struct.Field" is listed in the exceptions. +func filterAllowedSchemaFieldDiagnostics(diagnostics []*schemaFieldDiagnostic, exceptions []string) []*schemaFieldDiagnostic { + var kept []*schemaFieldDiagnostic + for _, diag := range diagnostics { + if slices.Contains(exceptions, diag.GoStruct+"."+diag.Field) { + continue + } + kept = append(kept, diag) + } + return kept +} + +// checkSchemaFields validates every Go struct that carries at least one "//meta:schema" annotation against +// the OpenAPI schema of the annotated operation. Structs without annotations are not checked. +func checkSchemaFields(opts schemaFieldCheckOptions) (schemaFieldCheckResult, error) { + if len(opts.descriptions) == 0 { + return schemaFieldCheckResult{}, errors.New("no OpenAPI descriptions loaded") + } + + goStructs, err := collectGoStructs(opts.githubDir) + if err != nil { + return schemaFieldCheckResult{}, err + } + + var result schemaFieldCheckResult + for _, name := range slices.Sorted(maps.Keys(goStructs)) { + goStruct := goStructs[name] + if len(goStruct.annotations) == 0 && len(goStruct.annotationProblems) == 0 { + continue + } + result.Summary.AnnotatedStructs++ + + for _, problem := range goStruct.annotationProblems { + result.Diagnostics = append(result.Diagnostics, &schemaFieldDiagnostic{ + GoStruct: goStruct.name, + Message: fmt.Sprintf("invalid annotation %q: %v", problem.text, problem.message), + Filename: problem.filename, + Line: problem.line, + }) + } + + for _, ann := range goStruct.annotations { + schema, openapiFilename, problem := resolveSchemaAnnotation(opts.descriptions, ann) + if problem != "" { + result.Diagnostics = append(result.Diagnostics, &schemaFieldDiagnostic{ + Annotation: ann.String(), + GoStruct: goStruct.name, + Message: problem, + Filename: ann.filename, + Line: ann.line, + OpenAPIFile: openapiFilename, + }) + continue + } + + flat, reason, err := flattenObjectSchema(schema) + if err != nil { + return schemaFieldCheckResult{}, fmt.Errorf("%v %v: %w", goStruct.name, ann, err) + } + if reason != "" { + result.Diagnostics = append(result.Diagnostics, &schemaFieldDiagnostic{ + Annotation: ann.String(), + GoStruct: goStruct.name, + Message: "annotated schema cannot be checked: " + reason, + Filename: ann.filename, + Line: ann.line, + OpenAPIFile: openapiFilename, + }) + continue + } + + match := &schemaFieldMatch{ + schema: &openapiSchemaFields{ + annotation: ann.String(), + openapiFile: openapiFilename, + required: flat.Required, + properties: schemaProperties(flat.Properties), + }, + goStruct: goStruct, + } + result.Checked = append(result.Checked, &schemaFieldChecked{ + Annotation: ann.String(), + GoStruct: goStruct.name, + OpenAPIFile: openapiFilename, + }) + result.Diagnostics = append(result.Diagnostics, compareSchemaFields(match)...) + } + } + + result.Diagnostics = filterAllowedSchemaFieldDiagnostics(result.Diagnostics, opts.exceptions) + sortSchemaFieldResult(&result) + result.Summary.GoStructs = len(goStructs) + result.Summary.Checked = len(result.Checked) + result.Summary.Diagnostics = len(result.Diagnostics) + return result, nil +} + +func sortSchemaFieldResult(result *schemaFieldCheckResult) { + slices.SortFunc(result.Diagnostics, func(a, b *schemaFieldDiagnostic) int { + return cmp.Or( + cmp.Compare(a.GoStruct, b.GoStruct), + cmp.Compare(a.JSONName, b.JSONName), + cmp.Compare(a.Field, b.Field), + cmp.Compare(a.Annotation, b.Annotation), + cmp.Compare(a.OpenAPIFile, b.OpenAPIFile), + cmp.Compare(a.Message, b.Message), + ) + }) + slices.SortFunc(result.Checked, func(a, b *schemaFieldChecked) int { + return cmp.Or( + cmp.Compare(a.GoStruct, b.GoStruct), + cmp.Compare(a.Annotation, b.Annotation), + cmp.Compare(a.OpenAPIFile, b.OpenAPIFile), + ) + }) +} + +// resolveSchemaAnnotation finds the schema named by ann in the first OpenAPI description that documents the +// annotated operation, searching the descriptions in their load order (api.github.com first, then ghec, +// then ghes). It returns a non-empty problem string when the operation cannot be found or has no matching +// JSON schema, mirroring how unknown "//meta:operation" names are reported. +func resolveSchemaAnnotation(descriptions []*openapiFile, ann *schemaAnnotation) (schema *openapi3.Schema, openapiFilename, problem string) { + normPath := normalizeOpPath(ann.path) + for _, desc := range descriptions { + if desc.description == nil { + continue + } + for path, pathItem := range desc.description.Paths.Map() { + if pathItem == nil || normalizeOpPath(path) != normPath { + continue + } + op := pathItem.Operations()[ann.method] + if op == nil { + continue + } + schema, problem = annotationSchema(op, ann.role) + return schema, desc.filename, problem + } + } + return nil, "", fmt.Sprintf("could not find operation %v %v in any OpenAPI description", ann.method, ann.path) +} + +// annotationSchema extracts the request or response JSON schema from op according to role. +func annotationSchema(op *openapi3.Operation, role string) (*openapi3.Schema, string) { + switch role { + case "request": + if op.RequestBody == nil || op.RequestBody.Value == nil { + return nil, "operation has no request body" + } + return jsonContentSchema(op.RequestBody.Value.Content, "operation request body") + default: // "response"; parseSchemaAnnotations rejects other roles + if op.Responses == nil { + return nil, "operation has no responses" + } + responses := op.Responses.Map() + for _, code := range slices.Sorted(maps.Keys(responses)) { + if !strings.HasPrefix(code, "2") || responses[code] == nil || responses[code].Value == nil { + continue + } + if schema, problem := jsonContentSchema(responses[code].Value.Content, ""); problem == "" { + return schema, "" + } + } + return nil, "operation has no 2xx response with an application/json schema" + } +} + +// jsonContentSchema returns the application/json schema from content, or a problem string naming what is +// missing. +func jsonContentSchema(content openapi3.Content, what string) (*openapi3.Schema, string) { + mediaType := content.Get("application/json") + if mediaType == nil || mediaType.Schema == nil || mediaType.Schema.Value == nil { + return nil, what + " has no application/json schema" + } + return mediaType.Schema.Value, "" +} + +func flattenObjectSchema(schema *openapi3.Schema) (*openapi3.Schema, string, error) { + if schema == nil { + return nil, "", errors.New("schema is nil") + } + if hasUnsupportedComposition(schema) { + return nil, "schema uses oneOf, anyOf, or not", nil + } + if len(schema.AllOf) == 0 { + return schema, "", nil + } + + merged := &openapi3.Schema{ + Required: slices.Clone(schema.Required), + Properties: openapi3.Schemas{}, + } + maps.Copy(merged.Properties, schema.Properties) + + for _, ref := range schema.AllOf { + if ref == nil || ref.Value == nil { + return nil, "schema contains an unresolved allOf reference", nil + } + part, reason, err := flattenObjectSchema(ref.Value) + if err != nil || reason != "" { + return nil, reason, err + } + merged.Required = append(merged.Required, part.Required...) + maps.Copy(merged.Properties, part.Properties) + } + + return merged, "", nil +} + +func hasUnsupportedComposition(schema *openapi3.Schema) bool { + return len(schema.OneOf) > 0 || len(schema.AnyOf) > 0 || schema.Not != nil +} + +func schemaProperties(properties openapi3.Schemas) map[string]openapiSchemaProperty { + result := make(map[string]openapiSchemaProperty, len(properties)) + for name, propRef := range properties { + prop := openapiSchemaProperty{} + if propRef != nil && propRef.Value != nil { + prop.nullable = propRef.Value.Nullable + prop.readOnly = propRef.Value.ReadOnly + prop.writeOnly = propRef.Value.WriteOnly + } + result[name] = prop + } + return result +} + +func compareSchemaFields(match *schemaFieldMatch) []*schemaFieldDiagnostic { + var diagnostics []*schemaFieldDiagnostic + for jsonName, field := range match.goStruct.fields { + prop, inSchema := match.schema.properties[jsonName] + if !inSchema { + diagnostics = append(diagnostics, newSchemaFieldDiagnostic(match, field, "field is not present in the OpenAPI schema properties")) + continue + } + if !prop.canCheckOptionality() { + continue + } + + required := slices.Contains(match.schema.required, jsonName) + switch { + case required && !prop.nullable && field.isPointer: + diagnostics = append(diagnostics, newSchemaFieldDiagnostic(match, field, "field is required and non-nullable in the OpenAPI schema but is a pointer")) + case required && field.hasOmitOption: + diagnostics = append(diagnostics, newSchemaFieldDiagnostic(match, field, "field is required by the OpenAPI schema but has an omit option")) + case !required && !field.canBeOmitted: + diagnostics = append(diagnostics, newSchemaFieldDiagnostic(match, field, "field is optional in the OpenAPI schema but is not a pointer, slice, map, interface, or selector type")) + case !required && !field.hasOmitOption: + diagnostics = append(diagnostics, newSchemaFieldDiagnostic(match, field, `field is optional in the OpenAPI schema but is missing "omitempty" or "omitzero"`)) + } + } + + for propName, prop := range match.schema.properties { + if !prop.canCheckOptionality() { + continue + } + if _, ok := match.goStruct.fields[propName]; ok { + continue + } + diagnostics = append(diagnostics, &schemaFieldDiagnostic{ + Annotation: match.schema.annotation, + GoStruct: match.goStruct.name, + JSONName: propName, + Field: propName, + Message: "OpenAPI schema property is missing from the Go struct", + OpenAPIFile: match.schema.openapiFile, + }) + } + return diagnostics +} + +func (p openapiSchemaProperty) canCheckOptionality() bool { + return !p.readOnly && !p.writeOnly +} + +func newSchemaFieldDiagnostic(match *schemaFieldMatch, field goStructField, message string) *schemaFieldDiagnostic { + return &schemaFieldDiagnostic{ + Annotation: match.schema.annotation, + GoStruct: match.goStruct.name, + Field: field.field, + JSONName: field.jsonName, + Message: message, + Filename: field.filename, + Line: field.line, + OpenAPIFile: match.schema.openapiFile, + } +} + +// metaSchemaLineRe recognizes a "//meta:schema ..." doc comment line; the arguments are validated by +// parseSchemaAnnotations. +var metaSchemaLineRe = regexp.MustCompile(`(?i)^\s*//\s*meta:schema\b(.*)$`) + +// parseSchemaAnnotations extracts every "//meta:schema " line from doc. Malformed +// lines are returned as problems so they surface as diagnostics instead of being silently ignored. +func parseSchemaAnnotations(fset *token.FileSet, doc *ast.CommentGroup, filename string) ([]*schemaAnnotation, []*schemaAnnotationProblem) { + if doc == nil { + return nil, nil + } + var annotations []*schemaAnnotation + var problems []*schemaAnnotationProblem + for _, comment := range doc.List { + m := metaSchemaLineRe.FindStringSubmatch(comment.Text) + if m == nil { + continue + } + line := fset.Position(comment.Pos()).Line + args := strings.Fields(m[1]) + if len(args) != 3 { + problems = append(problems, &schemaAnnotationProblem{ + text: strings.TrimSpace(strings.TrimPrefix(comment.Text, "//")), + message: "want //meta:schema ", + filename: filename, + line: line, + }) + continue + } + role := strings.ToLower(args[0]) + if role != "request" && role != "response" { + problems = append(problems, &schemaAnnotationProblem{ + text: strings.TrimSpace(strings.TrimPrefix(comment.Text, "//")), + message: fmt.Sprintf("unknown role %q; want request or response", args[0]), + filename: filename, + line: line, + }) + continue + } + annotations = append(annotations, &schemaAnnotation{ + role: role, + method: strings.ToUpper(args[1]), + path: args[2], + filename: filename, + line: line, + }) + } + return annotations, problems +} + +// collectGoStructs parses the Go source files in dir and returns every exported struct by name, along with +// any "//meta:schema" annotations found in the struct doc comments. +func collectGoStructs(dir string) (map[string]*goStructInfo, error) { + structs := map[string]*goStructInfo{} + fset := token.NewFileSet() + err := filepath.WalkDir(dir, func(filename string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + if !strings.HasSuffix(filename, ".go") || strings.HasSuffix(filename, "_test.go") { + return nil + } + + fileNode, err := parser.ParseFile(fset, filename, nil, parser.ParseComments|parser.SkipObjectResolution) + if err != nil { + return err + } + for _, decl := range fileNode.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok || gen.Tok != token.TYPE { + continue + } + for _, spec := range gen.Specs { + typeSpec, ok := spec.(*ast.TypeSpec) + if !ok || !typeSpec.Name.IsExported() { + continue + } + structType, ok := typeSpec.Type.(*ast.StructType) + if !ok { + continue + } + doc := typeSpec.Doc + if doc == nil && len(gen.Specs) == 1 { + doc = gen.Doc + } + annotations, problems := parseSchemaAnnotations(fset, doc, filename) + structs[typeSpec.Name.Name] = &goStructInfo{ + name: typeSpec.Name.Name, + filename: filename, + line: fset.Position(typeSpec.Name.Pos()).Line, + fields: collectFieldsForStruct(fset, filename, typeSpec.Name.Name, structType), + annotations: annotations, + annotationProblems: problems, + } + } + } + return nil + }) + if err != nil { + return nil, err + } + return structs, nil +} + +func collectFieldsForStruct(fset *token.FileSet, filename, structName string, structType *ast.StructType) map[string]goStructField { + fields := map[string]goStructField{} + for _, field := range structType.Fields.List { + if len(field.Names) == 0 { + continue + } + for _, name := range field.Names { + if !name.IsExported() { + continue + } + info := goStructField{ + goStruct: structName, + field: name.Name, + jsonName: name.Name, + isPointer: isPointerType(field.Type), + canBeOmitted: canBeOmitted(field.Type), + filename: filename, + line: fset.Position(name.Pos()).Line, + } + if field.Tag != nil { + tag := reflect.StructTag(strings.Trim(field.Tag.Value, "`")) + jsonName, hasOmitOption, ignored := parseJSONTag(tag.Get("json")) + if ignored { + continue + } + if jsonName != "" { + info.jsonName = jsonName + } + info.hasOmitOption = hasOmitOption + } + if info.jsonName == "" { + continue + } + fields[info.jsonName] = info + } + } + return fields +} + +func parseJSONTag(tag string) (name string, hasOmitOption, ignored bool) { + if tag == "" { + return "", false, false + } + parts := strings.Split(tag, ",") + name = parts[0] + if name == "-" { + return "", false, true + } + for _, opt := range parts[1:] { + if opt == "omitempty" || opt == "omitzero" { + hasOmitOption = true + } + } + return name, hasOmitOption, false +} + +func isPointerType(expr ast.Expr) bool { + _, ok := expr.(*ast.StarExpr) + return ok +} + +func canBeOmitted(expr ast.Expr) bool { + switch expr.(type) { + case *ast.StarExpr, *ast.ArrayType, *ast.MapType, *ast.InterfaceType, *ast.SelectorExpr: + return true + } + if ident, ok := expr.(*ast.Ident); ok && ident.Name == "any" { + return true + } + return false +} diff --git a/tools/metadata/schema_fields_test.go b/tools/metadata/schema_fields_test.go new file mode 100644 index 00000000000..1e3d88c3159 --- /dev/null +++ b/tools/metadata/schema_fields_test.go @@ -0,0 +1,717 @@ +// Copyright 2026 The go-github AUTHORS. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "go/parser" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/getkin/kin-openapi/openapi3" + "github.com/google/go-cmp/cmp" +) + +func TestCheckSchemaFieldsAnnotatedStruct(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +import "encoding/json" + +// Demo is a demo request body. +// +//meta:schema request POST /demo +type Demo struct { + ID *int64 `+"`json:\"id,omitempty\"`"+` + Name string `+"`json:\"name\"`"+` + Note string `+"`json:\"note\"`"+` + Items []string `+"`json:\"items\"`"+` + Metadata map[string]string `+"`json:\"metadata,omitempty\"`"+` + Raw json.RawMessage `+"`json:\"raw,omitempty\"`"+` + Extra *string `+"`json:\"extra,omitempty\"`"+` + Internal *string `+"`json:\"-\"`"+` +} +`) + + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{ + testOpenAPIFile("descriptions/api.github.com/api.github.com.json", + "/demo", &openapi3.PathItem{Post: testRequestBodyOperation(&openapi3.Schema{ + Required: []string{"id", "name", "items", "metadata"}, + Properties: openapi3.Schemas{ + "id": openapi3.NewSchemaRef("", openapi3.NewIntegerSchema()), + "name": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), + "note": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), + "items": openapi3.NewSchemaRef("", openapi3.NewArraySchema()), + "metadata": openapi3.NewSchemaRef("", openapi3.NewObjectSchema()), + "raw": openapi3.NewSchemaRef("", openapi3.NewObjectSchema()), + "missing": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), + }, + })}), + }, + githubDir: githubDir, + }) + if err != nil { + t.Fatal(err) + } + + if diff := cmp.Diff([]*schemaFieldChecked{{ + Annotation: "request POST /demo", + GoStruct: "Demo", + OpenAPIFile: "descriptions/api.github.com/api.github.com.json", + }}, result.Checked); diff != "" { + t.Errorf("checked mismatch (-want +got):\n%v", diff) + } + + var got []string + for _, diag := range result.Diagnostics { + got = append(got, diag.JSONName+": "+diag.Message) + } + want := []string{ + "extra: field is not present in the OpenAPI schema properties", + "id: field is required and non-nullable in the OpenAPI schema but is a pointer", + "metadata: field is required by the OpenAPI schema but has an omit option", + "missing: OpenAPI schema property is missing from the Go struct", + "note: field is optional in the OpenAPI schema but is not a pointer, slice, map, interface, or selector type", + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("diagnostics mismatch (-want +got):\n%v", diff) + } +} + +func TestCheckSchemaFieldsMultipleAnnotations(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +// DemoRequest is used by two operations. +// +//meta:schema request POST /repos/{owner}/{repo}/demos +//meta:schema request PATCH /repos/{owner}/{repo}/demos/{demo_id} +type DemoRequest struct { + Body string `+"`json:\"body\"`"+` +} +`) + + bodySchema := func() *openapi3.Schema { + return &openapi3.Schema{ + Required: []string{"body"}, + Properties: openapi3.Schemas{"body": openapi3.NewSchemaRef("", openapi3.NewStringSchema())}, + } + } + descriptions := []*openapiFile{ + testOpenAPIFile("descriptions/api.github.com/api.github.com.json", + "/repos/{owner}/{repo}/demos", &openapi3.PathItem{Post: testRequestBodyOperation(bodySchema())}), + testOpenAPIFile("descriptions/ghec/ghec.json", + "/repos/{owner}/{repo}/demos/{demo_id}", &openapi3.PathItem{Patch: testRequestBodyOperation(bodySchema())}), + } + + result, err := checkSchemaFields(schemaFieldCheckOptions{descriptions: descriptions, githubDir: githubDir}) + if err != nil { + t.Fatal(err) + } + + if diff := cmp.Diff([]*schemaFieldChecked{ + {Annotation: "request PATCH /repos/{owner}/{repo}/demos/{demo_id}", GoStruct: "DemoRequest", OpenAPIFile: "descriptions/ghec/ghec.json"}, + {Annotation: "request POST /repos/{owner}/{repo}/demos", GoStruct: "DemoRequest", OpenAPIFile: "descriptions/api.github.com/api.github.com.json"}, + }, result.Checked); diff != "" { + t.Errorf("checked mismatch (-want +got):\n%v", diff) + } + if len(result.Diagnostics) != 0 { + t.Errorf("diagnostics = %v, want none", result.Diagnostics) + } + if result.Summary.AnnotatedStructs != 1 { + t.Errorf("AnnotatedStructs = %v, want 1", result.Summary.AnnotatedStructs) + } +} + +func TestCheckSchemaFieldsUnresolvedAnnotation(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +//meta:schema request POST /missing +type Demo struct { + Body string `+"`json:\"body\"`"+` +} +`) + + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", + "/demo", &openapi3.PathItem{Post: testRequestBodyOperation(openapi3.NewObjectSchema())})}, + githubDir: githubDir, + }) + if err != nil { + t.Fatal(err) + } + if len(result.Diagnostics) != 1 { + t.Fatalf("diagnostics = %v, want one", result.Diagnostics) + } + if got, want := result.Diagnostics[0].Message, "could not find operation POST /missing in any OpenAPI description"; got != want { + t.Errorf("message = %q, want %q", got, want) + } +} + +func TestCheckSchemaFieldsInvalidAnnotation(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +//meta:schema POST /demo +type Demo struct{} + +//meta:schema body POST /demo +type Demo2 struct{} +`) + + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", + "/demo", &openapi3.PathItem{Post: testRequestBodyOperation(openapi3.NewObjectSchema())})}, + githubDir: githubDir, + }) + if err != nil { + t.Fatal(err) + } + + var got []string + for _, diag := range result.Diagnostics { + got = append(got, diag.GoStruct+": "+diag.Message) + } + want := []string{ + `Demo: invalid annotation "meta:schema POST /demo": want //meta:schema `, + `Demo2: invalid annotation "meta:schema body POST /demo": unknown role "body"; want request or response`, + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("diagnostics mismatch (-want +got):\n%v", diff) + } +} + +func TestCheckSchemaFieldsResponseRole(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +//meta:schema response GET /demo +type Demo struct { + ID int64 `+"`json:\"id\"`"+` +} +`) + + op := &openapi3.Operation{Responses: openapi3.NewResponses(openapi3.WithStatus(200, &openapi3.ResponseRef{ + Value: &openapi3.Response{Content: openapi3.NewContentWithJSONSchema(&openapi3.Schema{ + Required: []string{"id"}, + Properties: openapi3.Schemas{"id": openapi3.NewSchemaRef("", openapi3.NewIntegerSchema())}, + })}, + }))} + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", + "/demo", &openapi3.PathItem{Get: op})}, + githubDir: githubDir, + }) + if err != nil { + t.Fatal(err) + } + if len(result.Checked) != 1 || len(result.Diagnostics) != 0 { + t.Errorf("checked = %v, diagnostics = %v; want one check and no diagnostics", result.Checked, result.Diagnostics) + } +} + +func TestCheckSchemaFieldsUnsupportedComposition(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +//meta:schema request POST /demo +type Demo struct{} +`) + + oneOf := &openapi3.Schema{OneOf: openapi3.SchemaRefs{openapi3.NewSchemaRef("", openapi3.NewStringSchema())}} + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", + "/demo", &openapi3.PathItem{Post: testRequestBodyOperation(oneOf)})}, + githubDir: githubDir, + }) + if err != nil { + t.Fatal(err) + } + if len(result.Diagnostics) != 1 { + t.Fatalf("diagnostics = %v, want one", result.Diagnostics) + } + if !strings.Contains(result.Diagnostics[0].Message, "annotated schema cannot be checked") { + t.Errorf("message = %q, want an unsupported-composition diagnostic", result.Diagnostics[0].Message) + } +} + +func TestCheckSchemaFieldsIgnoresUnannotatedStructs(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +// Unannotated has fields that would fail the check if it were annotated. +type Unannotated struct { + Name *string `+"`json:\"name\"`"+` +} +`) + + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", + "/demo", &openapi3.PathItem{Post: testRequestBodyOperation(openapi3.NewObjectSchema())})}, + githubDir: githubDir, + }) + if err != nil { + t.Fatal(err) + } + if len(result.Checked) != 0 || len(result.Diagnostics) != 0 || result.Summary.AnnotatedStructs != 0 { + t.Errorf("result = %+v, want nothing checked for an unannotated struct", result) + } +} + +func TestCheckSchemaFieldsAllowsRequiredNullablePointer(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +//meta:schema request POST /demo +type Demo struct { + Name *string `+"`json:\"name\"`"+` +} +`) + + nullable := openapi3.NewStringSchema() + nullable.Nullable = true + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", + "/demo", &openapi3.PathItem{Post: testRequestBodyOperation(&openapi3.Schema{ + Required: []string{"name"}, + Properties: openapi3.Schemas{"name": openapi3.NewSchemaRef("", nullable)}, + })})}, + githubDir: githubDir, + }) + if err != nil { + t.Fatal(err) + } + if len(result.Diagnostics) != 0 { + t.Errorf("diagnostics = %v, want none for a required nullable pointer field", result.Diagnostics) + } +} + +func TestResolveSchemaAnnotation(t *testing.T) { + t.Parallel() + schema := &openapi3.Schema{Properties: openapi3.Schemas{"body": openapi3.NewSchemaRef("", openapi3.NewStringSchema())}} + descriptions := []*openapiFile{ + testOpenAPIFile("descriptions/api.github.com/api.github.com.json", + "/repos/{owner}/{repo}/demos", &openapi3.PathItem{Post: testRequestBodyOperation(schema)}), + testOpenAPIFile("descriptions/ghes-3.21/ghes-3.21.json", + "/admin/demos", &openapi3.PathItem{Post: testRequestBodyOperation(schema)}), + } + + t.Run("path parameter names are normalized", func(t *testing.T) { + t.Parallel() + got, file, problem := resolveSchemaAnnotation(descriptions, &schemaAnnotation{role: "request", method: "POST", path: "/repos/{o}/{r}/demos"}) + if got == nil || problem != "" || file != "descriptions/api.github.com/api.github.com.json" { + t.Errorf("resolveSchemaAnnotation = (%v, %q, %q), want the api.github.com schema", got, file, problem) + } + }) + + t.Run("operation only in a later description is found", func(t *testing.T) { + t.Parallel() + got, file, problem := resolveSchemaAnnotation(descriptions, &schemaAnnotation{role: "request", method: "POST", path: "/admin/demos"}) + if got == nil || problem != "" || file != "descriptions/ghes-3.21/ghes-3.21.json" { + t.Errorf("resolveSchemaAnnotation = (%v, %q, %q), want the ghes schema", got, file, problem) + } + }) + + t.Run("wrong method is not found", func(t *testing.T) { + t.Parallel() + got, _, problem := resolveSchemaAnnotation(descriptions, &schemaAnnotation{role: "request", method: "PATCH", path: "/repos/{owner}/{repo}/demos"}) + if got != nil || !strings.Contains(problem, "could not find operation") { + t.Errorf("resolveSchemaAnnotation = (%v, %q), want a not-found problem", got, problem) + } + }) + + t.Run("missing request body is a problem", func(t *testing.T) { + t.Parallel() + noBody := []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", + "/demo", &openapi3.PathItem{Post: &openapi3.Operation{}})} + got, _, problem := resolveSchemaAnnotation(noBody, &schemaAnnotation{role: "request", method: "POST", path: "/demo"}) + if got != nil || problem != "operation has no request body" { + t.Errorf("resolveSchemaAnnotation = (%v, %q), want a no-request-body problem", got, problem) + } + }) +} + +func TestParseSchemaAnnotationsViaCollectGoStructs(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +// Demo has annotations in mixed case with surrounding doc text. +// +//meta:schema Request post /demo +//meta:schema response GET /demo +type Demo struct{} + +type Group struct{} +`) + + structs, err := collectGoStructs(githubDir) + if err != nil { + t.Fatal(err) + } + + demo := structs["Demo"] + if demo == nil { + t.Fatal("Demo struct not collected") + } + var got []string + for _, ann := range demo.annotations { + got = append(got, ann.String()) + } + want := []string{"request POST /demo", "response GET /demo"} + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("annotations mismatch (-want +got):\n%v", diff) + } + if len(demo.annotationProblems) != 0 { + t.Errorf("annotationProblems = %v, want none", demo.annotationProblems) + } + if group := structs["Group"]; group == nil || len(group.annotations) != 0 { + t.Errorf("Group = %+v, want collected with no annotations", group) + } +} + +func TestParseJSONTag(t *testing.T) { + t.Parallel() + tests := []struct { + name string + tag string + wantName string + wantOmit bool + wantIgnored bool + }{ + {name: "name only", tag: "name", wantName: "name"}, + {name: "omitempty", tag: "name,omitempty", wantName: "name", wantOmit: true}, + {name: "omitzero", tag: "id,omitzero", wantName: "id", wantOmit: true}, + {name: "ignored", tag: "-", wantIgnored: true}, + {name: "empty", tag: ""}, + {name: "empty name with omit", tag: ",omitempty", wantOmit: true}, + {name: "extra options", tag: "name,omitempty,string", wantName: "name", wantOmit: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + name, omit, ignored := parseJSONTag(tt.tag) + if name != tt.wantName || omit != tt.wantOmit || ignored != tt.wantIgnored { + t.Errorf("parseJSONTag(%q) = (%q, %v, %v), want (%q, %v, %v)", + tt.tag, name, omit, ignored, tt.wantName, tt.wantOmit, tt.wantIgnored) + } + }) + } +} + +func TestIsPointerTypeAndCanBeOmitted(t *testing.T) { + t.Parallel() + tests := []struct { + expr string + wantPointer bool + wantOmittable bool + }{ + {expr: "*int", wantPointer: true, wantOmittable: true}, + {expr: "[]string", wantPointer: false, wantOmittable: true}, + {expr: "map[string]int", wantPointer: false, wantOmittable: true}, + {expr: "interface{}", wantPointer: false, wantOmittable: true}, + {expr: "any", wantPointer: false, wantOmittable: true}, + {expr: "pkg.Type", wantPointer: false, wantOmittable: true}, + {expr: "string", wantPointer: false, wantOmittable: false}, + {expr: "int", wantPointer: false, wantOmittable: false}, + } + for _, tt := range tests { + t.Run(tt.expr, func(t *testing.T) { + t.Parallel() + e, err := parser.ParseExpr(tt.expr) + if err != nil { + t.Fatalf("ParseExpr(%q): %v", tt.expr, err) + } + if got := isPointerType(e); got != tt.wantPointer { + t.Errorf("isPointerType(%q) = %v, want %v", tt.expr, got, tt.wantPointer) + } + if got := canBeOmitted(e); got != tt.wantOmittable { + t.Errorf("canBeOmitted(%q) = %v, want %v", tt.expr, got, tt.wantOmittable) + } + }) + } +} + +func TestDiagLocation(t *testing.T) { + t.Parallel() + tests := []struct { + filename string + line int + want string + }{ + {filename: "", line: 0, want: ""}, + {filename: "f.go", line: 0, want: "f.go"}, + {filename: "f.go", line: 12, want: "f.go:12"}, + } + for _, tt := range tests { + if got := diagLocation(tt.filename, tt.line); got != tt.want { + t.Errorf("diagLocation(%q, %v) = %q, want %q", tt.filename, tt.line, got, tt.want) + } + } +} + +func TestSchemaFieldDiagnosticString(t *testing.T) { + t.Parallel() + withLoc := schemaFieldDiagnostic{ + Annotation: "request POST /demo", GoStruct: "S", Field: "F", JSONName: "j", + Message: "msg", Filename: "f.go", Line: 3, OpenAPIFile: "api.json", + } + if got, want := withLoc.String(), "f.go:3: S.F (j from request POST /demo): msg [api.json]"; got != want { + t.Errorf("String() = %q, want %q", got, want) + } + noLoc := schemaFieldDiagnostic{ + Annotation: "request POST /demo", GoStruct: "S", Field: "F", JSONName: "j", Message: "msg", + } + if got, want := noLoc.String(), "S.F (j from request POST /demo): msg"; got != want { + t.Errorf("String() = %q, want %q", got, want) + } + annotationLevel := schemaFieldDiagnostic{ + GoStruct: "S", Message: "msg", Filename: "f.go", Line: 3, + } + if got, want := annotationLevel.String(), "f.go:3: S: msg"; got != want { + t.Errorf("String() = %q, want %q", got, want) + } +} + +func TestCanCheckOptionality(t *testing.T) { + t.Parallel() + tests := []struct { + name string + prop openapiSchemaProperty + want bool + }{ + {name: "plain", prop: openapiSchemaProperty{}, want: true}, + {name: "readOnly", prop: openapiSchemaProperty{readOnly: true}, want: false}, + {name: "writeOnly", prop: openapiSchemaProperty{writeOnly: true}, want: false}, + } + for _, tt := range tests { + if got := tt.prop.canCheckOptionality(); got != tt.want { + t.Errorf("%v: canCheckOptionality() = %v, want %v", tt.name, got, tt.want) + } + } +} + +func TestHasUnsupportedComposition(t *testing.T) { + t.Parallel() + str := openapi3.NewSchemaRef("", openapi3.NewStringSchema()) + tests := []struct { + name string + schema *openapi3.Schema + want bool + }{ + {name: "plain object", schema: openapi3.NewObjectSchema(), want: false}, + {name: "oneOf", schema: &openapi3.Schema{OneOf: openapi3.SchemaRefs{str}}, want: true}, + {name: "anyOf", schema: &openapi3.Schema{AnyOf: openapi3.SchemaRefs{str}}, want: true}, + {name: "not", schema: &openapi3.Schema{Not: str}, want: true}, + } + for _, tt := range tests { + if got := hasUnsupportedComposition(tt.schema); got != tt.want { + t.Errorf("%v: hasUnsupportedComposition = %v, want %v", tt.name, got, tt.want) + } + } +} + +func TestFlattenObjectSchema(t *testing.T) { + t.Parallel() + + t.Run("plain object is returned unchanged", func(t *testing.T) { + t.Parallel() + obj := openapi3.NewObjectSchema() + obj.Required = []string{"a"} + got, reason, err := flattenObjectSchema(obj) + if err != nil || reason != "" { + t.Fatalf("flattenObjectSchema = (_, %q, %v)", reason, err) + } + if got != obj { + t.Error("flattenObjectSchema returned a different schema for a plain object") + } + }) + + t.Run("unsupported composition is skipped with a reason", func(t *testing.T) { + t.Parallel() + schema := &openapi3.Schema{OneOf: openapi3.SchemaRefs{openapi3.NewSchemaRef("", openapi3.NewStringSchema())}} + got, reason, err := flattenObjectSchema(schema) + if err != nil || got != nil || reason == "" { + t.Fatalf("flattenObjectSchema = (%v, %q, %v), want (nil, non-empty reason, nil)", got, reason, err) + } + }) + + t.Run("allOf is merged", func(t *testing.T) { + t.Parallel() + part := &openapi3.Schema{ + Required: []string{"b"}, + Properties: openapi3.Schemas{"b": openapi3.NewSchemaRef("", openapi3.NewStringSchema())}, + } + base := &openapi3.Schema{ + Required: []string{"a"}, + Properties: openapi3.Schemas{"a": openapi3.NewSchemaRef("", openapi3.NewStringSchema())}, + AllOf: openapi3.SchemaRefs{openapi3.NewSchemaRef("", part)}, + } + got, reason, err := flattenObjectSchema(base) + if err != nil || reason != "" { + t.Fatalf("flattenObjectSchema = (_, %q, %v)", reason, err) + } + if _, ok := got.Properties["a"]; !ok { + t.Error("merged schema missing property a") + } + if _, ok := got.Properties["b"]; !ok { + t.Error("merged schema missing property b") + } + if len(got.Required) != 2 { + t.Errorf("merged Required = %v, want a and b", got.Required) + } + }) + + t.Run("nil schema is an error", func(t *testing.T) { + t.Parallel() + if _, _, err := flattenObjectSchema(nil); err == nil { + t.Error("flattenObjectSchema(nil) = nil error, want error") + } + }) +} + +func TestSchemaProperties(t *testing.T) { + t.Parallel() + nullable := openapi3.NewStringSchema() + nullable.Nullable = true + readOnly := openapi3.NewStringSchema() + readOnly.ReadOnly = true + got := schemaProperties(openapi3.Schemas{ + "n": openapi3.NewSchemaRef("", nullable), + "r": openapi3.NewSchemaRef("", readOnly), + "nil": nil, + }) + if !got["n"].nullable { + t.Error("property n should be nullable") + } + if !got["r"].readOnly { + t.Error("property r should be readOnly") + } + if _, ok := got["nil"]; !ok { + t.Error("nil property ref should still yield a zero-value entry") + } +} + +//nolint:paralleltest // cannot use t.Parallel() when helper calls t.Setenv +func TestCheckSchemaFieldsCommand(t *testing.T) { + testServer := newTestServer(t, "schema-ref", map[string]any{ + "api.github.com/api.github.com.json": openapi3.T{ + Paths: openapi3.NewPaths(openapi3.WithPath("/demo", &openapi3.PathItem{ + Post: testRequestBodyOperation(&openapi3.Schema{ + Required: []string{"id", "name"}, + Properties: openapi3.Schemas{ + "id": openapi3.NewSchemaRef("", openapi3.NewIntegerSchema()), + "name": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), + "note": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), + }, + }), + })), + }, + }) + + res := runTest(t, "testdata/check-schema-fields", "check-schema-fields", "--github-url", testServer.URL) + res.assertOutput("Found 0 schema field issues\nChecked 1 annotations on 1 annotated structs", "") + res.assertNoErr() + res.checkGolden() +} + +func TestFilterAllowedSchemaFieldDiagnostics(t *testing.T) { + t.Parallel() + exceptions := []string{"ExemptStruct.ExemptField"} + got := filterAllowedSchemaFieldDiagnostics([]*schemaFieldDiagnostic{ + {GoStruct: "ExemptStruct", Field: "ExemptField"}, + {GoStruct: "NotExemptStruct", Field: "NotExemptField"}, + }, exceptions) + if len(got) != 1 || got[0].GoStruct != "NotExemptStruct" { + t.Errorf("filterAllowedSchemaFieldDiagnostics = %+v, want only NotExemptStruct.NotExemptField", got) + } +} + +func TestLoadSchemaFieldExceptions(t *testing.T) { + t.Parallel() + dir := t.TempDir() + metadataDir := filepath.Join(dir, "tools", "metadata") + if err := os.MkdirAll(metadataDir, 0o700); err != nil { + t.Fatal(err) + } + writeFile(t, filepath.Join(metadataDir, "schema_field_exceptions.yaml"), `# comment +exceptions: + - StructA.FieldA + - StructB.FieldB # TODO: fix +`) + + got, err := loadSchemaFieldExceptions(dir) + if err != nil { + t.Fatal(err) + } + want := []string{"StructA.FieldA", "StructB.FieldB"} + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("loadSchemaFieldExceptions mismatch (-want +got):\n%v", diff) + } + + // A missing file yields no exceptions and no error. + got, err = loadSchemaFieldExceptions(t.TempDir()) + if err != nil { + t.Fatalf("missing file: unexpected error %v", err) + } + if len(got) != 0 { + t.Errorf("missing file: got %v, want empty", got) + } +} + +// TestSchemaFieldExceptionsFileParses guards the committed exceptions file so a +// malformed edit is caught by unit tests rather than only in CI. +func TestSchemaFieldExceptionsFileParses(t *testing.T) { + t.Parallel() + // Tests run with the package directory as the working directory, so the repository root that + // loadSchemaFieldExceptions joins with the fixed relative path is "../..". + got, err := loadSchemaFieldExceptions(filepath.Join("..", "..")) + if err != nil { + t.Fatal(err) + } + for _, key := range got { + if _, _, ok := strings.Cut(key, "."); !ok { + t.Errorf("exception %q is not in Struct.Field form", key) + } + } +} + +func testOpenAPIFile(filename, path string, pathItem *openapi3.PathItem) *openapiFile { + return &openapiFile{ + filename: filename, + description: &openapi3.T{ + Paths: openapi3.NewPaths(openapi3.WithPath(path, pathItem)), + }, + } +} + +func testRequestBodyOperation(schema *openapi3.Schema) *openapi3.Operation { + return &openapi3.Operation{ + RequestBody: &openapi3.RequestBodyRef{ + Value: &openapi3.RequestBody{ + Content: openapi3.NewContentWithJSONSchema(schema), + }, + }, + } +} + +func writeFile(t *testing.T, filename, content string) { + t.Helper() + if err := os.WriteFile(filename, []byte(content), 0o600); err != nil { + t.Fatal(err) + } +} diff --git a/tools/metadata/testdata/check-schema-fields/github/demo.go b/tools/metadata/testdata/check-schema-fields/github/demo.go new file mode 100644 index 00000000000..225f7b6ac63 --- /dev/null +++ b/tools/metadata/testdata/check-schema-fields/github/demo.go @@ -0,0 +1,10 @@ +package github + +// DemoRequest is a demo request body. +// +//meta:schema request POST /demo +type DemoRequest struct { + ID int64 `json:"id"` + Name string `json:"name"` + Note *string `json:"note,omitempty"` +} diff --git a/tools/metadata/testdata/check-schema-fields/openapi_operations.yaml b/tools/metadata/testdata/check-schema-fields/openapi_operations.yaml new file mode 100644 index 00000000000..27c84c9a523 --- /dev/null +++ b/tools/metadata/testdata/check-schema-fields/openapi_operations.yaml @@ -0,0 +1 @@ +openapi_commit: schema-ref