fix!: Match request body fields to the OpenAPI schemas

[gmlewis]

BREAKING CHANGE: Many required request body fields are now passed by value.

I ran ./script/check-schema-fields.sh -fix from #4576 and applied what it planned, then regenerated the accessors.

I also realized that the tool was not doing everything I had originally hoped, so now -fix also compiles the checkout and repairs the call sites its own type changes break, reporting the ones no mechanical repair can express. testJSONBody compares JSON trees now, so it no longer needs per-call opts.

AI assistance was used to create this PR.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.59%. Comparing base (f88a6d1) to head (117e8a3).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4581   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files         197      197           
  Lines       18326    18326           
=======================================
  Hits        18068    18068           
  Misses        258      258           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

cc: @stevehipwell - @Not-Dhananjay-Mishra

[Not-Dhananjay-Mishra]

@gmlewis, I think there is one case we missed, If there is a struct that is used by more than one method, and those methods have different request schemas, IMO we should skip that fix.
We are already filling usesByStruct in run() present in check.go how about we store that inside checker struct and then, inside fixCandidates() check whether there is any conflict before applying the fix?

[gmlewis]

@gmlewis, I think there is one case we missed, If there is a struct that is used by more than one method, and those methods have different request schemas, IMO we should skip that fix. We are already filling usesByStruct in run() present in check.go how about we store that inside checker struct and then, inside fixCandidates() check whether there is any conflict before applying the fix?

Great catch! I believe this fixes it. PTAL.

[Not-Dhananjay-Mishra]

@gmlewis, -fix doesn't seem to be fixing the test call sites on my laptop. I ran ./script/check-schema-fields.sh -fix it only fix struct but not the test call sites.

dhananjay@Macbook-Air-M5 go-github % ./script/check-schema-fields.sh -fix        
planned repairs (1):
  github/actions_artifacts.go:79: ArtifactPeriodOpt.Days: remove the omit option from the json tag; replace the pointer type with the type it points at
repaired 1 finding(s) in 1 Go file(s); 40 finding(s) remain, 0 of them repairable by -fix

scanned 209 files, 7981 methods (1301 with //meta:operation)
body params: 206 by value, 62 by pointer (skipped; run paramcheck to convert them)
checked 141 body structs: 193 resolved operation uses, 10 uses with no JSON request body
fields checked: 618 (13 conditionally required, left alone)
40 findings (0 shown, 0 errors, 0 repairable by -fix)
    33  not-in-request-schema: the Go field is not in the request body schema
     4  optional-value-type: the property is optional, but a value type cannot be omitted
     1  optional-pointer-without-omitempty: the property is optional, but a nil pointer is sent as null
     1  optional-without-omit: the property is optional, but the field cannot be omitted
     1  required-but-omittable: the schema REQUIRES the property, but the tag lets it be omitted

dhananjay@Macbook-Air-M5 go-github % go test -gcflags=-e -vet=off -run=NONE ./...
# github.com/google/go-github/v92/github [github.com/google/go-github/v92/github.test]
github/github-accessors.go:2483:27: invalid operation: a.Days == nil (mismatched types int and untyped nil)
github/github-accessors.go:2486:10: invalid operation: cannot indirect a.Days (variable of type int)
github/actions_permissions_enterprise_test.go:407:36: cannot use new(90) (value of type *int) as int value in struct literal
github/actions_permissions_orgs_test.go:513:36: cannot use new(90) (value of type *int) as int value in struct literal
github/github-accessors_test.go:3100:32: cannot use &zeroValue (value of type *int) as int value in struct literal
github/repos_actions_permissions_test.go:202:36: cannot use new(90) (value of type *int) as int value in struct literal
FAIL    github.com/google/go-github/v92/github [build failed]
?       github.com/google/go-github/v92/test/integration        [no test files]
FAIL

[gmlewis]

@Not-Dhananjay-Mishra - I see what happened - with the -repo flag, everything was clean, but now I've fixed the path errors, so you should not see the problem anymore.

Also, ./script/generate.sh MUST be called after -fix and then everything should be clean.

PTAL.

[Not-Dhananjay-Mishra]

Early returning causes some problems.
for example -

  github/actions_permissions_enterprise_test.go:407: ArtifactPeriodOpt.Days: replace new(90) with 90
  github/actions_permissions_orgs_test.go:513: ArtifactPeriodOpt.Days: replace new(90) with 90
  github/github-accessors_test.go:3100: ArtifactPeriodOpt.Days: replace &zeroValue with zeroValue
  github/repos_actions_permissions_test.go:202: ArtifactPeriodOpt.Days: replace new(90) with 90

when loop reaches at github/github-accessors_test.go it error out on my laptop because of some permission issue. (idk why these errors are happening on my laptop 🥲)

schemafields: github/github-accessors_test.go: permission denied

As a result, github/repos_actions_permissions_test.go is skipped because we return from the previous iteration before the loop can reach it.

Suggested change

	if err := os.WriteFile(path, formatted, 0o600); err != nil {
	return fixed, written, err
	}
	if err := os.WriteFile(path, formatted, 0o600); err != nil {
	continue
	}

So I guess best solution here is not to early return, just continue. Or we can add strings.Contains() statement above and check if path contains any of these github-(accessors/iterators/stringify)_test.go

[gmlewis]

OK, I believe this is now cleaned up and I cleaned up a bunch of other reporting so that it should be much better now. PTAL.

[Not-Dhananjay-Mishra]

LGTM 🚀