From 8ff01f96c07bbf97b53656adac3f8ca29d852acf Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Thu, 9 Jul 2026 22:14:00 +0900 Subject: [PATCH 1/6] metadata: auto-detect OpenAPI schema field checks --- .github/workflows/linter.yml | 4 +- CONTRIBUTING.md | 32 + script/lint.sh | 12 +- tools/metadata/main.go | 91 +- tools/metadata/schema_fields.go | 969 ++++++++++++++++++ tools/metadata/schema_fields_test.go | 350 +++++++ .../check-schema-fields/github/demo.go | 11 + .../openapi_operations.yaml | 1 + 8 files changed, 1465 insertions(+), 5 deletions(-) create mode 100644 tools/metadata/schema_fields.go create mode 100644 tools/metadata/schema_fields_test.go create mode 100644 tools/metadata/testdata/check-schema-fields/github/demo.go create mode 100644 tools/metadata/testdata/check-schema-fields/openapi_operations.yaml diff --git a/.github/workflows/linter.yml b/.github/workflows/linter.yml index 384f53996a8..c7e6ce9f7ce 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 8aaf4307b64..e516ea94050 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -643,6 +643,38 @@ Its subcommands are: - `unused` - lists operations from `openapi_operations.yaml` that are not mapped from any methods. +- `check-schema-fields` - automatically matches GitHub's OpenAPI component + schemas to Go request structs when the JSON field set makes the match + unambiguous, then reports JSON field optionality mismatches. Ambiguous or + unsupported schemas are skipped instead of configured with per-schema + exceptions. It can be used to check whether required, non-nullable schema + fields are represented as non-pointer fields without `omitempty` or + `omitzero`, and whether optional schema fields remain omittable in Go. For + example: + + ```sh + script/metadata.sh check-schema-fields + ``` + + To experiment with one schema while refactoring, pass `--schema` with the + OpenAPI schema name. Filtered schemas also allow high-confidence schema-name + matches and response structs so the command can report the current + differences before the JSON field set is fully aligned: + + ```sh + script/metadata.sh check-schema-fields --schema repository-ruleset --verbose + ``` + + Use `--include-responses` to inspect response structs in bulk. This is useful + for measuring drift, but response required fields are treated more cautiously + than request bodies in this project. + + A few Go fields intentionally deviate from the OpenAPI schema (for example a + required field kept as a pointer pending a value-parameter refactor). These are + listed as `Struct.Field` entries in `schemaFieldExceptions` in + `tools/metadata/schema_fields.go`, and their diagnostics are suppressed; each is + a known deviation to fix and remove over time. + [OpenAPI descriptions of their API]: https://github.com/github/rest-api-description ## Scripts 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 00a9b0495de..ea3ef15462f 100644 --- a/tools/metadata/main.go +++ b/tools/metadata/main.go @@ -35,9 +35,20 @@ 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. +By default, the check automatically matches OpenAPI component schemas to Go +request structs only when the JSON field set makes the match unambiguous. Use +--schema to try one or more OpenAPI schema names; filtered schemas also allow +high-confidence schema-name matches and response structs to make refactoring +experiments easier. +`, - "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.`, + "schema_filter_help": `OpenAPI schema name to check. May be repeated. Defaults to all automatically matched schemas.`, + "include_responses_help": `Also check response structs. By default only request structs are checked unless --schema is provided.`, "openapi_validate_help": ` Instead of updating, make sure that the operations in openapi_operations.yaml's "openapi_operations" field are @@ -54,6 +65,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}"` @@ -79,6 +91,14 @@ func githubClient(apiURL, uploadURL string) (*github.Client, error) { return github.NewClient(github.WithAuthToken(token), github.WithEnterpriseURLs(apiURL, uploadURL)) } +func publicGithubClient(apiURL, uploadURL string) (*github.Client, error) { + token := os.Getenv("GITHUB_TOKEN") + if token == "" { + return github.NewClient(github.WithEnterpriseURLs(apiURL, uploadURL)) + } + return github.NewClient(github.WithAuthToken(token), github.WithEnterpriseURLs(apiURL, uploadURL)) +} + type updateOpenAPICmd struct { Ref string `kong:"default=main,help=${openapi_ref_help}"` ValidateGithub bool `kong:"name=validate,help=${openapi_validate_help}"` @@ -182,6 +202,73 @@ func (c *unusedCmd) Run(root *rootCmd, k *kong.Context) error { return nil } +type checkSchemaCmd struct { + Ref string `kong:"help=${openapi_ref_default_help}"` + Schemas []string `kong:"name=schema,help=${schema_filter_help}"` + IncludeResponses bool `kong:"name=include-responses,help=${include_responses_help}"` + JSON bool `kong:"help=${output_json_help}"` + Verbose bool `kong:"help='Print checked and skipped schema matches.'"` +} + +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 := publicGithubClient(root.GithubURL, root.UploadURL) + if err != nil { + return err + } + descriptions, err := getDescriptions(ctx, client, ref) + if err != nil { + return err + } + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: descriptions, + githubDir: filepath.Join(root.WorkingDir, "github"), + schemaNames: sliceSet(c.Schemas), + includeResponses: c.IncludeResponses, + }) + if err != nil { + return err + } + + if c.JSON { + enc := json.NewEncoder(k.Stdout) + enc.SetIndent("", " ") + if err := enc.Encode(result); err != nil { + return err + } + } else { + fmt.Fprintf(k.Stdout, "Found %v schema field issues\n", len(result.Diagnostics)) + fmt.Fprintf(k.Stdout, "Checked %v OpenAPI schema/Go struct pairs; skipped %v OpenAPI schemas\n", result.Summary.Checked, result.Summary.Skipped) + if c.Verbose { + for _, checked := range result.Checked { + fmt.Fprintf(k.Stdout, "checked: %s -> %s (%s)\n", checked.OpenAPISchema, checked.GoStruct, checked.MatchReason) + } + for _, skipped := range result.Skipped { + fmt.Fprintf(k.Stdout, "skipped: %s (%s)\n", skipped.OpenAPISchema, skipped.Reason) + } + } + 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_fields.go b/tools/metadata/schema_fields.go new file mode 100644 index 00000000000..23973ba1470 --- /dev/null +++ b/tools/metadata/schema_fields.go @@ -0,0 +1,969 @@ +// 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" + "path/filepath" + "reflect" + "slices" + "strings" + "unicode" + + "github.com/getkin/kin-openapi/openapi3" +) + +type schemaFieldCheckOptions struct { + descriptions []*openapiFile + githubDir string + schemaNames map[string]bool + includeResponses bool +} + +type schemaFieldCheckResult struct { + Summary schemaFieldCheckSummary `json:"summary"` + Checked []*schemaFieldChecked `json:"checked"` + Skipped []*schemaFieldSkipped `json:"skipped,omitempty"` + Diagnostics []*schemaFieldDiagnostic `json:"diagnostics"` +} + +type schemaFieldCheckSummary struct { + OpenAPISchemas int `json:"openapi_schemas"` + GoStructs int `json:"go_structs"` + Checked int `json:"checked"` + Skipped int `json:"skipped"` + Diagnostics int `json:"diagnostics"` +} + +type schemaFieldChecked struct { + OpenAPISchema string `json:"openapi_schema"` + GoStruct string `json:"go_struct"` + OpenAPIFile string `json:"openapi_file,omitempty"` + MatchReason string `json:"match_reason"` +} + +type schemaFieldSkipped struct { + OpenAPISchema string `json:"openapi_schema"` + OpenAPIFile string `json:"openapi_file,omitempty"` + Reason string `json:"reason"` +} + +type schemaFieldDiagnostic struct { + OpenAPISchema string `json:"openapi_schema"` + GoStruct string `json:"go_struct"` + Field string `json:"field"` + JSONName string `json:"json_name"` + Message string `json:"message"` + Filename string `json:"filename,omitempty"` + Line int `json:"line,omitempty"` + OpenAPIFile string `json:"openapi_file,omitempty"` +} + +func (d schemaFieldDiagnostic) String() string { + loc := diagLocation(d.Filename, d.Line) + if loc != "" { + loc += ": " + } + source := "" + if d.OpenAPIFile != "" { + source = fmt.Sprintf(" [%v]", d.OpenAPIFile) + } + return fmt.Sprintf("%v%v.%v (%v from %v): %v%v", loc, d.GoStruct, d.Field, d.JSONName, d.OpenAPISchema, 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) +} + +type goStructInfo struct { + name string + filename string + line int + fields map[string]goStructField +} + +type goStructField struct { + goStruct string + field string + jsonName string + hasOmitOption bool + isPointer bool + canBeOmitted bool + filename string + line int +} + +type openapiSchemaFields struct { + openapiSchema string + openapiFile string + required map[string]bool + properties map[string]openapiSchemaProperty +} + +type openapiSchemaProperty struct { + nullable bool + readOnly bool + writeOnly bool +} + +type schemaFieldMatch struct { + schema *openapiSchemaFields + goStruct *goStructInfo + matchReason string +} + +// schemaFieldExceptions lists "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 (for example a required field kept as a pointer pending a +// value-parameter refactor for #3644, or an optional field left as a value type). +// TODO: fix these fields and remove the exceptions. +var schemaFieldExceptions = map[string]bool{ + "DependencyGraphSnapshot.Detector": true, + "DependencyGraphSnapshot.Job": true, + "DependencyGraphSnapshot.Ref": true, + "DependencyGraphSnapshot.Scanned": true, + "DependencyGraphSnapshot.Sha": true, + "DeploymentBranchPolicyRequest.Name": true, + "ReviewCustomDeploymentProtectionRuleRequest.Comment": true, + "WorkflowsPermissionsOpt.RequireApprovalForForkPRWorkflows": true, + "WorkflowsPermissionsOpt.SendSecretsAndVariables": true, + "WorkflowsPermissionsOpt.SendWriteTokensToWorkflows": true, +} + +// filterAllowedSchemaFieldDiagnostics removes diagnostics whose "Struct.Field" is listed in +// schemaFieldExceptions. +func filterAllowedSchemaFieldDiagnostics(diagnostics []*schemaFieldDiagnostic) []*schemaFieldDiagnostic { + var kept []*schemaFieldDiagnostic + for _, diag := range diagnostics { + if schemaFieldExceptions[diag.GoStruct+"."+diag.Field] { + continue + } + kept = append(kept, diag) + } + return kept +} + +func checkSchemaFields(opts schemaFieldCheckOptions) (schemaFieldCheckResult, error) { + if len(opts.descriptions) == 0 { + return schemaFieldCheckResult{}, errors.New("no OpenAPI descriptions loaded") + } + + goStructs, requestStructs, err := collectGoStructs(opts.githubDir) + if err != nil { + return schemaFieldCheckResult{}, err + } + + schemas, skipped, err := collectOpenAPISchemaFields(opts.descriptions, opts.schemaNames) + if err != nil { + return schemaFieldCheckResult{}, err + } + + includeResponses := opts.includeResponses || len(opts.schemaNames) > 0 + matches, matchSkipped := matchOpenAPISchemasToGoStructs(schemas, goStructs, requestStructs, len(opts.schemaNames) > 0, includeResponses) + result := schemaFieldCheckResult{ + Skipped: append(skipped, matchSkipped...), + Summary: schemaFieldCheckSummary{ + OpenAPISchemas: len(schemas), + GoStructs: len(goStructs), + }, + } + + for _, match := range matches { + result.Checked = append(result.Checked, &schemaFieldChecked{ + OpenAPISchema: match.schema.openapiSchema, + GoStruct: match.goStruct.name, + OpenAPIFile: match.schema.openapiFile, + MatchReason: match.matchReason, + }) + result.Diagnostics = append(result.Diagnostics, compareSchemaFields(match)...) + } + + result.Diagnostics = filterAllowedSchemaFieldDiagnostics(result.Diagnostics) + sortSchemaFieldResult(&result) + result.Summary.Checked = len(result.Checked) + result.Summary.Skipped = len(result.Skipped) + 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.OpenAPISchema, b.OpenAPISchema), + 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.OpenAPISchema, b.OpenAPISchema), + cmp.Compare(a.GoStruct, b.GoStruct), + cmp.Compare(a.OpenAPIFile, b.OpenAPIFile), + cmp.Compare(a.MatchReason, b.MatchReason), + ) + }) + slices.SortFunc(result.Skipped, func(a, b *schemaFieldSkipped) int { + return cmp.Or( + cmp.Compare(a.OpenAPISchema, b.OpenAPISchema), + cmp.Compare(a.OpenAPIFile, b.OpenAPIFile), + cmp.Compare(a.Reason, b.Reason), + ) + }) +} + +func collectOpenAPISchemaFields(descriptions []*openapiFile, schemaNames map[string]bool) ([]*openapiSchemaFields, []*schemaFieldSkipped, error) { + var schemas []*openapiSchemaFields + var skipped []*schemaFieldSkipped + seen := map[string]bool{} + + for _, desc := range descriptions { + if desc.description == nil || desc.description.Components == nil || desc.description.Components.Schemas == nil { + continue + } + + names := make([]string, 0, len(desc.description.Components.Schemas)) + for name := range desc.description.Components.Schemas { + if len(schemaNames) > 0 && !schemaNames[name] { + continue + } + if seen[name] { + continue + } + names = append(names, name) + } + slices.Sort(names) + + for _, name := range names { + seen[name] = true + schemaRef := desc.description.Components.Schemas[name] + if schemaRef == nil || schemaRef.Value == nil { + skipped = append(skipped, newSchemaFieldSkipped(name, desc.filename, "schema reference is unresolved")) + continue + } + + schema, reason, err := flattenObjectSchema(schemaRef.Value) + if err != nil { + return nil, nil, fmt.Errorf("%v %v: %w", desc.filename, name, err) + } + if reason != "" { + skipped = append(skipped, newSchemaFieldSkipped(name, desc.filename, reason)) + continue + } + if len(schema.Properties) == 0 { + skipped = append(skipped, newSchemaFieldSkipped(name, desc.filename, "schema has no object properties")) + continue + } + + schemas = append(schemas, &openapiSchemaFields{ + openapiSchema: name, + openapiFile: desc.filename, + required: sliceSet(schema.Required), + properties: schemaProperties(schema.Properties), + }) + } + } + + for name := range schemaNames { + if !seen[name] { + skipped = append(skipped, newSchemaFieldSkipped(name, "", "schema filter did not match an OpenAPI schema")) + } + } + + return schemas, skipped, nil +} + +func newSchemaFieldSkipped(schemaName, filename, reason string) *schemaFieldSkipped { + return &schemaFieldSkipped{ + OpenAPISchema: schemaName, + OpenAPIFile: filename, + Reason: reason, + } +} + +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: append([]string{}, 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 sliceSet(values []string) map[string]bool { + set := make(map[string]bool, len(values)) + for _, value := range values { + set[value] = true + } + return set +} + +func matchOpenAPISchemasToGoStructs(schemas []*openapiSchemaFields, goStructs map[string]*goStructInfo, requestStructs map[string]bool, allowSchemaNameMatch, includeResponses bool) ([]*schemaFieldMatch, []*schemaFieldSkipped) { + var matches []*schemaFieldMatch + var skipped []*schemaFieldSkipped + + for _, schema := range schemas { + if allowSchemaNameMatch { + if match, ok, reason := matchBySchemaName(schema, goStructs, requestStructs, includeResponses); ok { + matches = append(matches, match) + continue + } else if reason != "" { + skipped = append(skipped, newSchemaFieldSkipped(schema.openapiSchema, schema.openapiFile, reason)) + continue + } + } + if match, ok, reason := matchByExactFieldSet(schema, goStructs, requestStructs, includeResponses); ok { + matches = append(matches, match) + } else { + skipped = append(skipped, newSchemaFieldSkipped(schema.openapiSchema, schema.openapiFile, reason)) + } + } + + return dropAmbiguousFieldSetMatches(matches, skipped) +} + +// dropAmbiguousFieldSetMatches removes exact-field-set matches for a Go struct that matched more +// than one OpenAPI schema. A field set that coincidentally equals several unrelated schemas (for +// example a generic {id, type}) is not a reliable match, so it is skipped rather than reported. +func dropAmbiguousFieldSetMatches(matches []*schemaFieldMatch, skipped []*schemaFieldSkipped) ([]*schemaFieldMatch, []*schemaFieldSkipped) { + fieldSetMatchCount := map[string]int{} + for _, match := range matches { + if match.matchReason == "exact JSON field set" { + fieldSetMatchCount[match.goStruct.name]++ + } + } + + var kept []*schemaFieldMatch + for _, match := range matches { + if match.matchReason == "exact JSON field set" && fieldSetMatchCount[match.goStruct.name] > 1 { + skipped = append(skipped, newSchemaFieldSkipped(match.schema.openapiSchema, match.schema.openapiFile, + "Go struct "+match.goStruct.name+" matches multiple schemas by field set")) + continue + } + kept = append(kept, match) + } + return kept, skipped +} + +func matchBySchemaName(schema *openapiSchemaFields, goStructs map[string]*goStructInfo, requestStructs map[string]bool, includeResponses bool) (*schemaFieldMatch, bool, string) { + var matches []*goStructInfo + for _, name := range goNameCandidates(schema.openapiSchema) { + goStruct, ok := goStructs[name] + if !ok { + continue + } + if !canCheckGoStruct(goStruct, requestStructs, includeResponses) { + continue + } + if !hasEnoughSharedFields(schema, goStruct) { + continue + } + matches = appendUniqueGoStruct(matches, goStruct) + } + + switch len(matches) { + case 0: + return nil, false, "" + case 1: + return &schemaFieldMatch{ + schema: schema, + goStruct: matches[0], + matchReason: "schema name", + }, true, "" + default: + return nil, false, "ambiguous Go struct name match: " + joinGoStructNames(matches) + } +} + +func matchByExactFieldSet(schema *openapiSchemaFields, goStructs map[string]*goStructInfo, requestStructs map[string]bool, includeResponses bool) (*schemaFieldMatch, bool, string) { + if len(schema.properties) < 2 { + return nil, false, "no unambiguous Go struct match" + } + + var matches []*goStructInfo + for _, goStruct := range goStructs { + if !canCheckGoStruct(goStruct, requestStructs, includeResponses) { + continue + } + if sameJSONFieldSet(schema, goStruct) { + matches = append(matches, goStruct) + } + } + + switch len(matches) { + case 0: + return nil, false, "no unambiguous Go struct match" + case 1: + return &schemaFieldMatch{ + schema: schema, + goStruct: matches[0], + matchReason: "exact JSON field set", + }, true, "" + default: + slices.SortFunc(matches, func(a, b *goStructInfo) int { + return cmp.Compare(a.name, b.name) + }) + return nil, false, "ambiguous Go struct field-set match: " + joinGoStructNames(matches) + } +} + +// canCheckGoStruct reports whether goStruct should be compared against an OpenAPI schema. +// By default only request body structs are checked; requestStructs holds the names of +// structs used as the body argument of a mutating client.NewRequest call. Response and +// other structs are only checked when includeResponses is set. +func canCheckGoStruct(goStruct *goStructInfo, requestStructs map[string]bool, includeResponses bool) bool { + return includeResponses || requestStructs[goStruct.name] +} + +func appendUniqueGoStruct(matches []*goStructInfo, goStruct *goStructInfo) []*goStructInfo { + for _, existing := range matches { + if existing.name == goStruct.name { + return matches + } + } + return append(matches, goStruct) +} + +func joinGoStructNames(goStructs []*goStructInfo) string { + names := make([]string, 0, len(goStructs)) + for _, goStruct := range goStructs { + names = append(names, goStruct.name) + } + slices.Sort(names) + return strings.Join(names, ", ") +} + +func hasEnoughSharedFields(schema *openapiSchemaFields, goStruct *goStructInfo) bool { + shared := sharedFieldCount(schema, goStruct) + if shared == 0 { + return false + } + smallest := min(len(schema.properties), len(goStruct.fields)) + if smallest <= 2 { + return shared == smallest + } + return shared >= 3 || shared*10 >= smallest*6 +} + +func sharedFieldCount(schema *openapiSchemaFields, goStruct *goStructInfo) int { + var shared int + for name := range schema.properties { + if _, ok := goStruct.fields[name]; ok { + shared++ + } + } + return shared +} + +func sameJSONFieldSet(schema *openapiSchemaFields, goStruct *goStructInfo) bool { + if len(schema.properties) != len(goStruct.fields) { + return false + } + for name := range schema.properties { + if _, ok := goStruct.fields[name]; !ok { + return false + } + } + return true +} + +var goInitialisms = map[string]string{ + "api": "API", + "apis": "APIs", + "gpg": "GPG", + "html": "HTML", + "http": "HTTP", + "https": "HTTPS", + "id": "ID", + "ids": "IDs", + "ip": "IP", + "ips": "IPs", + "oauth": "OAuth", + "oidc": "OIDC", + "scim": "SCIM", + "sms": "SMS", + "sso": "SSO", + "ssh": "SSH", + "totp": "TOTP", + "url": "URL", + "urls": "URLs", + "webhook": "Webhook", +} + +func goNameCandidates(openapiName string) []string { + tokens := splitOpenAPIName(openapiName) + if len(tokens) == 0 { + return nil + } + + variants := [][]string{tokens} + allSingular := make([]string, len(tokens)) + var allChanged bool + for i, token := range tokens { + singular := singularize(token) + allSingular[i] = singular + if singular != token { + allChanged = true + variant := append([]string{}, tokens...) + variant[i] = singular + variants = append(variants, variant) + } + } + if allChanged { + variants = append(variants, allSingular) + } + + var names []string + seen := map[string]bool{} + for _, variant := range variants { + name := goName(variant) + if name == "" || seen[name] { + continue + } + seen[name] = true + names = append(names, name) + } + return names +} + +func splitOpenAPIName(name string) []string { + return strings.FieldsFunc(name, func(r rune) bool { + return !unicode.IsLetter(r) && !unicode.IsDigit(r) + }) +} + +func singularize(token string) string { + lower := strings.ToLower(token) + switch { + case strings.HasSuffix(lower, "ies") && len(token) > 3: + return token[:len(token)-3] + "y" + case strings.HasSuffix(lower, "statuses"): + return token[:len(token)-2] + case strings.HasSuffix(lower, "ches") || strings.HasSuffix(lower, "shes") || strings.HasSuffix(lower, "xes") || strings.HasSuffix(lower, "ses"): + return token[:len(token)-2] + case strings.HasSuffix(lower, "s") && !strings.HasSuffix(lower, "ss") && len(token) > 1: + return token[:len(token)-1] + default: + return token + } +} + +func goName(tokens []string) string { + var b strings.Builder + for _, token := range tokens { + if token == "" { + continue + } + lower := strings.ToLower(token) + if initialism, ok := goInitialisms[lower]; ok { + b.WriteString(initialism) + continue + } + if isVersionToken(lower) { + b.WriteString(strings.ToUpper(lower[:1])) + b.WriteString(lower[1:]) + continue + } + b.WriteString(strings.ToUpper(token[:1])) + if len(token) > 1 { + b.WriteString(strings.ToLower(token[1:])) + } + } + return b.String() +} + +func isVersionToken(token string) bool { + if len(token) < 2 || token[0] != 'v' { + return false + } + for _, r := range token[1:] { + if !unicode.IsDigit(r) { + return false + } + } + return true +} + +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 := 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{ + OpenAPISchema: match.schema.openapiSchema, + 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{ + OpenAPISchema: match.schema.openapiSchema, + GoStruct: match.goStruct.name, + Field: field.field, + JSONName: field.jsonName, + Message: message, + Filename: field.filename, + Line: field.line, + OpenAPIFile: match.schema.openapiFile, + } +} + +// collectGoStructs parses the Go source files in dir and returns every exported struct by +// name along with the set of struct types used exclusively as request bodies. A request body +// is the type of the body argument passed to a mutating (POST, PUT, or PATCH) client.NewRequest +// call; types that are also returned as a response (for example shared types like Label) are +// excluded because they follow the all-pointer response convention rather than the request-body +// convention. +func collectGoStructs(dir string) (map[string]*goStructInfo, map[string]bool, error) { + structs := map[string]*goStructInfo{} + requestStructs := map[string]bool{} + responseStructs := map[string]bool{} + 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.SkipObjectResolution) + if err != nil { + return err + } + for _, decl := range fileNode.Decls { + if fn, ok := decl.(*ast.FuncDecl); ok { + collectRequestStructNames(fn, requestStructs) + collectResponseStructNames(fn, responseStructs) + continue + } + 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 + } + 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), + } + } + } + return nil + }) + if err != nil { + return nil, nil, err + } + // Exclude shared types that are also returned as a response; they follow the + // all-pointer response convention rather than the request-body convention. + for name := range responseStructs { + delete(requestStructs, name) + } + return structs, requestStructs, nil +} + +// collectRequestStructNames adds to requestStructs the name of the struct type passed as the +// body argument of every mutating client.NewRequest call in fn. +func collectRequestStructNames(fn *ast.FuncDecl, requestStructs map[string]bool) { + if fn.Body == nil { + return + } + ast.Inspect(fn.Body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if isClientNewRequest(call) && isMutatingNewRequest(call) { + if name := requestBodyStructName(fn, call.Args[3]); name != "" { + requestStructs[name] = true + } + } + return true + }) +} + +// collectResponseStructNames adds to responseStructs the name of every struct type returned +// as a pointer (*T) or pointer slice ([]*T) by fn, which marks it as a response type. +func collectResponseStructNames(fn *ast.FuncDecl, responseStructs map[string]bool) { + if fn.Type.Results == nil { + return + } + for _, field := range fn.Type.Results.List { + if name := responseStructName(field.Type); name != "" { + responseStructs[name] = true + } + } +} + +// responseStructName returns the struct name of a *T or []*T result type, or "" otherwise. +func responseStructName(expr ast.Expr) string { + switch t := expr.(type) { + case *ast.StarExpr: + if ident, ok := t.X.(*ast.Ident); ok { + return ident.Name + } + case *ast.ArrayType: + return responseStructName(t.Elt) + } + return "" +} + +// isClientNewRequest reports whether call is of the form x.client.NewRequest(...) or client.NewRequest(...). +func isClientNewRequest(call *ast.CallExpr) bool { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "NewRequest" { + return false + } + switch x := sel.X.(type) { + case *ast.SelectorExpr: + return x.Sel.Name == "client" + case *ast.Ident: + return x.Name == "client" + default: + return false + } +} + +// isMutatingNewRequest reports whether call's method argument is "PATCH", "POST", or "PUT" and a body argument is present. +func isMutatingNewRequest(call *ast.CallExpr) bool { + if len(call.Args) < 4 { + return false + } + lit, ok := call.Args[1].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return false + } + switch lit.Value { + case `"PATCH"`, `"POST"`, `"PUT"`: + return true + default: + return false + } +} + +// requestBodyStructName returns the struct type name of a client.NewRequest body argument, +// resolving a function parameter to its declared type or a composite literal to its type. +func requestBodyStructName(fn *ast.FuncDecl, arg ast.Expr) string { + switch a := arg.(type) { + case *ast.Ident: + if field := findFuncParam(fn, a.Name); field != nil { + return exprTypeName(field.Type) + } + case *ast.UnaryExpr: + if a.Op == token.AND { + return requestBodyStructName(fn, a.X) + } + case *ast.CompositeLit: + return exprTypeName(a.Type) + } + return "" +} + +func findFuncParam(fn *ast.FuncDecl, name string) *ast.Field { + if fn.Type.Params == nil { + return nil + } + for _, field := range fn.Type.Params.List { + for _, ident := range field.Names { + if ident.Name == name { + return field + } + } + } + return nil +} + +// exprTypeName returns the base type name of expr, unwrapping a pointer and resolving a qualified (pkg.Type) selector. +func exprTypeName(expr ast.Expr) string { + switch t := expr.(type) { + case *ast.StarExpr: + return exprTypeName(t.X) + case *ast.Ident: + return t.Name + case *ast.SelectorExpr: + return t.Sel.Name + default: + return "" + } +} + +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: defaultJSONName(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 defaultJSONName(name string) string { + return name +} + +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..d49cbcbcc72 --- /dev/null +++ b/tools/metadata/schema_fields_test.go @@ -0,0 +1,350 @@ +// 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 ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/getkin/kin-openapi/openapi3" + "github.com/google/go-cmp/cmp" +) + +func TestCheckSchemaFieldsMatchesBySchemaName(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +import "encoding/json" + +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", openapi3.Schemas{ + "demo": openapi3.NewSchemaRef("", &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, + schemaNames: sliceSet([]string{ + "demo", + }), + }) + if err != nil { + t.Fatal(err) + } + + if diff := cmp.Diff([]*schemaFieldChecked{{ + OpenAPISchema: "demo", + GoStruct: "Demo", + OpenAPIFile: "descriptions/api.github.com/api.github.com.json", + MatchReason: "schema name", + }}, 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 TestCheckSchemaFieldsMatchesByExactFieldSet(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +type ExactFieldsRequest struct { + ID *int64 `+"`json:\"id,omitempty\"`"+` + Name string `+"`json:\"name\"`"+` + Note *string `+"`json:\"note,omitempty\"`"+` +} + +func (s *svc) Create(ctx context.Context, body *ExactFieldsRequest) { + s.client.NewRequest(ctx, "POST", "u", body) +} +`) + + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", openapi3.Schemas{ + "unrelated-schema-name": openapi3.NewSchemaRef("", &openapi3.Schema{ + Required: []string{"id", "name"}, + Properties: openapi3.Schemas{ + "id": openapi3.NewSchemaRef("", openapi3.NewIntegerSchema()), + "name": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), + "note": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), + }, + }), + })}, + githubDir: githubDir, + }) + if err != nil { + t.Fatal(err) + } + + if diff := cmp.Diff([]*schemaFieldChecked{{ + OpenAPISchema: "unrelated-schema-name", + GoStruct: "ExactFieldsRequest", + OpenAPIFile: "descriptions/api.github.com/api.github.com.json", + MatchReason: "exact JSON field set", + }}, result.Checked); diff != "" { + t.Errorf("checked mismatch (-want +got):\n%v\nskipped: %#v", diff, result.Skipped) + } + + var got []string + for _, diag := range result.Diagnostics { + got = append(got, diag.JSONName+": "+diag.Message) + } + want := []string{ + "id: field is required and non-nullable in the OpenAPI schema but is a pointer", + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("diagnostics mismatch (-want +got):\n%v", diff) + } +} + +func TestCheckSchemaFieldsSkipsAmbiguousExactFieldSet(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +type FirstMatchRequest struct { + ID int64 `+"`json:\"id\"`"+` + Name string `+"`json:\"name\"`"+` +} + +type SecondMatchRequest struct { + ID int64 `+"`json:\"id\"`"+` + Name string `+"`json:\"name\"`"+` +} + +func (s *svc) First(ctx context.Context, body *FirstMatchRequest) { + s.client.NewRequest(ctx, "POST", "u", body) +} + +func (s *svc) Second(ctx context.Context, body *SecondMatchRequest) { + s.client.NewRequest(ctx, "POST", "u", body) +} +`) + + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", openapi3.Schemas{ + "unrelated-schema-name": openapi3.NewSchemaRef("", &openapi3.Schema{ + Required: []string{"id", "name"}, + Properties: openapi3.Schemas{ + "id": openapi3.NewSchemaRef("", openapi3.NewIntegerSchema()), + "name": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), + }, + }), + })}, + githubDir: githubDir, + }) + if err != nil { + t.Fatal(err) + } + + if len(result.Checked) != 0 { + t.Fatalf("checked = %v, want none", result.Checked) + } + if len(result.Diagnostics) != 0 { + t.Fatalf("diagnostics = %v, want none", result.Diagnostics) + } + if len(result.Skipped) != 1 { + t.Fatalf("skipped = %v, want one skip", result.Skipped) + } + if got := result.Skipped[0].Reason; !strings.Contains(got, "ambiguous Go struct field-set match") { + t.Errorf("skip reason = %q, want ambiguous field-set match", got) + } +} + +func TestCheckSchemaFieldsAllowsRequiredNullablePointer(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "nullable.go"), `package github + +type NullableDemo struct { + ID *int64 `+"`json:\"id\"`"+` +} +`) + + nullableInteger := openapi3.NewIntegerSchema() + nullableInteger.Nullable = true + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", openapi3.Schemas{ + "nullable-demo": openapi3.NewSchemaRef("", &openapi3.Schema{ + Required: []string{"id"}, + Properties: openapi3.Schemas{ + "id": openapi3.NewSchemaRef("", nullableInteger), + }, + }), + })}, + githubDir: githubDir, + schemaNames: sliceSet([]string{"nullable-demo"}), + }) + if err != nil { + t.Fatal(err) + } + if len(result.Diagnostics) != 0 { + t.Errorf("diagnostics = %v, want none", result.Diagnostics) + } +} + +func TestGoNameCandidates(t *testing.T) { + t.Parallel() + got := goNameCandidates("projects-v2") + want := []string{"ProjectsV2", "ProjectV2"} + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("goNameCandidates mismatch (-want +got):\n%v", diff) + } +} + +//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{ + Components: &openapi3.Components{ + Schemas: openapi3.Schemas{ + "demo": openapi3.NewSchemaRef("", &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 OpenAPI schema/Go struct pairs; skipped 0 OpenAPI schemas", "") + res.assertNoErr() + res.checkGolden() +} + +func TestCheckSchemaFieldsSkipsStructMatchingMultipleSchemas(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github + +type ItemRequest struct { + ID int64 `+"`json:\"id\"`"+` + Type string `+"`json:\"type\"`"+` +} + +func (s *svc) Add(ctx context.Context, body *ItemRequest) { + s.client.NewRequest(ctx, "POST", "u", body) +} +`) + + itemSchema := func() *openapi3.SchemaRef { + return openapi3.NewSchemaRef("", &openapi3.Schema{ + Required: []string{"id", "type"}, + Properties: openapi3.Schemas{ + "id": openapi3.NewSchemaRef("", openapi3.NewIntegerSchema()), + "type": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), + }, + }) + } + result, err := checkSchemaFields(schemaFieldCheckOptions{ + descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", openapi3.Schemas{ + "schema-a": itemSchema(), + "schema-b": itemSchema(), + })}, + githubDir: githubDir, + }) + if err != nil { + t.Fatal(err) + } + + if len(result.Checked) != 0 { + t.Fatalf("checked = %v, want none", result.Checked) + } + if len(result.Diagnostics) != 0 { + t.Fatalf("diagnostics = %v, want none", result.Diagnostics) + } + var dropped bool + for _, skip := range result.Skipped { + if strings.Contains(skip.Reason, "matches multiple schemas by field set") { + dropped = true + } + } + if !dropped { + t.Errorf("skipped = %#v, want a \"matches multiple schemas by field set\" reason", result.Skipped) + } +} + +func TestFilterAllowedSchemaFieldDiagnostics(t *testing.T) { + t.Parallel() + var exemptStruct, exemptField string + for key := range schemaFieldExceptions { + if s, f, ok := strings.Cut(key, "."); ok { + exemptStruct, exemptField = s, f + break + } + } + if exemptStruct == "" { + t.Skip("no schema field exceptions configured") + } + + got := filterAllowedSchemaFieldDiagnostics([]*schemaFieldDiagnostic{ + {GoStruct: exemptStruct, Field: exemptField}, + {GoStruct: "NotExemptStruct", Field: "NotExemptField"}, + }) + if len(got) != 1 || got[0].GoStruct != "NotExemptStruct" { + t.Errorf("filterAllowedSchemaFieldDiagnostics = %+v, want only NotExemptStruct.NotExemptField", got) + } +} + +func testOpenAPIFile(filename string, schemas openapi3.Schemas) *openapiFile { + return &openapiFile{ + filename: filename, + description: &openapi3.T{ + Components: &openapi3.Components{ + Schemas: schemas, + }, + }, + } +} + +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..7571e4cea28 --- /dev/null +++ b/tools/metadata/testdata/check-schema-fields/github/demo.go @@ -0,0 +1,11 @@ +package github + +type DemoRequest struct { + ID int64 `json:"id"` + Name string `json:"name"` + Note *string `json:"note,omitempty"` +} + +func (s *svc) Create(ctx context.Context, body *DemoRequest) { + s.client.NewRequest(ctx, "POST", "u", body) +} 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 From c6425df5c38ec5f42d0278fffedd8624a4c0f4eb Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Fri, 10 Jul 2026 00:11:12 +0000 Subject: [PATCH 2/6] metadata: address check-schema-fields review feedback - Load field-optionality exceptions from schema_field_exceptions.yaml instead of hardcoding them in Go, with a --exceptions flag. - Add table-driven unit tests for the schema_fields.go helpers and the new exceptions loader. - Document the hasEnoughSharedFields thresholds as named constants. - Use %v instead of %s in the check-schema-fields verbose output. - Note the intentional Go-initialisms duplication with tools/structfield. --- CONTRIBUTING.md | 8 +- tools/metadata/main.go | 18 +- tools/metadata/schema_field_exceptions.yaml | 21 + tools/metadata/schema_fields.go | 84 +++- tools/metadata/schema_fields_test.go | 487 +++++++++++++++++++- 5 files changed, 576 insertions(+), 42 deletions(-) create mode 100644 tools/metadata/schema_field_exceptions.yaml diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e516ea94050..84e7fc404d6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -671,9 +671,11 @@ Its subcommands are: A few Go fields intentionally deviate from the OpenAPI schema (for example a required field kept as a pointer pending a value-parameter refactor). These are - listed as `Struct.Field` entries in `schemaFieldExceptions` in - `tools/metadata/schema_fields.go`, and their diagnostics are suppressed; each is - a known deviation to fix and remove over time. + 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. Use + `--exceptions` to point the command at a different file. [OpenAPI descriptions of their API]: https://github.com/github/rest-api-description diff --git a/tools/metadata/main.go b/tools/metadata/main.go index ea3ef15462f..41ab935824a 100644 --- a/tools/metadata/main.go +++ b/tools/metadata/main.go @@ -49,6 +49,7 @@ experiments easier. "openapi_ref_default_help": `Git ref to pull OpenAPI descriptions from. Defaults to openapi_commit from openapi_operations.yaml.`, "schema_filter_help": `OpenAPI schema name to check. May be repeated. Defaults to all automatically matched schemas.`, "include_responses_help": `Also check response structs. By default only request structs are checked unless --schema is provided.`, + "schema_exceptions_help": `Path (relative to the working directory) of the YAML file listing "Struct.Field" exceptions to suppress. Missing default file is treated as no exceptions.`, "openapi_validate_help": ` Instead of updating, make sure that the operations in openapi_operations.yaml's "openapi_operations" field are @@ -202,10 +203,15 @@ func (c *unusedCmd) Run(root *rootCmd, k *kong.Context) error { return nil } +// defaultSchemaFieldExceptionsFile is the working-directory-relative path of the +// exceptions file loaded by check-schema-fields when --exceptions is not overridden. +const defaultSchemaFieldExceptionsFile = "tools/metadata/schema_field_exceptions.yaml" + type checkSchemaCmd struct { Ref string `kong:"help=${openapi_ref_default_help}"` Schemas []string `kong:"name=schema,help=${schema_filter_help}"` IncludeResponses bool `kong:"name=include-responses,help=${include_responses_help}"` + ExceptionsFile string `kong:"name=exceptions,default='tools/metadata/schema_field_exceptions.yaml',help=${schema_exceptions_help}"` JSON bool `kong:"help=${output_json_help}"` Verbose bool `kong:"help='Print checked and skipped schema matches.'"` } @@ -232,11 +238,19 @@ func (c *checkSchemaCmd) Run(root *rootCmd, k *kong.Context) error { if err != nil { return err } + exceptions, err := loadSchemaFieldExceptions( + filepath.Join(root.WorkingDir, c.ExceptionsFile), + c.ExceptionsFile == defaultSchemaFieldExceptionsFile, + ) + if err != nil { + return err + } result, err := checkSchemaFields(schemaFieldCheckOptions{ descriptions: descriptions, githubDir: filepath.Join(root.WorkingDir, "github"), schemaNames: sliceSet(c.Schemas), includeResponses: c.IncludeResponses, + exceptions: exceptions, }) if err != nil { return err @@ -253,10 +267,10 @@ func (c *checkSchemaCmd) Run(root *rootCmd, k *kong.Context) error { fmt.Fprintf(k.Stdout, "Checked %v OpenAPI schema/Go struct pairs; skipped %v OpenAPI schemas\n", result.Summary.Checked, result.Summary.Skipped) if c.Verbose { for _, checked := range result.Checked { - fmt.Fprintf(k.Stdout, "checked: %s -> %s (%s)\n", checked.OpenAPISchema, checked.GoStruct, checked.MatchReason) + fmt.Fprintf(k.Stdout, "checked: %v -> %v (%v)\n", checked.OpenAPISchema, checked.GoStruct, checked.MatchReason) } for _, skipped := range result.Skipped { - fmt.Fprintf(k.Stdout, "skipped: %s (%s)\n", skipped.OpenAPISchema, skipped.Reason) + fmt.Fprintf(k.Stdout, "skipped: %v (%v)\n", skipped.OpenAPISchema, skipped.Reason) } } for _, diag := range result.Diagnostics { diff --git a/tools/metadata/schema_field_exceptions.yaml b/tools/metadata/schema_field_exceptions.yaml new file mode 100644 index 00000000000..a80b89e0d7a --- /dev/null +++ b/tools/metadata/schema_field_exceptions.yaml @@ -0,0 +1,21 @@ +# schema_field_exceptions.yaml lists "Struct.Field" entries whose JSON field +# optionality intentionally deviates from the OpenAPI schema, so that their +# check-schema-fields diagnostics are suppressed. +# +# Each entry is a known deviation awaiting cleanup (for example a required field +# kept as a pointer pending a value-parameter refactor for #3644, or an optional +# field left as a value type). This mirrors how the paramcheck and structfield +# linters keep their allowlists in .golangci.yml. +# +# TODO: fix these fields and remove the exceptions. +exceptions: + - DependencyGraphSnapshot.Detector + - DependencyGraphSnapshot.Job + - DependencyGraphSnapshot.Ref + - DependencyGraphSnapshot.Scanned + - DependencyGraphSnapshot.Sha + - DeploymentBranchPolicyRequest.Name + - ReviewCustomDeploymentProtectionRuleRequest.Comment + - WorkflowsPermissionsOpt.RequireApprovalForForkPRWorkflows + - WorkflowsPermissionsOpt.SendSecretsAndVariables + - WorkflowsPermissionsOpt.SendWriteTokensToWorkflows diff --git a/tools/metadata/schema_fields.go b/tools/metadata/schema_fields.go index 23973ba1470..f9d127d520b 100644 --- a/tools/metadata/schema_fields.go +++ b/tools/metadata/schema_fields.go @@ -14,6 +14,7 @@ import ( "go/token" "io/fs" "maps" + "os" "path/filepath" "reflect" "slices" @@ -21,6 +22,7 @@ import ( "unicode" "github.com/getkin/kin-openapi/openapi3" + "go.yaml.in/yaml/v3" ) type schemaFieldCheckOptions struct { @@ -28,6 +30,9 @@ type schemaFieldCheckOptions struct { githubDir string schemaNames map[string]bool includeResponses bool + // exceptions holds "Struct.Field" entries whose diagnostics are suppressed. It is + // loaded from schema_field_exceptions.yaml by the command; see loadSchemaFieldExceptions. + exceptions map[string]bool } type schemaFieldCheckResult struct { @@ -128,30 +133,39 @@ type schemaFieldMatch struct { matchReason string } -// schemaFieldExceptions lists "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 (for example a required field kept as a pointer pending a -// value-parameter refactor for #3644, or an optional field left as a value type). -// TODO: fix these fields and remove the exceptions. -var schemaFieldExceptions = map[string]bool{ - "DependencyGraphSnapshot.Detector": true, - "DependencyGraphSnapshot.Job": true, - "DependencyGraphSnapshot.Ref": true, - "DependencyGraphSnapshot.Scanned": true, - "DependencyGraphSnapshot.Sha": true, - "DeploymentBranchPolicyRequest.Name": true, - "ReviewCustomDeploymentProtectionRuleRequest.Comment": true, - "WorkflowsPermissionsOpt.RequireApprovalForForkPRWorkflows": true, - "WorkflowsPermissionsOpt.SendSecretsAndVariables": true, - "WorkflowsPermissionsOpt.SendWriteTokensToWorkflows": true, +// 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 filename and +// returns them as a set. A missing file yields an empty set and no error when optional is +// true, so callers relying on the default path do not need the file to exist; an explicitly +// requested file that is missing is an error. +func loadSchemaFieldExceptions(filename string, optional bool) (map[string]bool, error) { + b, err := os.ReadFile(filename) + if err != nil { + if optional && errors.Is(err, fs.ErrNotExist) { + return map[string]bool{}, nil + } + return nil, err + } + var exceptionsFile schemaFieldExceptionsFile + if err := yaml.Unmarshal(b, &exceptionsFile); err != nil { + return nil, fmt.Errorf("%v: %w", filename, err) + } + return sliceSet(exceptionsFile.Exceptions), nil } // filterAllowedSchemaFieldDiagnostics removes diagnostics whose "Struct.Field" is listed in -// schemaFieldExceptions. -func filterAllowedSchemaFieldDiagnostics(diagnostics []*schemaFieldDiagnostic) []*schemaFieldDiagnostic { +// the exceptions set. +func filterAllowedSchemaFieldDiagnostics(diagnostics []*schemaFieldDiagnostic, exceptions map[string]bool) []*schemaFieldDiagnostic { var kept []*schemaFieldDiagnostic for _, diag := range diagnostics { - if schemaFieldExceptions[diag.GoStruct+"."+diag.Field] { + if exceptions[diag.GoStruct+"."+diag.Field] { continue } kept = append(kept, diag) @@ -194,7 +208,7 @@ func checkSchemaFields(opts schemaFieldCheckOptions) (schemaFieldCheckResult, er result.Diagnostics = append(result.Diagnostics, compareSchemaFields(match)...) } - result.Diagnostics = filterAllowedSchemaFieldDiagnostics(result.Diagnostics) + result.Diagnostics = filterAllowedSchemaFieldDiagnostics(result.Diagnostics, opts.exceptions) sortSchemaFieldResult(&result) result.Summary.Checked = len(result.Checked) result.Summary.Skipped = len(result.Skipped) @@ -492,16 +506,36 @@ func joinGoStructNames(goStructs []*goStructInfo) string { return strings.Join(names, ", ") } +// Thresholds for hasEnoughSharedFields. A schema-name match already agrees on the +// Go type name, so these guard only against a name that coincidentally collides with +// an unrelated struct. They are deliberately lenient: a wrong match is dropped later +// as ambiguous, but a missed match silently skips a real check. +const ( + // minSharedFieldsForMatch accepts a match once this many JSON fields overlap, + // regardless of struct size: three shared, correctly named fields are unlikely + // to line up by chance. + minSharedFieldsForMatch = 3 + // minSharedFieldPercent is the fallback for structs smaller than + // minSharedFieldsForMatch: the overlap must cover at least this share of the + // smaller field set. Compared as shared*100 >= smallest*minSharedFieldPercent + // to stay in integer math. + minSharedFieldPercent = 60 +) + +// hasEnoughSharedFields reports whether schema and goStruct share enough JSON fields to +// treat a schema-name match as genuine rather than a coincidental name collision. func hasEnoughSharedFields(schema *openapiSchemaFields, goStruct *goStructInfo) bool { shared := sharedFieldCount(schema, goStruct) if shared == 0 { return false } smallest := min(len(schema.properties), len(goStruct.fields)) + // A type with one or two fields has too little signal for a percentage test, so + // require every field to line up before trusting the match. if smallest <= 2 { return shared == smallest } - return shared >= 3 || shared*10 >= smallest*6 + return shared >= minSharedFieldsForMatch || shared*100 >= smallest*minSharedFieldPercent } func sharedFieldCount(schema *openapiSchemaFields, goStruct *goStructInfo) int { @@ -526,6 +560,14 @@ func sameJSONFieldSet(schema *openapiSchemaFields, goStruct *goStructInfo) bool return true } +// goInitialisms maps a lowercase OpenAPI name token to its idiomatic Go casing when +// building candidate struct names in goName. The structfield linter keeps a similar +// list (its `initialisms`/`specialCases`), but it lives in the separate +// github.com/google/go-github/v89/tools/structfield module and is keyed and shaped +// differently (uppercase-keyed set for a different purpose), so the two are +// intentionally not shared: unifying them would require a new shared module that both +// tools depend on. This list is deliberately small and only needs the initialisms that +// actually appear in OpenAPI schema names. var goInitialisms = map[string]string{ "api": "API", "apis": "APIs", diff --git a/tools/metadata/schema_fields_test.go b/tools/metadata/schema_fields_test.go index d49cbcbcc72..6b0a4513ac9 100644 --- a/tools/metadata/schema_fields_test.go +++ b/tools/metadata/schema_fields_test.go @@ -6,6 +6,7 @@ package main import ( + "go/parser" "os" "path/filepath" "strings" @@ -225,10 +226,422 @@ type NullableDemo struct { func TestGoNameCandidates(t *testing.T) { t.Parallel() - got := goNameCandidates("projects-v2") - want := []string{"ProjectsV2", "ProjectV2"} + tests := []struct { + name string + in string + want []string + }{ + {name: "plural and version tokens", in: "projects-v2", want: []string{"ProjectsV2", "ProjectV2"}}, + {name: "singular unchanged", in: "repository", want: []string{"Repository"}}, + {name: "trailing plural", in: "teams", want: []string{"Teams", "Team"}}, + {name: "multiple words no plural", in: "code-scanning-alert", want: []string{"CodeScanningAlert"}}, + {name: "initialism token", in: "api", want: []string{"API"}}, + {name: "empty", in: "", want: nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if diff := cmp.Diff(tt.want, goNameCandidates(tt.in)); diff != "" { + t.Errorf("goNameCandidates(%q) mismatch (-want +got):\n%v", tt.in, diff) + } + }) + } +} + +func TestSingularize(t *testing.T) { + t.Parallel() + tests := []struct { + in, want string + }{ + {"projects", "project"}, + {"policies", "policy"}, + {"statuses", "status"}, + {"boxes", "box"}, + {"branches", "branch"}, + {"buses", "bus"}, + {"keys", "key"}, + {"class", "class"}, // "ss" is not treated as a plural "s" + {"v2", "v2"}, + {"", ""}, + } + for _, tt := range tests { + if got := singularize(tt.in); got != tt.want { + t.Errorf("singularize(%q) = %q, want %q", tt.in, got, tt.want) + } + } +} + +func TestGoName(t *testing.T) { + t.Parallel() + tests := []struct { + name string + in []string + want string + }{ + {name: "initialism", in: []string{"api"}, want: "API"}, + {name: "url initialism", in: []string{"url"}, want: "URL"}, + {name: "special case oauth", in: []string{"oauth"}, want: "OAuth"}, + {name: "version token", in: []string{"projects", "v2"}, want: "ProjectsV2"}, + {name: "plain word", in: []string{"repository"}, want: "Repository"}, + {name: "skips empty tokens", in: []string{"code", "", "scanning"}, want: "CodeScanning"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := goName(tt.in); got != tt.want { + t.Errorf("goName(%v) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +func TestIsVersionToken(t *testing.T) { + t.Parallel() + tests := []struct { + in string + want bool + }{ + {"v2", true}, + {"v10", true}, + {"v", false}, + {"vx", false}, + {"2", false}, + {"version", false}, + {"", false}, + } + for _, tt := range tests { + if got := isVersionToken(tt.in); got != tt.want { + t.Errorf("isVersionToken(%q) = %v, want %v", tt.in, got, tt.want) + } + } +} + +func TestSplitOpenAPIName(t *testing.T) { + t.Parallel() + tests := []struct { + in string + want []string + }{ + {"projects-v2", []string{"projects", "v2"}}, + {"api.github.com", []string{"api", "github", "com"}}, + {"foo_bar-baz", []string{"foo", "bar", "baz"}}, + {"single", []string{"single"}}, + } + for _, tt := range tests { + if diff := cmp.Diff(tt.want, splitOpenAPIName(tt.in)); diff != "" { + t.Errorf("splitOpenAPIName(%q) mismatch (-want +got):\n%v", tt.in, diff) + } + } + if got := splitOpenAPIName(""); len(got) != 0 { + t.Errorf("splitOpenAPIName(\"\") = %v, want empty", got) + } +} + +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 TestSliceSet(t *testing.T) { + t.Parallel() + got := sliceSet([]string{"a", "b", "a"}) + want := map[string]bool{"a": true, "b": true} if diff := cmp.Diff(want, got); diff != "" { - t.Errorf("goNameCandidates mismatch (-want +got):\n%v", diff) + t.Errorf("sliceSet mismatch (-want +got):\n%v", diff) + } + if got := sliceSet(nil); len(got) != 0 { + t.Errorf("sliceSet(nil) = %v, want empty", got) + } +} + +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, %d) = %q, want %q", tt.filename, tt.line, got, tt.want) + } + } +} + +func TestSchemaFieldDiagnosticString(t *testing.T) { + t.Parallel() + withLoc := schemaFieldDiagnostic{ + OpenAPISchema: "sch", 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 sch): msg [api.json]"; got != want { + t.Errorf("String() = %q, want %q", got, want) + } + noLoc := schemaFieldDiagnostic{ + OpenAPISchema: "sch", GoStruct: "S", Field: "F", JSONName: "j", Message: "msg", + } + if got, want := noLoc.String(), "S.F (j from sch): 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("%s: canCheckOptionality() = %v, want %v", tt.name, got, tt.want) + } + } +} + +// fieldSet and structWith build the minimal shapes the field-matching helpers need. +func fieldSet[T any](names ...string) map[string]T { + m := make(map[string]T, len(names)) + for _, name := range names { + var zero T + m[name] = zero + } + return m +} + +func structWith(fields ...string) *goStructInfo { + return &goStructInfo{fields: fieldSet[goStructField](fields...)} +} + +func TestSharedFieldCount(t *testing.T) { + t.Parallel() + got := sharedFieldCount( + &openapiSchemaFields{properties: fieldSet[openapiSchemaProperty]("a", "b", "c")}, + structWith("b", "c", "d"), + ) + if got != 2 { + t.Errorf("sharedFieldCount = %d, want 2", got) + } +} + +func TestSameJSONFieldSet(t *testing.T) { + t.Parallel() + tests := []struct { + name string + schema []string + strct []string + want bool + }{ + {name: "equal", schema: []string{"a", "b"}, strct: []string{"a", "b"}, want: true}, + {name: "different length", schema: []string{"a", "b"}, strct: []string{"a"}, want: false}, + {name: "same length different members", schema: []string{"a", "b"}, strct: []string{"a", "c"}, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := sameJSONFieldSet( + &openapiSchemaFields{properties: fieldSet[openapiSchemaProperty](tt.schema...)}, + structWith(tt.strct...), + ) + if got != tt.want { + t.Errorf("sameJSONFieldSet = %v, want %v", got, tt.want) + } + }) + } +} + +func TestHasEnoughSharedFields(t *testing.T) { + t.Parallel() + tests := []struct { + name string + schema []string + strct []string + want bool + }{ + {name: "three shared", schema: []string{"a", "b", "c"}, strct: []string{"a", "b", "c"}, want: true}, + {name: "tiny type all shared", schema: []string{"a", "b"}, strct: []string{"a", "b"}, want: true}, + {name: "tiny type partial", schema: []string{"a", "b"}, strct: []string{"a", "x"}, want: false}, + {name: "percentage threshold met", schema: []string{"a", "b", "c"}, strct: []string{"a", "b", "x"}, want: true}, + {name: "percentage threshold missed", schema: []string{"a", "b", "c", "d"}, strct: []string{"a", "b", "x", "y"}, want: false}, + {name: "no overlap", schema: []string{"a", "b", "c"}, strct: []string{"x", "y", "z"}, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := hasEnoughSharedFields( + &openapiSchemaFields{properties: fieldSet[openapiSchemaProperty](tt.schema...)}, + structWith(tt.strct...), + ) + if got != tt.want { + t.Errorf("hasEnoughSharedFields = %v, want %v", 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("%s: 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.Errorf("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") } } @@ -311,26 +724,68 @@ func (s *svc) Add(ctx context.Context, body *ItemRequest) { func TestFilterAllowedSchemaFieldDiagnostics(t *testing.T) { t.Parallel() - var exemptStruct, exemptField string - for key := range schemaFieldExceptions { - if s, f, ok := strings.Cut(key, "."); ok { - exemptStruct, exemptField = s, f - break - } - } - if exemptStruct == "" { - t.Skip("no schema field exceptions configured") - } - + exceptions := sliceSet([]string{"ExemptStruct.ExemptField"}) got := filterAllowedSchemaFieldDiagnostics([]*schemaFieldDiagnostic{ - {GoStruct: exemptStruct, Field: exemptField}, + {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() + path := filepath.Join(dir, "schema_field_exceptions.yaml") + writeFile(t, path, `# comment +exceptions: + - StructA.FieldA + - StructB.FieldB # TODO: fix +`) + + got, err := loadSchemaFieldExceptions(path, false) + if err != nil { + t.Fatal(err) + } + want := map[string]bool{"StructA.FieldA": true, "StructB.FieldB": true} + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("loadSchemaFieldExceptions mismatch (-want +got):\n%v", diff) + } + + // A missing optional file yields an empty set and no error. + missing := filepath.Join(dir, "does-not-exist.yaml") + got, err = loadSchemaFieldExceptions(missing, true) + if err != nil { + t.Fatalf("optional missing file: unexpected error %v", err) + } + if len(got) != 0 { + t.Errorf("optional missing file: got %v, want empty", got) + } + + // A missing required file is an error. + if _, err := loadSchemaFieldExceptions(missing, false); err == nil { + t.Error("required missing file: got nil error, want error") + } +} + +// 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 committed + // file (tools/metadata/schema_field_exceptions.yaml) is reachable by its basename. + got, err := loadSchemaFieldExceptions("schema_field_exceptions.yaml", false) + 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 string, schemas openapi3.Schemas) *openapiFile { return &openapiFile{ filename: filename, From e8c781b20d9559e66af622e293b15ee7054615f4 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Fri, 10 Jul 2026 00:32:12 +0000 Subject: [PATCH 3/6] metadata: fix linter issues in schema_fields_test.go Use %v instead of %d/%s to satisfy the fmtpercentv linter, and replace a no-argument t.Errorf with t.Error for the revive unnecessary-format check. --- tools/metadata/schema_fields_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/metadata/schema_fields_test.go b/tools/metadata/schema_fields_test.go index 6b0a4513ac9..b849f1c8a91 100644 --- a/tools/metadata/schema_fields_test.go +++ b/tools/metadata/schema_fields_test.go @@ -424,7 +424,7 @@ func TestDiagLocation(t *testing.T) { } for _, tt := range tests { if got := diagLocation(tt.filename, tt.line); got != tt.want { - t.Errorf("diagLocation(%q, %d) = %q, want %q", tt.filename, tt.line, got, tt.want) + t.Errorf("diagLocation(%q, %v) = %q, want %q", tt.filename, tt.line, got, tt.want) } } } @@ -459,7 +459,7 @@ func TestCanCheckOptionality(t *testing.T) { } for _, tt := range tests { if got := tt.prop.canCheckOptionality(); got != tt.want { - t.Errorf("%s: canCheckOptionality() = %v, want %v", tt.name, got, tt.want) + t.Errorf("%v: canCheckOptionality() = %v, want %v", tt.name, got, tt.want) } } } @@ -485,7 +485,7 @@ func TestSharedFieldCount(t *testing.T) { structWith("b", "c", "d"), ) if got != 2 { - t.Errorf("sharedFieldCount = %d, want 2", got) + t.Errorf("sharedFieldCount = %v, want 2", got) } } @@ -559,7 +559,7 @@ func TestHasUnsupportedComposition(t *testing.T) { } for _, tt := range tests { if got := hasUnsupportedComposition(tt.schema); got != tt.want { - t.Errorf("%s: hasUnsupportedComposition = %v, want %v", tt.name, got, tt.want) + t.Errorf("%v: hasUnsupportedComposition = %v, want %v", tt.name, got, tt.want) } } } @@ -576,7 +576,7 @@ func TestFlattenObjectSchema(t *testing.T) { t.Fatalf("flattenObjectSchema = (_, %q, %v)", reason, err) } if got != obj { - t.Errorf("flattenObjectSchema returned a different schema for a plain object") + t.Error("flattenObjectSchema returned a different schema for a plain object") } }) From aa993d8864c48938268aedca510312dc68de8ae7 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Fri, 10 Jul 2026 23:15:49 +0900 Subject: [PATCH 4/6] metadata: simplify check-schema-fields per review Address inline review feedback on the check-schema-fields command: - Replace the sliceSet helper with []string plus slices.Contains for the exceptions, schema-name filter, and required-field sets. - Replace append([]string{}, s...) with slices.Clone(s). - Drop the --exceptions flag and always read the fixed tools/metadata/schema_field_exceptions.yaml path. - Drop the unused --json flag from check-schema-fields. - Use githubClient (which requires GITHUB_TOKEN) instead of a duplicate publicGithubClient. - Remove the trivial defaultJSONName pass-through. - Wrap the added comments to the package's line-length convention and trim the exceptions file header. --- CONTRIBUTING.md | 3 +- tools/metadata/main.go | 62 +++------ tools/metadata/schema_field_exceptions.yaml | 10 +- tools/metadata/schema_fields.go | 144 +++++++++----------- tools/metadata/schema_fields_test.go | 55 +++----- 5 files changed, 102 insertions(+), 172 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 84e7fc404d6..d10b1dcd6da 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -674,8 +674,7 @@ Its subcommands 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. Use - `--exceptions` to point the command at a different file. + file (rather than the Go source) to add or remove an exception. [OpenAPI descriptions of their API]: https://github.com/github/rest-api-description diff --git a/tools/metadata/main.go b/tools/metadata/main.go index 41ab935824a..6df274741ff 100644 --- a/tools/metadata/main.go +++ b/tools/metadata/main.go @@ -36,12 +36,10 @@ 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. -By default, the check automatically matches OpenAPI component schemas to Go -request structs only when the JSON field set makes the match unambiguous. Use ---schema to try one or more OpenAPI schema names; filtered schemas also allow -high-confidence schema-name matches and response structs to make refactoring -experiments easier. +Check Go struct JSON field optionality against GitHub's OpenAPI schemas. By default, the check automatically +matches OpenAPI component schemas to Go request structs only when the JSON field set makes the match unambiguous. +Use --schema to try one or more OpenAPI schema names; filtered schemas also allow high-confidence schema-name +matches and response structs to make refactoring experiments easier. `, "working_dir_help": `Working directory. Should be the root of the go-github repository.`, @@ -49,7 +47,6 @@ experiments easier. "openapi_ref_default_help": `Git ref to pull OpenAPI descriptions from. Defaults to openapi_commit from openapi_operations.yaml.`, "schema_filter_help": `OpenAPI schema name to check. May be repeated. Defaults to all automatically matched schemas.`, "include_responses_help": `Also check response structs. By default only request structs are checked unless --schema is provided.`, - "schema_exceptions_help": `Path (relative to the working directory) of the YAML file listing "Struct.Field" exceptions to suppress. Missing default file is treated as no exceptions.`, "openapi_validate_help": ` Instead of updating, make sure that the operations in openapi_operations.yaml's "openapi_operations" field are @@ -92,14 +89,6 @@ func githubClient(apiURL, uploadURL string) (*github.Client, error) { return github.NewClient(github.WithAuthToken(token), github.WithEnterpriseURLs(apiURL, uploadURL)) } -func publicGithubClient(apiURL, uploadURL string) (*github.Client, error) { - token := os.Getenv("GITHUB_TOKEN") - if token == "" { - return github.NewClient(github.WithEnterpriseURLs(apiURL, uploadURL)) - } - return github.NewClient(github.WithAuthToken(token), github.WithEnterpriseURLs(apiURL, uploadURL)) -} - type updateOpenAPICmd struct { Ref string `kong:"default=main,help=${openapi_ref_help}"` ValidateGithub bool `kong:"name=validate,help=${openapi_validate_help}"` @@ -203,16 +192,10 @@ func (c *unusedCmd) Run(root *rootCmd, k *kong.Context) error { return nil } -// defaultSchemaFieldExceptionsFile is the working-directory-relative path of the -// exceptions file loaded by check-schema-fields when --exceptions is not overridden. -const defaultSchemaFieldExceptionsFile = "tools/metadata/schema_field_exceptions.yaml" - type checkSchemaCmd struct { Ref string `kong:"help=${openapi_ref_default_help}"` Schemas []string `kong:"name=schema,help=${schema_filter_help}"` IncludeResponses bool `kong:"name=include-responses,help=${include_responses_help}"` - ExceptionsFile string `kong:"name=exceptions,default='tools/metadata/schema_field_exceptions.yaml',help=${schema_exceptions_help}"` - JSON bool `kong:"help=${output_json_help}"` Verbose bool `kong:"help='Print checked and skipped schema matches.'"` } @@ -230,7 +213,7 @@ func (c *checkSchemaCmd) Run(root *rootCmd, k *kong.Context) error { } } - client, err := publicGithubClient(root.GithubURL, root.UploadURL) + client, err := githubClient(root.GithubURL, root.UploadURL) if err != nil { return err } @@ -238,17 +221,14 @@ func (c *checkSchemaCmd) Run(root *rootCmd, k *kong.Context) error { if err != nil { return err } - exceptions, err := loadSchemaFieldExceptions( - filepath.Join(root.WorkingDir, c.ExceptionsFile), - c.ExceptionsFile == defaultSchemaFieldExceptionsFile, - ) + exceptions, err := loadSchemaFieldExceptions(root.WorkingDir) if err != nil { return err } result, err := checkSchemaFields(schemaFieldCheckOptions{ descriptions: descriptions, githubDir: filepath.Join(root.WorkingDir, "github"), - schemaNames: sliceSet(c.Schemas), + schemaNames: c.Schemas, includeResponses: c.IncludeResponses, exceptions: exceptions, }) @@ -256,27 +236,19 @@ func (c *checkSchemaCmd) Run(root *rootCmd, k *kong.Context) error { return err } - if c.JSON { - enc := json.NewEncoder(k.Stdout) - enc.SetIndent("", " ") - if err := enc.Encode(result); err != nil { - return err - } - } else { - fmt.Fprintf(k.Stdout, "Found %v schema field issues\n", len(result.Diagnostics)) - fmt.Fprintf(k.Stdout, "Checked %v OpenAPI schema/Go struct pairs; skipped %v OpenAPI schemas\n", result.Summary.Checked, result.Summary.Skipped) - if c.Verbose { - for _, checked := range result.Checked { - fmt.Fprintf(k.Stdout, "checked: %v -> %v (%v)\n", checked.OpenAPISchema, checked.GoStruct, checked.MatchReason) - } - for _, skipped := range result.Skipped { - fmt.Fprintf(k.Stdout, "skipped: %v (%v)\n", skipped.OpenAPISchema, skipped.Reason) - } + fmt.Fprintf(k.Stdout, "Found %v schema field issues\n", len(result.Diagnostics)) + fmt.Fprintf(k.Stdout, "Checked %v OpenAPI schema/Go struct pairs; skipped %v OpenAPI schemas\n", result.Summary.Checked, result.Summary.Skipped) + if c.Verbose { + for _, checked := range result.Checked { + fmt.Fprintf(k.Stdout, "checked: %v -> %v (%v)\n", checked.OpenAPISchema, checked.GoStruct, checked.MatchReason) } - for _, diag := range result.Diagnostics { - fmt.Fprintln(k.Stdout, diag.String()) + for _, skipped := range result.Skipped { + fmt.Fprintf(k.Stdout, "skipped: %v (%v)\n", skipped.OpenAPISchema, skipped.Reason) } } + 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)) } diff --git a/tools/metadata/schema_field_exceptions.yaml b/tools/metadata/schema_field_exceptions.yaml index a80b89e0d7a..b8b36085abd 100644 --- a/tools/metadata/schema_field_exceptions.yaml +++ b/tools/metadata/schema_field_exceptions.yaml @@ -1,11 +1,5 @@ -# schema_field_exceptions.yaml lists "Struct.Field" entries whose JSON field -# optionality intentionally deviates from the OpenAPI schema, so that their -# check-schema-fields diagnostics are suppressed. -# -# Each entry is a known deviation awaiting cleanup (for example a required field -# kept as a pointer pending a value-parameter refactor for #3644, or an optional -# field left as a value type). This mirrors how the paramcheck and structfield -# linters keep their allowlists in .golangci.yml. +# 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. # # TODO: fix these fields and remove the exceptions. exceptions: diff --git a/tools/metadata/schema_fields.go b/tools/metadata/schema_fields.go index f9d127d520b..14b198c721e 100644 --- a/tools/metadata/schema_fields.go +++ b/tools/metadata/schema_fields.go @@ -28,11 +28,11 @@ import ( type schemaFieldCheckOptions struct { descriptions []*openapiFile githubDir string - schemaNames map[string]bool + schemaNames []string includeResponses bool - // exceptions holds "Struct.Field" entries whose diagnostics are suppressed. It is - // loaded from schema_field_exceptions.yaml by the command; see loadSchemaFieldExceptions. - exceptions map[string]bool + // 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 { @@ -117,7 +117,7 @@ type goStructField struct { type openapiSchemaFields struct { openapiSchema string openapiFile string - required map[string]bool + required []string properties map[string]openapiSchemaProperty } @@ -133,23 +133,22 @@ type schemaFieldMatch struct { matchReason string } -// 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. +// 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 filename and -// returns them as a set. A missing file yields an empty set and no error when optional is -// true, so callers relying on the default path do not need the file to exist; an explicitly -// requested file that is missing is an error. -func loadSchemaFieldExceptions(filename string, optional bool) (map[string]bool, error) { +// 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 optional && errors.Is(err, fs.ErrNotExist) { - return map[string]bool{}, nil + if errors.Is(err, fs.ErrNotExist) { + return nil, nil } return nil, err } @@ -157,15 +156,14 @@ func loadSchemaFieldExceptions(filename string, optional bool) (map[string]bool, if err := yaml.Unmarshal(b, &exceptionsFile); err != nil { return nil, fmt.Errorf("%v: %w", filename, err) } - return sliceSet(exceptionsFile.Exceptions), nil + return exceptionsFile.Exceptions, nil } -// filterAllowedSchemaFieldDiagnostics removes diagnostics whose "Struct.Field" is listed in -// the exceptions set. -func filterAllowedSchemaFieldDiagnostics(diagnostics []*schemaFieldDiagnostic, exceptions map[string]bool) []*schemaFieldDiagnostic { +// 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 exceptions[diag.GoStruct+"."+diag.Field] { + if slices.Contains(exceptions, diag.GoStruct+"."+diag.Field) { continue } kept = append(kept, diag) @@ -244,7 +242,7 @@ func sortSchemaFieldResult(result *schemaFieldCheckResult) { }) } -func collectOpenAPISchemaFields(descriptions []*openapiFile, schemaNames map[string]bool) ([]*openapiSchemaFields, []*schemaFieldSkipped, error) { +func collectOpenAPISchemaFields(descriptions []*openapiFile, schemaNames []string) ([]*openapiSchemaFields, []*schemaFieldSkipped, error) { var schemas []*openapiSchemaFields var skipped []*schemaFieldSkipped seen := map[string]bool{} @@ -256,7 +254,7 @@ func collectOpenAPISchemaFields(descriptions []*openapiFile, schemaNames map[str names := make([]string, 0, len(desc.description.Components.Schemas)) for name := range desc.description.Components.Schemas { - if len(schemaNames) > 0 && !schemaNames[name] { + if len(schemaNames) > 0 && !slices.Contains(schemaNames, name) { continue } if seen[name] { @@ -290,13 +288,13 @@ func collectOpenAPISchemaFields(descriptions []*openapiFile, schemaNames map[str schemas = append(schemas, &openapiSchemaFields{ openapiSchema: name, openapiFile: desc.filename, - required: sliceSet(schema.Required), + required: schema.Required, properties: schemaProperties(schema.Properties), }) } } - for name := range schemaNames { + for _, name := range schemaNames { if !seen[name] { skipped = append(skipped, newSchemaFieldSkipped(name, "", "schema filter did not match an OpenAPI schema")) } @@ -325,7 +323,7 @@ func flattenObjectSchema(schema *openapi3.Schema) (*openapi3.Schema, string, err } merged := &openapi3.Schema{ - Required: append([]string{}, schema.Required...), + Required: slices.Clone(schema.Required), Properties: openapi3.Schemas{}, } maps.Copy(merged.Properties, schema.Properties) @@ -363,14 +361,6 @@ func schemaProperties(properties openapi3.Schemas) map[string]openapiSchemaPrope return result } -func sliceSet(values []string) map[string]bool { - set := make(map[string]bool, len(values)) - for _, value := range values { - set[value] = true - } - return set -} - func matchOpenAPISchemasToGoStructs(schemas []*openapiSchemaFields, goStructs map[string]*goStructInfo, requestStructs map[string]bool, allowSchemaNameMatch, includeResponses bool) ([]*schemaFieldMatch, []*schemaFieldSkipped) { var matches []*schemaFieldMatch var skipped []*schemaFieldSkipped @@ -395,9 +385,9 @@ func matchOpenAPISchemasToGoStructs(schemas []*openapiSchemaFields, goStructs ma return dropAmbiguousFieldSetMatches(matches, skipped) } -// dropAmbiguousFieldSetMatches removes exact-field-set matches for a Go struct that matched more -// than one OpenAPI schema. A field set that coincidentally equals several unrelated schemas (for -// example a generic {id, type}) is not a reliable match, so it is skipped rather than reported. +// dropAmbiguousFieldSetMatches removes exact-field-set matches for a Go struct that matched more than one +// OpenAPI schema. A field set that coincidentally equals several unrelated schemas (for example a generic +// {id, type}) is not a reliable match, so it is skipped rather than reported. func dropAmbiguousFieldSetMatches(matches []*schemaFieldMatch, skipped []*schemaFieldSkipped) ([]*schemaFieldMatch, []*schemaFieldSkipped) { fieldSetMatchCount := map[string]int{} for _, match := range matches { @@ -480,10 +470,9 @@ func matchByExactFieldSet(schema *openapiSchemaFields, goStructs map[string]*goS } } -// canCheckGoStruct reports whether goStruct should be compared against an OpenAPI schema. -// By default only request body structs are checked; requestStructs holds the names of -// structs used as the body argument of a mutating client.NewRequest call. Response and -// other structs are only checked when includeResponses is set. +// canCheckGoStruct reports whether goStruct should be compared against an OpenAPI schema. By default only +// request body structs are checked; requestStructs holds the names of structs used as the body argument of a +// mutating client.NewRequest call. Response and other structs are only checked when includeResponses is set. func canCheckGoStruct(goStruct *goStructInfo, requestStructs map[string]bool, includeResponses bool) bool { return includeResponses || requestStructs[goStruct.name] } @@ -506,32 +495,29 @@ func joinGoStructNames(goStructs []*goStructInfo) string { return strings.Join(names, ", ") } -// Thresholds for hasEnoughSharedFields. A schema-name match already agrees on the -// Go type name, so these guard only against a name that coincidentally collides with -// an unrelated struct. They are deliberately lenient: a wrong match is dropped later -// as ambiguous, but a missed match silently skips a real check. +// Thresholds for hasEnoughSharedFields. A schema-name match already agrees on the Go type name, so these +// guard only against a name that coincidentally collides with an unrelated struct. They are deliberately +// lenient: a wrong match is dropped later as ambiguous, but a missed match silently skips a real check. const ( - // minSharedFieldsForMatch accepts a match once this many JSON fields overlap, - // regardless of struct size: three shared, correctly named fields are unlikely - // to line up by chance. + // minSharedFieldsForMatch accepts a match once this many JSON fields overlap, regardless of struct + // size: three shared, correctly named fields are unlikely to line up by chance. minSharedFieldsForMatch = 3 - // minSharedFieldPercent is the fallback for structs smaller than - // minSharedFieldsForMatch: the overlap must cover at least this share of the - // smaller field set. Compared as shared*100 >= smallest*minSharedFieldPercent - // to stay in integer math. + // minSharedFieldPercent is the fallback for structs smaller than minSharedFieldsForMatch: the overlap + // must cover at least this share of the smaller field set. Compared as + // shared*100 >= smallest*minSharedFieldPercent to stay in integer math. minSharedFieldPercent = 60 ) -// hasEnoughSharedFields reports whether schema and goStruct share enough JSON fields to -// treat a schema-name match as genuine rather than a coincidental name collision. +// hasEnoughSharedFields reports whether schema and goStruct share enough JSON fields to treat a schema-name +// match as genuine rather than a coincidental name collision. func hasEnoughSharedFields(schema *openapiSchemaFields, goStruct *goStructInfo) bool { shared := sharedFieldCount(schema, goStruct) if shared == 0 { return false } smallest := min(len(schema.properties), len(goStruct.fields)) - // A type with one or two fields has too little signal for a percentage test, so - // require every field to line up before trusting the match. + // A type with one or two fields has too little signal for a percentage test, so require every field to + // line up before trusting the match. if smallest <= 2 { return shared == smallest } @@ -560,14 +546,12 @@ func sameJSONFieldSet(schema *openapiSchemaFields, goStruct *goStructInfo) bool return true } -// goInitialisms maps a lowercase OpenAPI name token to its idiomatic Go casing when -// building candidate struct names in goName. The structfield linter keeps a similar -// list (its `initialisms`/`specialCases`), but it lives in the separate -// github.com/google/go-github/v89/tools/structfield module and is keyed and shaped -// differently (uppercase-keyed set for a different purpose), so the two are -// intentionally not shared: unifying them would require a new shared module that both -// tools depend on. This list is deliberately small and only needs the initialisms that -// actually appear in OpenAPI schema names. +// goInitialisms maps a lowercase OpenAPI name token to its idiomatic Go casing when building candidate struct +// names in goName. The structfield linter keeps a similar list (its `initialisms`/`specialCases`), but it +// lives in the separate github.com/google/go-github/v89/tools/structfield module and is keyed and shaped +// differently (uppercase-keyed set for a different purpose), so the two are intentionally not shared: +// unifying them would require a new shared module that both tools depend on. This list is deliberately small +// and only needs the initialisms that actually appear in OpenAPI schema names. var goInitialisms = map[string]string{ "api": "API", "apis": "APIs", @@ -605,7 +589,7 @@ func goNameCandidates(openapiName string) []string { allSingular[i] = singular if singular != token { allChanged = true - variant := append([]string{}, tokens...) + variant := slices.Clone(tokens) variant[i] = singular variants = append(variants, variant) } @@ -697,7 +681,7 @@ func compareSchemaFields(match *schemaFieldMatch) []*schemaFieldDiagnostic { continue } - required := match.schema.required[jsonName] + 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")) @@ -801,16 +785,16 @@ func collectGoStructs(dir string) (map[string]*goStructInfo, map[string]bool, er if err != nil { return nil, nil, err } - // Exclude shared types that are also returned as a response; they follow the - // all-pointer response convention rather than the request-body convention. + // Exclude shared types that are also returned as a response; they follow the all-pointer response + // convention rather than the request-body convention. for name := range responseStructs { delete(requestStructs, name) } return structs, requestStructs, nil } -// collectRequestStructNames adds to requestStructs the name of the struct type passed as the -// body argument of every mutating client.NewRequest call in fn. +// collectRequestStructNames adds to requestStructs the name of the struct type passed as the body argument of +// every mutating client.NewRequest call in fn. func collectRequestStructNames(fn *ast.FuncDecl, requestStructs map[string]bool) { if fn.Body == nil { return @@ -829,8 +813,8 @@ func collectRequestStructNames(fn *ast.FuncDecl, requestStructs map[string]bool) }) } -// collectResponseStructNames adds to responseStructs the name of every struct type returned -// as a pointer (*T) or pointer slice ([]*T) by fn, which marks it as a response type. +// collectResponseStructNames adds to responseStructs the name of every struct type returned as a pointer (*T) +// or pointer slice ([]*T) by fn, which marks it as a response type. func collectResponseStructNames(fn *ast.FuncDecl, responseStructs map[string]bool) { if fn.Type.Results == nil { return @@ -871,7 +855,8 @@ func isClientNewRequest(call *ast.CallExpr) bool { } } -// isMutatingNewRequest reports whether call's method argument is "PATCH", "POST", or "PUT" and a body argument is present. +// isMutatingNewRequest reports whether call's method argument is "PATCH", "POST", or "PUT" and a body +// argument is present. func isMutatingNewRequest(call *ast.CallExpr) bool { if len(call.Args) < 4 { return false @@ -888,8 +873,8 @@ func isMutatingNewRequest(call *ast.CallExpr) bool { } } -// requestBodyStructName returns the struct type name of a client.NewRequest body argument, -// resolving a function parameter to its declared type or a composite literal to its type. +// requestBodyStructName returns the struct type name of a client.NewRequest body argument, resolving a +// function parameter to its declared type or a composite literal to its type. func requestBodyStructName(fn *ast.FuncDecl, arg ast.Expr) string { switch a := arg.(type) { case *ast.Ident: @@ -920,7 +905,8 @@ func findFuncParam(fn *ast.FuncDecl, name string) *ast.Field { return nil } -// exprTypeName returns the base type name of expr, unwrapping a pointer and resolving a qualified (pkg.Type) selector. +// exprTypeName returns the base type name of expr, unwrapping a pointer and resolving a qualified (pkg.Type) +// selector. func exprTypeName(expr ast.Expr) string { switch t := expr.(type) { case *ast.StarExpr: @@ -947,7 +933,7 @@ func collectFieldsForStruct(fset *token.FileSet, filename, structName string, st info := goStructField{ goStruct: structName, field: name.Name, - jsonName: defaultJSONName(name.Name), + jsonName: name.Name, isPointer: isPointerType(field.Type), canBeOmitted: canBeOmitted(field.Type), filename: filename, @@ -990,10 +976,6 @@ func parseJSONTag(tag string) (name string, hasOmitOption, ignored bool) { return name, hasOmitOption, false } -func defaultJSONName(name string) string { - return name -} - func isPointerType(expr ast.Expr) bool { _, ok := expr.(*ast.StarExpr) return ok diff --git a/tools/metadata/schema_fields_test.go b/tools/metadata/schema_fields_test.go index b849f1c8a91..be9d88dcb1c 100644 --- a/tools/metadata/schema_fields_test.go +++ b/tools/metadata/schema_fields_test.go @@ -50,10 +50,8 @@ type Demo struct { }, }), })}, - githubDir: githubDir, - schemaNames: sliceSet([]string{ - "demo", - }), + githubDir: githubDir, + schemaNames: []string{"demo"}, }) if err != nil { t.Fatal(err) @@ -214,7 +212,7 @@ type NullableDemo struct { }), })}, githubDir: githubDir, - schemaNames: sliceSet([]string{"nullable-demo"}), + schemaNames: []string{"nullable-demo"}, }) if err != nil { t.Fatal(err) @@ -399,18 +397,6 @@ func TestIsPointerTypeAndCanBeOmitted(t *testing.T) { } } -func TestSliceSet(t *testing.T) { - t.Parallel() - got := sliceSet([]string{"a", "b", "a"}) - want := map[string]bool{"a": true, "b": true} - if diff := cmp.Diff(want, got); diff != "" { - t.Errorf("sliceSet mismatch (-want +got):\n%v", diff) - } - if got := sliceSet(nil); len(got) != 0 { - t.Errorf("sliceSet(nil) = %v, want empty", got) - } -} - func TestDiagLocation(t *testing.T) { t.Parallel() tests := []struct { @@ -724,7 +710,7 @@ func (s *svc) Add(ctx context.Context, body *ItemRequest) { func TestFilterAllowedSchemaFieldDiagnostics(t *testing.T) { t.Parallel() - exceptions := sliceSet([]string{"ExemptStruct.ExemptField"}) + exceptions := []string{"ExemptStruct.ExemptField"} got := filterAllowedSchemaFieldDiagnostics([]*schemaFieldDiagnostic{ {GoStruct: "ExemptStruct", Field: "ExemptField"}, {GoStruct: "NotExemptStruct", Field: "NotExemptField"}, @@ -737,35 +723,32 @@ func TestFilterAllowedSchemaFieldDiagnostics(t *testing.T) { func TestLoadSchemaFieldExceptions(t *testing.T) { t.Parallel() dir := t.TempDir() - path := filepath.Join(dir, "schema_field_exceptions.yaml") - writeFile(t, path, `# comment + 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(path, false) + got, err := loadSchemaFieldExceptions(dir) if err != nil { t.Fatal(err) } - want := map[string]bool{"StructA.FieldA": true, "StructB.FieldB": true} + want := []string{"StructA.FieldA", "StructB.FieldB"} if diff := cmp.Diff(want, got); diff != "" { t.Errorf("loadSchemaFieldExceptions mismatch (-want +got):\n%v", diff) } - // A missing optional file yields an empty set and no error. - missing := filepath.Join(dir, "does-not-exist.yaml") - got, err = loadSchemaFieldExceptions(missing, true) + // A missing file yields no exceptions and no error. + got, err = loadSchemaFieldExceptions(t.TempDir()) if err != nil { - t.Fatalf("optional missing file: unexpected error %v", err) + t.Fatalf("missing file: unexpected error %v", err) } if len(got) != 0 { - t.Errorf("optional missing file: got %v, want empty", got) - } - - // A missing required file is an error. - if _, err := loadSchemaFieldExceptions(missing, false); err == nil { - t.Error("required missing file: got nil error, want error") + t.Errorf("missing file: got %v, want empty", got) } } @@ -773,13 +756,13 @@ exceptions: // 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 committed - // file (tools/metadata/schema_field_exceptions.yaml) is reachable by its basename. - got, err := loadSchemaFieldExceptions("schema_field_exceptions.yaml", false) + // 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 { + for _, key := range got { if _, _, ok := strings.Cut(key, "."); !ok { t.Errorf("exception %q is not in Struct.Field form", key) } From ccd7712110fcd8c1dad8985abc340be6d6c546b2 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Sat, 29 Aug 2026 09:53:29 +0900 Subject: [PATCH 5/6] metadata: resolve schema checks via explicit //meta:schema annotations Replace the fuzzy schema-to-struct matching (name heuristics, exact field-set matching, ambiguity dropping, and match thresholds) with explicit opt-in annotations: a struct doc comment carries one "//meta:schema " line per operation whose body schema it must match, mirroring the //meta:operation convention. Only annotated structs are checked, annotations that do not resolve to an operation in the OpenAPI descriptions are reported as issues, and the field-level comparison is unchanged. The magic match thresholds and the duplicated initialisms list are removed along with the matcher, and the stale exception entries are dropped now that unannotated structs are simply not checked. --- CONTRIBUTING.md | 43 +- tools/metadata/main.go | 31 +- tools/metadata/schema_field_exceptions.yaml | 17 +- tools/metadata/schema_fields.go | 858 ++++++------------ tools/metadata/schema_fields_test.go | 615 ++++++------- .../check-schema-fields/github/demo.go | 7 +- 6 files changed, 565 insertions(+), 1006 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 610939923cf..0a68f4cb6b8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -652,34 +652,35 @@ Its subcommands are: - `unused` - lists operations from `openapi_operations.yaml` that are not mapped from any methods. -- `check-schema-fields` - automatically matches GitHub's OpenAPI component - schemas to Go request structs when the JSON field set makes the match - unambiguous, then reports JSON field optionality mismatches. Ambiguous or - unsupported schemas are skipped instead of configured with per-schema - exceptions. It can be used to check whether required, non-nullable schema - fields are represented as non-pointer fields without `omitempty` or - `omitzero`, and whether optional schema fields remain omittable in Go. For - example: - - ```sh - script/metadata.sh check-schema-fields +- `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 { ``` - To experiment with one schema while refactoring, pass `--schema` with the - OpenAPI schema name. Filtered schemas also allow high-confidence schema-name - matches and response structs so the command can report the current - differences before the JSON field set is fully aligned: + 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 --schema repository-ruleset --verbose + script/metadata.sh check-schema-fields ``` - Use `--include-responses` to inspect response structs in bulk. This is useful - for measuring drift, but response required fields are treated more cautiously - than request bodies in this project. + 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 intentionally deviate from the OpenAPI schema (for example a - required field kept as a pointer pending a value-parameter refactor). These are + 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 diff --git a/tools/metadata/main.go b/tools/metadata/main.go index 1889e551324..52de5aa013f 100644 --- a/tools/metadata/main.go +++ b/tools/metadata/main.go @@ -36,17 +36,15 @@ 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. By default, the check automatically -matches OpenAPI component schemas to Go request structs only when the JSON field set makes the match unambiguous. -Use --schema to try one or more OpenAPI schema names; filtered schemas also allow high-confidence schema-name -matches and response structs to make refactoring experiments easier. +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.`, "openapi_ref_default_help": `Git ref to pull OpenAPI descriptions from. Defaults to openapi_commit from openapi_operations.yaml.`, - "schema_filter_help": `OpenAPI schema name to check. May be repeated. Defaults to all automatically matched schemas.`, - "include_responses_help": `Also check response structs. By default only request structs are checked unless --schema is provided.`, "openapi_validate_help": ` Instead of updating, make sure that the operations in openapi_operations.yaml's "openapi_operations" field are @@ -193,10 +191,8 @@ func (c *unusedCmd) Run(root *rootCmd, k *kong.Context) error { } type checkSchemaCmd struct { - Ref string `kong:"help=${openapi_ref_default_help}"` - Schemas []string `kong:"name=schema,help=${schema_filter_help}"` - IncludeResponses bool `kong:"name=include-responses,help=${include_responses_help}"` - Verbose bool `kong:"help='Print checked and skipped schema matches.'"` + 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 { @@ -226,24 +222,19 @@ func (c *checkSchemaCmd) Run(root *rootCmd, k *kong.Context) error { return err } result, err := checkSchemaFields(schemaFieldCheckOptions{ - descriptions: descriptions, - githubDir: filepath.Join(root.WorkingDir, "github"), - schemaNames: c.Schemas, - includeResponses: c.IncludeResponses, - exceptions: exceptions, + 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 OpenAPI schema/Go struct pairs; skipped %v OpenAPI schemas\n", result.Summary.Checked, result.Summary.Skipped) + 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 (%v)\n", checked.OpenAPISchema, checked.GoStruct, checked.MatchReason) - } - for _, skipped := range result.Skipped { - fmt.Fprintf(k.Stdout, "skipped: %v (%v)\n", skipped.OpenAPISchema, skipped.Reason) + fmt.Fprintf(k.Stdout, "checked: %v -> %v\n", checked.GoStruct, checked.Annotation) } } for _, diag := range result.Diagnostics { diff --git a/tools/metadata/schema_field_exceptions.yaml b/tools/metadata/schema_field_exceptions.yaml index b8b36085abd..55fa9db22ea 100644 --- a/tools/metadata/schema_field_exceptions.yaml +++ b/tools/metadata/schema_field_exceptions.yaml @@ -1,15 +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. -# -# TODO: fix these fields and remove the exceptions. -exceptions: - - DependencyGraphSnapshot.Detector - - DependencyGraphSnapshot.Job - - DependencyGraphSnapshot.Ref - - DependencyGraphSnapshot.Scanned - - DependencyGraphSnapshot.Sha - - DeploymentBranchPolicyRequest.Name - - ReviewCustomDeploymentProtectionRuleRequest.Comment - - WorkflowsPermissionsOpt.RequireApprovalForForkPRWorkflows - - WorkflowsPermissionsOpt.SendSecretsAndVariables - - WorkflowsPermissionsOpt.SendWriteTokensToWorkflows +# 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 index 14b198c721e..092a60218ed 100644 --- a/tools/metadata/schema_fields.go +++ b/tools/metadata/schema_fields.go @@ -17,61 +17,50 @@ import ( "os" "path/filepath" "reflect" + "regexp" "slices" "strings" - "unicode" "github.com/getkin/kin-openapi/openapi3" "go.yaml.in/yaml/v3" ) type schemaFieldCheckOptions struct { - descriptions []*openapiFile - githubDir string - schemaNames []string - includeResponses bool + 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 `json:"summary"` - Checked []*schemaFieldChecked `json:"checked"` - Skipped []*schemaFieldSkipped `json:"skipped,omitempty"` - Diagnostics []*schemaFieldDiagnostic `json:"diagnostics"` + Summary schemaFieldCheckSummary + Checked []*schemaFieldChecked + Diagnostics []*schemaFieldDiagnostic } type schemaFieldCheckSummary struct { - OpenAPISchemas int `json:"openapi_schemas"` - GoStructs int `json:"go_structs"` - Checked int `json:"checked"` - Skipped int `json:"skipped"` - Diagnostics int `json:"diagnostics"` + GoStructs int + AnnotatedStructs int + Checked int + Diagnostics int } type schemaFieldChecked struct { - OpenAPISchema string `json:"openapi_schema"` - GoStruct string `json:"go_struct"` - OpenAPIFile string `json:"openapi_file,omitempty"` - MatchReason string `json:"match_reason"` -} - -type schemaFieldSkipped struct { - OpenAPISchema string `json:"openapi_schema"` - OpenAPIFile string `json:"openapi_file,omitempty"` - Reason string `json:"reason"` + Annotation string + GoStruct string + OpenAPIFile string } type schemaFieldDiagnostic struct { - OpenAPISchema string `json:"openapi_schema"` - GoStruct string `json:"go_struct"` - Field string `json:"field"` - JSONName string `json:"json_name"` - Message string `json:"message"` - Filename string `json:"filename,omitempty"` - Line int `json:"line,omitempty"` - OpenAPIFile string `json:"openapi_file,omitempty"` + Annotation string + GoStruct string + Field string + JSONName string + Message string + Filename string + Line int + OpenAPIFile string } func (d schemaFieldDiagnostic) String() string { @@ -83,7 +72,14 @@ func (d schemaFieldDiagnostic) String() string { if d.OpenAPIFile != "" { source = fmt.Sprintf(" [%v]", d.OpenAPIFile) } - return fmt.Sprintf("%v%v.%v (%v from %v): %v%v", loc, d.GoStruct, d.Field, d.JSONName, d.OpenAPISchema, d.Message, source) + 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 { @@ -96,11 +92,36 @@ func diagLocation(filename string, line int) string { return fmt.Sprintf("%v:%v", filename, line) } -type goStructInfo struct { - name string +// 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 - fields map[string]goStructField +} + +type goStructInfo struct { + name string + filename string + line int + fields map[string]goStructField + annotations []*schemaAnnotation + annotationProblems []*schemaAnnotationProblem } type goStructField struct { @@ -115,10 +136,10 @@ type goStructField struct { } type openapiSchemaFields struct { - openapiSchema string - openapiFile string - required []string - properties map[string]openapiSchemaProperty + annotation string + openapiFile string + required []string + properties map[string]openapiSchemaProperty } type openapiSchemaProperty struct { @@ -128,9 +149,8 @@ type openapiSchemaProperty struct { } type schemaFieldMatch struct { - schema *openapiSchemaFields - goStruct *goStructInfo - matchReason string + schema *openapiSchemaFields + goStruct *goStructInfo } // schemaFieldExceptionsFile is the on-disk format of schema_field_exceptions.yaml: a list of "Struct.Field" @@ -171,45 +191,87 @@ func filterAllowedSchemaFieldDiagnostics(diagnostics []*schemaFieldDiagnostic, e 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, requestStructs, err := collectGoStructs(opts.githubDir) + goStructs, err := collectGoStructs(opts.githubDir) if err != nil { return schemaFieldCheckResult{}, err } - schemas, skipped, err := collectOpenAPISchemaFields(opts.descriptions, opts.schemaNames) - 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++ - includeResponses := opts.includeResponses || len(opts.schemaNames) > 0 - matches, matchSkipped := matchOpenAPISchemasToGoStructs(schemas, goStructs, requestStructs, len(opts.schemaNames) > 0, includeResponses) - result := schemaFieldCheckResult{ - Skipped: append(skipped, matchSkipped...), - Summary: schemaFieldCheckSummary{ - OpenAPISchemas: len(schemas), - GoStructs: len(goStructs), - }, - } + 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 _, match := range matches { - result.Checked = append(result.Checked, &schemaFieldChecked{ - OpenAPISchema: match.schema.openapiSchema, - GoStruct: match.goStruct.name, - OpenAPIFile: match.schema.openapiFile, - MatchReason: match.matchReason, - }) - result.Diagnostics = append(result.Diagnostics, compareSchemaFields(match)...) + 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.Skipped = len(result.Skipped) result.Summary.Diagnostics = len(result.Diagnostics) return result, nil } @@ -220,95 +282,78 @@ func sortSchemaFieldResult(result *schemaFieldCheckResult) { cmp.Compare(a.GoStruct, b.GoStruct), cmp.Compare(a.JSONName, b.JSONName), cmp.Compare(a.Field, b.Field), - cmp.Compare(a.OpenAPISchema, b.OpenAPISchema), + 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.OpenAPISchema, b.OpenAPISchema), cmp.Compare(a.GoStruct, b.GoStruct), + cmp.Compare(a.Annotation, b.Annotation), cmp.Compare(a.OpenAPIFile, b.OpenAPIFile), - cmp.Compare(a.MatchReason, b.MatchReason), - ) - }) - slices.SortFunc(result.Skipped, func(a, b *schemaFieldSkipped) int { - return cmp.Or( - cmp.Compare(a.OpenAPISchema, b.OpenAPISchema), - cmp.Compare(a.OpenAPIFile, b.OpenAPIFile), - cmp.Compare(a.Reason, b.Reason), ) }) } -func collectOpenAPISchemaFields(descriptions []*openapiFile, schemaNames []string) ([]*openapiSchemaFields, []*schemaFieldSkipped, error) { - var schemas []*openapiSchemaFields - var skipped []*schemaFieldSkipped - seen := map[string]bool{} - +// 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 || desc.description.Components == nil || desc.description.Components.Schemas == nil { + if desc.description == nil { continue } - - names := make([]string, 0, len(desc.description.Components.Schemas)) - for name := range desc.description.Components.Schemas { - if len(schemaNames) > 0 && !slices.Contains(schemaNames, name) { + for path, pathItem := range desc.description.Paths.Map() { + if pathItem == nil || normalizeOpPath(path) != normPath { continue } - if seen[name] { + op := pathItem.Operations()[ann.method] + if op == nil { continue } - names = append(names, name) + schema, problem = annotationSchema(op, ann.role) + return schema, desc.filename, problem } - slices.Sort(names) - - for _, name := range names { - seen[name] = true - schemaRef := desc.description.Components.Schemas[name] - if schemaRef == nil || schemaRef.Value == nil { - skipped = append(skipped, newSchemaFieldSkipped(name, desc.filename, "schema reference is unresolved")) - continue - } + } + return nil, "", fmt.Sprintf("could not find operation %v %v in any OpenAPI description", ann.method, ann.path) +} - schema, reason, err := flattenObjectSchema(schemaRef.Value) - if err != nil { - return nil, nil, fmt.Errorf("%v %v: %w", desc.filename, name, err) - } - if reason != "" { - skipped = append(skipped, newSchemaFieldSkipped(name, desc.filename, reason)) +// 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 len(schema.Properties) == 0 { - skipped = append(skipped, newSchemaFieldSkipped(name, desc.filename, "schema has no object properties")) - continue + if schema, problem := jsonContentSchema(responses[code].Value.Content, ""); problem == "" { + return schema, "" } - - schemas = append(schemas, &openapiSchemaFields{ - openapiSchema: name, - openapiFile: desc.filename, - required: schema.Required, - properties: schemaProperties(schema.Properties), - }) } + return nil, "operation has no 2xx response with an application/json schema" } - - for _, name := range schemaNames { - if !seen[name] { - skipped = append(skipped, newSchemaFieldSkipped(name, "", "schema filter did not match an OpenAPI schema")) - } - } - - return schemas, skipped, nil } -func newSchemaFieldSkipped(schemaName, filename, reason string) *schemaFieldSkipped { - return &schemaFieldSkipped{ - OpenAPISchema: schemaName, - OpenAPIFile: filename, - Reason: reason, +// 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) { @@ -361,314 +406,6 @@ func schemaProperties(properties openapi3.Schemas) map[string]openapiSchemaPrope return result } -func matchOpenAPISchemasToGoStructs(schemas []*openapiSchemaFields, goStructs map[string]*goStructInfo, requestStructs map[string]bool, allowSchemaNameMatch, includeResponses bool) ([]*schemaFieldMatch, []*schemaFieldSkipped) { - var matches []*schemaFieldMatch - var skipped []*schemaFieldSkipped - - for _, schema := range schemas { - if allowSchemaNameMatch { - if match, ok, reason := matchBySchemaName(schema, goStructs, requestStructs, includeResponses); ok { - matches = append(matches, match) - continue - } else if reason != "" { - skipped = append(skipped, newSchemaFieldSkipped(schema.openapiSchema, schema.openapiFile, reason)) - continue - } - } - if match, ok, reason := matchByExactFieldSet(schema, goStructs, requestStructs, includeResponses); ok { - matches = append(matches, match) - } else { - skipped = append(skipped, newSchemaFieldSkipped(schema.openapiSchema, schema.openapiFile, reason)) - } - } - - return dropAmbiguousFieldSetMatches(matches, skipped) -} - -// dropAmbiguousFieldSetMatches removes exact-field-set matches for a Go struct that matched more than one -// OpenAPI schema. A field set that coincidentally equals several unrelated schemas (for example a generic -// {id, type}) is not a reliable match, so it is skipped rather than reported. -func dropAmbiguousFieldSetMatches(matches []*schemaFieldMatch, skipped []*schemaFieldSkipped) ([]*schemaFieldMatch, []*schemaFieldSkipped) { - fieldSetMatchCount := map[string]int{} - for _, match := range matches { - if match.matchReason == "exact JSON field set" { - fieldSetMatchCount[match.goStruct.name]++ - } - } - - var kept []*schemaFieldMatch - for _, match := range matches { - if match.matchReason == "exact JSON field set" && fieldSetMatchCount[match.goStruct.name] > 1 { - skipped = append(skipped, newSchemaFieldSkipped(match.schema.openapiSchema, match.schema.openapiFile, - "Go struct "+match.goStruct.name+" matches multiple schemas by field set")) - continue - } - kept = append(kept, match) - } - return kept, skipped -} - -func matchBySchemaName(schema *openapiSchemaFields, goStructs map[string]*goStructInfo, requestStructs map[string]bool, includeResponses bool) (*schemaFieldMatch, bool, string) { - var matches []*goStructInfo - for _, name := range goNameCandidates(schema.openapiSchema) { - goStruct, ok := goStructs[name] - if !ok { - continue - } - if !canCheckGoStruct(goStruct, requestStructs, includeResponses) { - continue - } - if !hasEnoughSharedFields(schema, goStruct) { - continue - } - matches = appendUniqueGoStruct(matches, goStruct) - } - - switch len(matches) { - case 0: - return nil, false, "" - case 1: - return &schemaFieldMatch{ - schema: schema, - goStruct: matches[0], - matchReason: "schema name", - }, true, "" - default: - return nil, false, "ambiguous Go struct name match: " + joinGoStructNames(matches) - } -} - -func matchByExactFieldSet(schema *openapiSchemaFields, goStructs map[string]*goStructInfo, requestStructs map[string]bool, includeResponses bool) (*schemaFieldMatch, bool, string) { - if len(schema.properties) < 2 { - return nil, false, "no unambiguous Go struct match" - } - - var matches []*goStructInfo - for _, goStruct := range goStructs { - if !canCheckGoStruct(goStruct, requestStructs, includeResponses) { - continue - } - if sameJSONFieldSet(schema, goStruct) { - matches = append(matches, goStruct) - } - } - - switch len(matches) { - case 0: - return nil, false, "no unambiguous Go struct match" - case 1: - return &schemaFieldMatch{ - schema: schema, - goStruct: matches[0], - matchReason: "exact JSON field set", - }, true, "" - default: - slices.SortFunc(matches, func(a, b *goStructInfo) int { - return cmp.Compare(a.name, b.name) - }) - return nil, false, "ambiguous Go struct field-set match: " + joinGoStructNames(matches) - } -} - -// canCheckGoStruct reports whether goStruct should be compared against an OpenAPI schema. By default only -// request body structs are checked; requestStructs holds the names of structs used as the body argument of a -// mutating client.NewRequest call. Response and other structs are only checked when includeResponses is set. -func canCheckGoStruct(goStruct *goStructInfo, requestStructs map[string]bool, includeResponses bool) bool { - return includeResponses || requestStructs[goStruct.name] -} - -func appendUniqueGoStruct(matches []*goStructInfo, goStruct *goStructInfo) []*goStructInfo { - for _, existing := range matches { - if existing.name == goStruct.name { - return matches - } - } - return append(matches, goStruct) -} - -func joinGoStructNames(goStructs []*goStructInfo) string { - names := make([]string, 0, len(goStructs)) - for _, goStruct := range goStructs { - names = append(names, goStruct.name) - } - slices.Sort(names) - return strings.Join(names, ", ") -} - -// Thresholds for hasEnoughSharedFields. A schema-name match already agrees on the Go type name, so these -// guard only against a name that coincidentally collides with an unrelated struct. They are deliberately -// lenient: a wrong match is dropped later as ambiguous, but a missed match silently skips a real check. -const ( - // minSharedFieldsForMatch accepts a match once this many JSON fields overlap, regardless of struct - // size: three shared, correctly named fields are unlikely to line up by chance. - minSharedFieldsForMatch = 3 - // minSharedFieldPercent is the fallback for structs smaller than minSharedFieldsForMatch: the overlap - // must cover at least this share of the smaller field set. Compared as - // shared*100 >= smallest*minSharedFieldPercent to stay in integer math. - minSharedFieldPercent = 60 -) - -// hasEnoughSharedFields reports whether schema and goStruct share enough JSON fields to treat a schema-name -// match as genuine rather than a coincidental name collision. -func hasEnoughSharedFields(schema *openapiSchemaFields, goStruct *goStructInfo) bool { - shared := sharedFieldCount(schema, goStruct) - if shared == 0 { - return false - } - smallest := min(len(schema.properties), len(goStruct.fields)) - // A type with one or two fields has too little signal for a percentage test, so require every field to - // line up before trusting the match. - if smallest <= 2 { - return shared == smallest - } - return shared >= minSharedFieldsForMatch || shared*100 >= smallest*minSharedFieldPercent -} - -func sharedFieldCount(schema *openapiSchemaFields, goStruct *goStructInfo) int { - var shared int - for name := range schema.properties { - if _, ok := goStruct.fields[name]; ok { - shared++ - } - } - return shared -} - -func sameJSONFieldSet(schema *openapiSchemaFields, goStruct *goStructInfo) bool { - if len(schema.properties) != len(goStruct.fields) { - return false - } - for name := range schema.properties { - if _, ok := goStruct.fields[name]; !ok { - return false - } - } - return true -} - -// goInitialisms maps a lowercase OpenAPI name token to its idiomatic Go casing when building candidate struct -// names in goName. The structfield linter keeps a similar list (its `initialisms`/`specialCases`), but it -// lives in the separate github.com/google/go-github/v89/tools/structfield module and is keyed and shaped -// differently (uppercase-keyed set for a different purpose), so the two are intentionally not shared: -// unifying them would require a new shared module that both tools depend on. This list is deliberately small -// and only needs the initialisms that actually appear in OpenAPI schema names. -var goInitialisms = map[string]string{ - "api": "API", - "apis": "APIs", - "gpg": "GPG", - "html": "HTML", - "http": "HTTP", - "https": "HTTPS", - "id": "ID", - "ids": "IDs", - "ip": "IP", - "ips": "IPs", - "oauth": "OAuth", - "oidc": "OIDC", - "scim": "SCIM", - "sms": "SMS", - "sso": "SSO", - "ssh": "SSH", - "totp": "TOTP", - "url": "URL", - "urls": "URLs", - "webhook": "Webhook", -} - -func goNameCandidates(openapiName string) []string { - tokens := splitOpenAPIName(openapiName) - if len(tokens) == 0 { - return nil - } - - variants := [][]string{tokens} - allSingular := make([]string, len(tokens)) - var allChanged bool - for i, token := range tokens { - singular := singularize(token) - allSingular[i] = singular - if singular != token { - allChanged = true - variant := slices.Clone(tokens) - variant[i] = singular - variants = append(variants, variant) - } - } - if allChanged { - variants = append(variants, allSingular) - } - - var names []string - seen := map[string]bool{} - for _, variant := range variants { - name := goName(variant) - if name == "" || seen[name] { - continue - } - seen[name] = true - names = append(names, name) - } - return names -} - -func splitOpenAPIName(name string) []string { - return strings.FieldsFunc(name, func(r rune) bool { - return !unicode.IsLetter(r) && !unicode.IsDigit(r) - }) -} - -func singularize(token string) string { - lower := strings.ToLower(token) - switch { - case strings.HasSuffix(lower, "ies") && len(token) > 3: - return token[:len(token)-3] + "y" - case strings.HasSuffix(lower, "statuses"): - return token[:len(token)-2] - case strings.HasSuffix(lower, "ches") || strings.HasSuffix(lower, "shes") || strings.HasSuffix(lower, "xes") || strings.HasSuffix(lower, "ses"): - return token[:len(token)-2] - case strings.HasSuffix(lower, "s") && !strings.HasSuffix(lower, "ss") && len(token) > 1: - return token[:len(token)-1] - default: - return token - } -} - -func goName(tokens []string) string { - var b strings.Builder - for _, token := range tokens { - if token == "" { - continue - } - lower := strings.ToLower(token) - if initialism, ok := goInitialisms[lower]; ok { - b.WriteString(initialism) - continue - } - if isVersionToken(lower) { - b.WriteString(strings.ToUpper(lower[:1])) - b.WriteString(lower[1:]) - continue - } - b.WriteString(strings.ToUpper(token[:1])) - if len(token) > 1 { - b.WriteString(strings.ToLower(token[1:])) - } - } - return b.String() -} - -func isVersionToken(token string) bool { - if len(token) < 2 || token[0] != 'v' { - return false - } - for _, r := range token[1:] { - if !unicode.IsDigit(r) { - return false - } - } - return true -} - func compareSchemaFields(match *schemaFieldMatch) []*schemaFieldDiagnostic { var diagnostics []*schemaFieldDiagnostic for jsonName, field := range match.goStruct.fields { @@ -702,12 +439,12 @@ func compareSchemaFields(match *schemaFieldMatch) []*schemaFieldDiagnostic { continue } diagnostics = append(diagnostics, &schemaFieldDiagnostic{ - OpenAPISchema: match.schema.openapiSchema, - GoStruct: match.goStruct.name, - JSONName: propName, - Field: propName, - Message: "OpenAPI schema property is missing from the Go struct", - OpenAPIFile: match.schema.openapiFile, + 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 @@ -719,27 +456,70 @@ func (p openapiSchemaProperty) canCheckOptionality() bool { func newSchemaFieldDiagnostic(match *schemaFieldMatch, field goStructField, message string) *schemaFieldDiagnostic { return &schemaFieldDiagnostic{ - OpenAPISchema: match.schema.openapiSchema, - GoStruct: match.goStruct.name, - Field: field.field, - JSONName: field.jsonName, - Message: message, - Filename: field.filename, - Line: field.line, - OpenAPIFile: match.schema.openapiFile, - } -} - -// collectGoStructs parses the Go source files in dir and returns every exported struct by -// name along with the set of struct types used exclusively as request bodies. A request body -// is the type of the body argument passed to a mutating (POST, PUT, or PATCH) client.NewRequest -// call; types that are also returned as a response (for example shared types like Label) are -// excluded because they follow the all-pointer response convention rather than the request-body -// convention. -func collectGoStructs(dir string) (map[string]*goStructInfo, map[string]bool, error) { + 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{} - requestStructs := map[string]bool{} - responseStructs := map[string]bool{} fset := token.NewFileSet() err := filepath.WalkDir(dir, func(filename string, d fs.DirEntry, err error) error { if err != nil || d.IsDir() { @@ -749,16 +529,11 @@ func collectGoStructs(dir string) (map[string]*goStructInfo, map[string]bool, er return nil } - fileNode, err := parser.ParseFile(fset, filename, nil, parser.SkipObjectResolution) + fileNode, err := parser.ParseFile(fset, filename, nil, parser.ParseComments|parser.SkipObjectResolution) if err != nil { return err } for _, decl := range fileNode.Decls { - if fn, ok := decl.(*ast.FuncDecl); ok { - collectRequestStructNames(fn, requestStructs) - collectResponseStructNames(fn, responseStructs) - continue - } gen, ok := decl.(*ast.GenDecl) if !ok || gen.Tok != token.TYPE { continue @@ -772,152 +547,27 @@ func collectGoStructs(dir string) (map[string]*goStructInfo, map[string]bool, er 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), + 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, nil, err - } - // Exclude shared types that are also returned as a response; they follow the all-pointer response - // convention rather than the request-body convention. - for name := range responseStructs { - delete(requestStructs, name) - } - return structs, requestStructs, nil -} - -// collectRequestStructNames adds to requestStructs the name of the struct type passed as the body argument of -// every mutating client.NewRequest call in fn. -func collectRequestStructNames(fn *ast.FuncDecl, requestStructs map[string]bool) { - if fn.Body == nil { - return - } - ast.Inspect(fn.Body, func(n ast.Node) bool { - call, ok := n.(*ast.CallExpr) - if !ok { - return true - } - if isClientNewRequest(call) && isMutatingNewRequest(call) { - if name := requestBodyStructName(fn, call.Args[3]); name != "" { - requestStructs[name] = true - } - } - return true - }) -} - -// collectResponseStructNames adds to responseStructs the name of every struct type returned as a pointer (*T) -// or pointer slice ([]*T) by fn, which marks it as a response type. -func collectResponseStructNames(fn *ast.FuncDecl, responseStructs map[string]bool) { - if fn.Type.Results == nil { - return - } - for _, field := range fn.Type.Results.List { - if name := responseStructName(field.Type); name != "" { - responseStructs[name] = true - } - } -} - -// responseStructName returns the struct name of a *T or []*T result type, or "" otherwise. -func responseStructName(expr ast.Expr) string { - switch t := expr.(type) { - case *ast.StarExpr: - if ident, ok := t.X.(*ast.Ident); ok { - return ident.Name - } - case *ast.ArrayType: - return responseStructName(t.Elt) - } - return "" -} - -// isClientNewRequest reports whether call is of the form x.client.NewRequest(...) or client.NewRequest(...). -func isClientNewRequest(call *ast.CallExpr) bool { - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok || sel.Sel.Name != "NewRequest" { - return false - } - switch x := sel.X.(type) { - case *ast.SelectorExpr: - return x.Sel.Name == "client" - case *ast.Ident: - return x.Name == "client" - default: - return false - } -} - -// isMutatingNewRequest reports whether call's method argument is "PATCH", "POST", or "PUT" and a body -// argument is present. -func isMutatingNewRequest(call *ast.CallExpr) bool { - if len(call.Args) < 4 { - return false - } - lit, ok := call.Args[1].(*ast.BasicLit) - if !ok || lit.Kind != token.STRING { - return false - } - switch lit.Value { - case `"PATCH"`, `"POST"`, `"PUT"`: - return true - default: - return false - } -} - -// requestBodyStructName returns the struct type name of a client.NewRequest body argument, resolving a -// function parameter to its declared type or a composite literal to its type. -func requestBodyStructName(fn *ast.FuncDecl, arg ast.Expr) string { - switch a := arg.(type) { - case *ast.Ident: - if field := findFuncParam(fn, a.Name); field != nil { - return exprTypeName(field.Type) - } - case *ast.UnaryExpr: - if a.Op == token.AND { - return requestBodyStructName(fn, a.X) - } - case *ast.CompositeLit: - return exprTypeName(a.Type) - } - return "" -} - -func findFuncParam(fn *ast.FuncDecl, name string) *ast.Field { - if fn.Type.Params == nil { - return nil - } - for _, field := range fn.Type.Params.List { - for _, ident := range field.Names { - if ident.Name == name { - return field - } - } - } - return nil -} - -// exprTypeName returns the base type name of expr, unwrapping a pointer and resolving a qualified (pkg.Type) -// selector. -func exprTypeName(expr ast.Expr) string { - switch t := expr.(type) { - case *ast.StarExpr: - return exprTypeName(t.X) - case *ast.Ident: - return t.Name - case *ast.SelectorExpr: - return t.Sel.Name - default: - return "" + return nil, err } + return structs, nil } func collectFieldsForStruct(fset *token.FileSet, filename, structName string, structType *ast.StructType) map[string]goStructField { diff --git a/tools/metadata/schema_fields_test.go b/tools/metadata/schema_fields_test.go index be9d88dcb1c..1e3d88c3159 100644 --- a/tools/metadata/schema_fields_test.go +++ b/tools/metadata/schema_fields_test.go @@ -16,13 +16,16 @@ import ( "github.com/google/go-cmp/cmp" ) -func TestCheckSchemaFieldsMatchesBySchemaName(t *testing.T) { +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\"`"+` @@ -36,32 +39,31 @@ type Demo struct { `) result, err := checkSchemaFields(schemaFieldCheckOptions{ - descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", openapi3.Schemas{ - "demo": openapi3.NewSchemaRef("", &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, - schemaNames: []string{"demo"}, + 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{{ - OpenAPISchema: "demo", - GoStruct: "Demo", - OpenAPIFile: "descriptions/api.github.com/api.github.com.json", - MatchReason: "schema name", + 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) } @@ -82,256 +84,303 @@ type Demo struct { } } -func TestCheckSchemaFieldsMatchesByExactFieldSet(t *testing.T) { +func TestCheckSchemaFieldsMultipleAnnotations(t *testing.T) { t.Parallel() githubDir := t.TempDir() writeFile(t, filepath.Join(githubDir, "demo.go"), `package github -type ExactFieldsRequest struct { - ID *int64 `+"`json:\"id,omitempty\"`"+` - Name string `+"`json:\"name\"`"+` - Note *string `+"`json:\"note,omitempty\"`"+` +// 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 (s *svc) Create(ctx context.Context, body *ExactFieldsRequest) { - s.client.NewRequest(ctx, "POST", "u", body) +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", openapi3.Schemas{ - "unrelated-schema-name": openapi3.NewSchemaRef("", &openapi3.Schema{ - Required: []string{"id", "name"}, - Properties: openapi3.Schemas{ - "id": openapi3.NewSchemaRef("", openapi3.NewIntegerSchema()), - "name": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), - "note": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), - }, - }), - })}, + 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) + } +} - if diff := cmp.Diff([]*schemaFieldChecked{{ - OpenAPISchema: "unrelated-schema-name", - GoStruct: "ExactFieldsRequest", - OpenAPIFile: "descriptions/api.github.com/api.github.com.json", - MatchReason: "exact JSON field set", - }}, result.Checked); diff != "" { - t.Errorf("checked mismatch (-want +got):\n%v\nskipped: %#v", diff, result.Skipped) +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.JSONName+": "+diag.Message) + got = append(got, diag.GoStruct+": "+diag.Message) } want := []string{ - "id: field is required and non-nullable in the OpenAPI schema but is a pointer", + `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 TestCheckSchemaFieldsSkipsAmbiguousExactFieldSet(t *testing.T) { +func TestCheckSchemaFieldsResponseRole(t *testing.T) { t.Parallel() githubDir := t.TempDir() writeFile(t, filepath.Join(githubDir, "demo.go"), `package github -type FirstMatchRequest struct { - ID int64 `+"`json:\"id\"`"+` - Name string `+"`json:\"name\"`"+` +//meta:schema response GET /demo +type Demo struct { + ID int64 `+"`json:\"id\"`"+` } +`) -type SecondMatchRequest struct { - ID int64 `+"`json:\"id\"`"+` - Name string `+"`json:\"name\"`"+` + 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 (s *svc) First(ctx context.Context, body *FirstMatchRequest) { - s.client.NewRequest(ctx, "POST", "u", body) -} +func TestCheckSchemaFieldsUnsupportedComposition(t *testing.T) { + t.Parallel() + githubDir := t.TempDir() + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github -func (s *svc) Second(ctx context.Context, body *SecondMatchRequest) { - s.client.NewRequest(ctx, "POST", "u", body) -} +//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", openapi3.Schemas{ - "unrelated-schema-name": openapi3.NewSchemaRef("", &openapi3.Schema{ - Required: []string{"id", "name"}, - Properties: openapi3.Schemas{ - "id": openapi3.NewSchemaRef("", openapi3.NewIntegerSchema()), - "name": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), - }, - }), - })}, + 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.Checked) != 0 { - t.Fatalf("checked = %v, want none", result.Checked) - } - if len(result.Diagnostics) != 0 { - t.Fatalf("diagnostics = %v, want none", result.Diagnostics) - } - if len(result.Skipped) != 1 { - t.Fatalf("skipped = %v, want one skip", result.Skipped) + if len(result.Diagnostics) != 1 { + t.Fatalf("diagnostics = %v, want one", result.Diagnostics) } - if got := result.Skipped[0].Reason; !strings.Contains(got, "ambiguous Go struct field-set match") { - t.Errorf("skip reason = %q, want ambiguous field-set match", got) + 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 TestCheckSchemaFieldsAllowsRequiredNullablePointer(t *testing.T) { +func TestCheckSchemaFieldsIgnoresUnannotatedStructs(t *testing.T) { t.Parallel() githubDir := t.TempDir() - writeFile(t, filepath.Join(githubDir, "nullable.go"), `package github + writeFile(t, filepath.Join(githubDir, "demo.go"), `package github -type NullableDemo struct { - ID *int64 `+"`json:\"id\"`"+` +// Unannotated has fields that would fail the check if it were annotated. +type Unannotated struct { + Name *string `+"`json:\"name\"`"+` } `) - nullableInteger := openapi3.NewIntegerSchema() - nullableInteger.Nullable = true result, err := checkSchemaFields(schemaFieldCheckOptions{ - descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", openapi3.Schemas{ - "nullable-demo": openapi3.NewSchemaRef("", &openapi3.Schema{ - Required: []string{"id"}, - Properties: openapi3.Schemas{ - "id": openapi3.NewSchemaRef("", nullableInteger), - }, - }), - })}, - githubDir: githubDir, - schemaNames: []string{"nullable-demo"}, + 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) != 0 { - t.Errorf("diagnostics = %v, want none", result.Diagnostics) + 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 TestGoNameCandidates(t *testing.T) { +func TestCheckSchemaFieldsAllowsRequiredNullablePointer(t *testing.T) { t.Parallel() - tests := []struct { - name string - in string - want []string - }{ - {name: "plural and version tokens", in: "projects-v2", want: []string{"ProjectsV2", "ProjectV2"}}, - {name: "singular unchanged", in: "repository", want: []string{"Repository"}}, - {name: "trailing plural", in: "teams", want: []string{"Teams", "Team"}}, - {name: "multiple words no plural", in: "code-scanning-alert", want: []string{"CodeScanningAlert"}}, - {name: "initialism token", in: "api", want: []string{"API"}}, - {name: "empty", in: "", want: nil}, + 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) } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - if diff := cmp.Diff(tt.want, goNameCandidates(tt.in)); diff != "" { - t.Errorf("goNameCandidates(%q) mismatch (-want +got):\n%v", tt.in, diff) - } - }) + if len(result.Diagnostics) != 0 { + t.Errorf("diagnostics = %v, want none for a required nullable pointer field", result.Diagnostics) } } -func TestSingularize(t *testing.T) { +func TestResolveSchemaAnnotation(t *testing.T) { t.Parallel() - tests := []struct { - in, want string - }{ - {"projects", "project"}, - {"policies", "policy"}, - {"statuses", "status"}, - {"boxes", "box"}, - {"branches", "branch"}, - {"buses", "bus"}, - {"keys", "key"}, - {"class", "class"}, // "ss" is not treated as a plural "s" - {"v2", "v2"}, - {"", ""}, + 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)}), } - for _, tt := range tests { - if got := singularize(tt.in); got != tt.want { - t.Errorf("singularize(%q) = %q, want %q", tt.in, got, tt.want) + + 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 TestGoName(t *testing.T) { +func TestParseSchemaAnnotationsViaCollectGoStructs(t *testing.T) { t.Parallel() - tests := []struct { - name string - in []string - want string - }{ - {name: "initialism", in: []string{"api"}, want: "API"}, - {name: "url initialism", in: []string{"url"}, want: "URL"}, - {name: "special case oauth", in: []string{"oauth"}, want: "OAuth"}, - {name: "version token", in: []string{"projects", "v2"}, want: "ProjectsV2"}, - {name: "plain word", in: []string{"repository"}, want: "Repository"}, - {name: "skips empty tokens", in: []string{"code", "", "scanning"}, want: "CodeScanning"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - if got := goName(tt.in); got != tt.want { - t.Errorf("goName(%v) = %q, want %q", tt.in, got, tt.want) - } - }) + 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) } -} -func TestIsVersionToken(t *testing.T) { - t.Parallel() - tests := []struct { - in string - want bool - }{ - {"v2", true}, - {"v10", true}, - {"v", false}, - {"vx", false}, - {"2", false}, - {"version", false}, - {"", false}, + demo := structs["Demo"] + if demo == nil { + t.Fatal("Demo struct not collected") } - for _, tt := range tests { - if got := isVersionToken(tt.in); got != tt.want { - t.Errorf("isVersionToken(%q) = %v, want %v", tt.in, got, tt.want) - } + var got []string + for _, ann := range demo.annotations { + got = append(got, ann.String()) } -} - -func TestSplitOpenAPIName(t *testing.T) { - t.Parallel() - tests := []struct { - in string - want []string - }{ - {"projects-v2", []string{"projects", "v2"}}, - {"api.github.com", []string{"api", "github", "com"}}, - {"foo_bar-baz", []string{"foo", "bar", "baz"}}, - {"single", []string{"single"}}, + want := []string{"request POST /demo", "response GET /demo"} + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("annotations mismatch (-want +got):\n%v", diff) } - for _, tt := range tests { - if diff := cmp.Diff(tt.want, splitOpenAPIName(tt.in)); diff != "" { - t.Errorf("splitOpenAPIName(%q) mismatch (-want +got):\n%v", tt.in, diff) - } + if len(demo.annotationProblems) != 0 { + t.Errorf("annotationProblems = %v, want none", demo.annotationProblems) } - if got := splitOpenAPIName(""); len(got) != 0 { - t.Errorf("splitOpenAPIName(\"\") = %v, want empty", got) + if group := structs["Group"]; group == nil || len(group.annotations) != 0 { + t.Errorf("Group = %+v, want collected with no annotations", group) } } @@ -418,16 +467,22 @@ func TestDiagLocation(t *testing.T) { func TestSchemaFieldDiagnosticString(t *testing.T) { t.Parallel() withLoc := schemaFieldDiagnostic{ - OpenAPISchema: "sch", GoStruct: "S", Field: "F", JSONName: "j", + 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 sch): msg [api.json]"; got != want { + 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{ - OpenAPISchema: "sch", GoStruct: "S", Field: "F", JSONName: "j", Message: "msg", + Annotation: "request POST /demo", GoStruct: "S", Field: "F", JSONName: "j", Message: "msg", } - if got, want := noLoc.String(), "S.F (j from sch): msg"; got != want { + 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) } } @@ -450,86 +505,6 @@ func TestCanCheckOptionality(t *testing.T) { } } -// fieldSet and structWith build the minimal shapes the field-matching helpers need. -func fieldSet[T any](names ...string) map[string]T { - m := make(map[string]T, len(names)) - for _, name := range names { - var zero T - m[name] = zero - } - return m -} - -func structWith(fields ...string) *goStructInfo { - return &goStructInfo{fields: fieldSet[goStructField](fields...)} -} - -func TestSharedFieldCount(t *testing.T) { - t.Parallel() - got := sharedFieldCount( - &openapiSchemaFields{properties: fieldSet[openapiSchemaProperty]("a", "b", "c")}, - structWith("b", "c", "d"), - ) - if got != 2 { - t.Errorf("sharedFieldCount = %v, want 2", got) - } -} - -func TestSameJSONFieldSet(t *testing.T) { - t.Parallel() - tests := []struct { - name string - schema []string - strct []string - want bool - }{ - {name: "equal", schema: []string{"a", "b"}, strct: []string{"a", "b"}, want: true}, - {name: "different length", schema: []string{"a", "b"}, strct: []string{"a"}, want: false}, - {name: "same length different members", schema: []string{"a", "b"}, strct: []string{"a", "c"}, want: false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - got := sameJSONFieldSet( - &openapiSchemaFields{properties: fieldSet[openapiSchemaProperty](tt.schema...)}, - structWith(tt.strct...), - ) - if got != tt.want { - t.Errorf("sameJSONFieldSet = %v, want %v", got, tt.want) - } - }) - } -} - -func TestHasEnoughSharedFields(t *testing.T) { - t.Parallel() - tests := []struct { - name string - schema []string - strct []string - want bool - }{ - {name: "three shared", schema: []string{"a", "b", "c"}, strct: []string{"a", "b", "c"}, want: true}, - {name: "tiny type all shared", schema: []string{"a", "b"}, strct: []string{"a", "b"}, want: true}, - {name: "tiny type partial", schema: []string{"a", "b"}, strct: []string{"a", "x"}, want: false}, - {name: "percentage threshold met", schema: []string{"a", "b", "c"}, strct: []string{"a", "b", "x"}, want: true}, - {name: "percentage threshold missed", schema: []string{"a", "b", "c", "d"}, strct: []string{"a", "b", "x", "y"}, want: false}, - {name: "no overlap", schema: []string{"a", "b", "c"}, strct: []string{"x", "y", "z"}, want: false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - got := hasEnoughSharedFields( - &openapiSchemaFields{properties: fieldSet[openapiSchemaProperty](tt.schema...)}, - structWith(tt.strct...), - ) - if got != tt.want { - t.Errorf("hasEnoughSharedFields = %v, want %v", got, tt.want) - } - }) - } -} - func TestHasUnsupportedComposition(t *testing.T) { t.Parallel() str := openapi3.NewSchemaRef("", openapi3.NewStringSchema()) @@ -635,79 +610,25 @@ func TestSchemaProperties(t *testing.T) { func TestCheckSchemaFieldsCommand(t *testing.T) { testServer := newTestServer(t, "schema-ref", map[string]any{ "api.github.com/api.github.com.json": openapi3.T{ - Components: &openapi3.Components{ - Schemas: openapi3.Schemas{ - "demo": openapi3.NewSchemaRef("", &openapi3.Schema{ - Required: []string{"id", "name"}, - Properties: openapi3.Schemas{ - "id": openapi3.NewSchemaRef("", openapi3.NewIntegerSchema()), - "name": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), - "note": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), - }, - }), - }, - }, + 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 OpenAPI schema/Go struct pairs; skipped 0 OpenAPI schemas", "") + res.assertOutput("Found 0 schema field issues\nChecked 1 annotations on 1 annotated structs", "") res.assertNoErr() res.checkGolden() } -func TestCheckSchemaFieldsSkipsStructMatchingMultipleSchemas(t *testing.T) { - t.Parallel() - githubDir := t.TempDir() - writeFile(t, filepath.Join(githubDir, "demo.go"), `package github - -type ItemRequest struct { - ID int64 `+"`json:\"id\"`"+` - Type string `+"`json:\"type\"`"+` -} - -func (s *svc) Add(ctx context.Context, body *ItemRequest) { - s.client.NewRequest(ctx, "POST", "u", body) -} -`) - - itemSchema := func() *openapi3.SchemaRef { - return openapi3.NewSchemaRef("", &openapi3.Schema{ - Required: []string{"id", "type"}, - Properties: openapi3.Schemas{ - "id": openapi3.NewSchemaRef("", openapi3.NewIntegerSchema()), - "type": openapi3.NewSchemaRef("", openapi3.NewStringSchema()), - }, - }) - } - result, err := checkSchemaFields(schemaFieldCheckOptions{ - descriptions: []*openapiFile{testOpenAPIFile("descriptions/api.github.com/api.github.com.json", openapi3.Schemas{ - "schema-a": itemSchema(), - "schema-b": itemSchema(), - })}, - githubDir: githubDir, - }) - if err != nil { - t.Fatal(err) - } - - if len(result.Checked) != 0 { - t.Fatalf("checked = %v, want none", result.Checked) - } - if len(result.Diagnostics) != 0 { - t.Fatalf("diagnostics = %v, want none", result.Diagnostics) - } - var dropped bool - for _, skip := range result.Skipped { - if strings.Contains(skip.Reason, "matches multiple schemas by field set") { - dropped = true - } - } - if !dropped { - t.Errorf("skipped = %#v, want a \"matches multiple schemas by field set\" reason", result.Skipped) - } -} - func TestFilterAllowedSchemaFieldDiagnostics(t *testing.T) { t.Parallel() exceptions := []string{"ExemptStruct.ExemptField"} @@ -769,12 +690,20 @@ func TestSchemaFieldExceptionsFileParses(t *testing.T) { } } -func testOpenAPIFile(filename string, schemas openapi3.Schemas) *openapiFile { +func testOpenAPIFile(filename, path string, pathItem *openapi3.PathItem) *openapiFile { return &openapiFile{ filename: filename, description: &openapi3.T{ - Components: &openapi3.Components{ - Schemas: schemas, + 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), }, }, } diff --git a/tools/metadata/testdata/check-schema-fields/github/demo.go b/tools/metadata/testdata/check-schema-fields/github/demo.go index 7571e4cea28..225f7b6ac63 100644 --- a/tools/metadata/testdata/check-schema-fields/github/demo.go +++ b/tools/metadata/testdata/check-schema-fields/github/demo.go @@ -1,11 +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"` } - -func (s *svc) Create(ctx context.Context, body *DemoRequest) { - s.client.NewRequest(ctx, "POST", "u", body) -} From 1fe00a7646bac85aa4e14a5a788973a4af25dd10 Mon Sep 17 00:00:00 2001 From: JamBalaya56562 Date: Sat, 29 Aug 2026 10:01:46 +0900 Subject: [PATCH 6/6] github: annotate initial request types with //meta:schema Annotate 25 dedicated request body types (26 annotations) so that check-schema-fields validates them against the OpenAPI descriptions. All annotations verify clean against the current descriptions: "Found 0 schema field issues; Checked 26 annotations on 25 annotated structs". Further annotations can land incrementally alongside future pass-by-value conversions for #3644. --- github/gists_comments.go | 4 ++++ github/issues_comments.go | 3 +++ github/issues_milestones.go | 4 ++++ github/orgs_custom_repository_roles.go | 4 ++++ github/pulls_comments.go | 4 ++++ github/pulls_reviews.go | 4 ++++ github/repos.go | 2 ++ github/repos_autolinks.go | 2 ++ github/repos_deployment_branch_policies.go | 4 ++++ github/repos_keys.go | 2 ++ github/repos_merging.go | 4 ++++ github/repos_releases.go | 8 ++++++++ github/teams.go | 2 ++ github/users_keys.go | 2 ++ github/users_ssh_signing_keys.go | 2 ++ 15 files changed, 51 insertions(+) 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"`