Repository navigation
Conversation
| if isRead(r) && r.Method != "GET" { | ||
| reasons[key] = append(reasons[key], "read over "+r.Method) | ||
| } | ||
| if !isRead(r) && r.Method == "GET" { |
There was a problem hiding this comment.
"write over GET" asserts more than the check knows. isRead only recognises List*/Get*/Lookup* (plus Healthcheck), so a read-only RPC under any other name — say SearchTopics over GET — is reported as a write. Failing closed is right; the message is what's off, since it misdescribes the RPC and doesn't tell the author what to do.
Either of these is a one-string change (plus the expected strings in routes_conformance_test.go):
- Say what the check actually knows:
"GET on an RPC not named List*/Get*/Lookup*" - Or keep the framing and name the remedy:
"write over GET (reads are named List*/Get*/Lookup*)"
Related: the AGENTS.md row says List*/Get*/Lookup* use GET, but not that every read must carry one of those names — which is what this check now enforces. If that's intended, worth saying in the row too.
There was a problem hiding this comment.
Fixed in 9cca3d7. The diagnostic now gives the expected method and explains the read naming rule without calling an unrecognized RPC a write. Updated AGENTS.md and added the SearchTopics-over-GET case, keeping the exact HealthRoutes.Healthcheck exception. Focused tests and make test pass locally.
| if r.Method != "GET" && r.Method != "POST" { | ||
| reasons[key] = append(reasons[key], "unsupported method: "+r.Method) | ||
| } | ||
| if isRead(r) && r.Method != "GET" { | ||
| reasons[key] = append(reasons[key], "read over "+r.Method) | ||
| } | ||
| if !isRead(r) && r.Method == "GET" { | ||
| reasons[key] = append(reasons[key], "write over GET") | ||
| } |
There was a problem hiding this comment.
Optional, not a blocker.
These three checks can be one. Each RPC has exactly one correct method, so:
want, kind := "POST", "write"
if isRead(r) {
want, kind = "GET", "read"
}
if r.Method != want {
reasons[key] = append(reasons[key], kind+" over "+r.Method)
}- Still enforces "no other methods": anything but the one expected method fails.
- One reason per mistake. Today a read over PUT reports
unsupported method: PUT; read over PUT, while a write over PUT reports onlyunsupported method: PUTand never points at POST. With this, every reason implies its fix.
Cost: the expected strings in routes_conformance_test.go change (unsupported method: X → read over X / write over X). If you're touching the write over GET wording anyway (other comment), it lands on the same lines.
There was a problem hiding this comment.
Simplified this to one expected-method check in 9cca3d7: GET for recognized reads, POST otherwise. Each method mismatch now has one reason, and path errors still accumulate separately. Updated the expectations; focused tests and make test pass locally.
There was a problem hiding this comment.
Optional. Consider moving these tests into routes_test.go, beside nonConformingRoutes, and dropping this file. go/contract keeps one test file per topic, and "conformance" already names the reusable suites (storetest, sessiontest), which this isn't.
There was a problem hiding this comment.
Moved both regression tests beside nonConformingRoutes in routes_test.go and removed the separate file in 9cca3d7. Tests pass locally.
| {"error - HEAD", "TopicRoutes", "CreateTopic", "HEAD", "/topic", "unsupported method: HEAD"}, | ||
| {"error - OPTIONS", "TopicRoutes", "CreateTopic", "OPTIONS", "/topic", "unsupported method: OPTIONS"}, | ||
| {"error - unknown method", "TopicRoutes", "CreateTopic", "BREW", "/topic", "unsupported method: BREW"}, | ||
| {"error - empty method", "TopicRoutes", "CreateTopic", "", "/topic", "unsupported method: "}, | ||
| {"error - lowercase method", "TopicRoutes", "CreateTopic", "post", "/topic", "unsupported method: post"}, | ||
| {"error - read over HEAD", "TopicRoutes", "GetTopic", "HEAD", "/topic", "unsupported method: HEAD; read over HEAD"}, |
There was a problem hiding this comment.
Optional. These rows, and health over OPTIONS, test methods the manifest can't contain: routegen emits only GET/POST/PUT/PATCH/DELETE, uppercase, and fails make gen on anything else (routegen/internal/model/model.go, already tested in describe_test.go). Consider dropping them.
There was a problem hiding this comment.
Dropped those rows, including read-over-HEAD and health-over-OPTIONS, in 9cca3d7. Confirmed routegen already rejects unsupported patterns and tests that boundary. Kept PUT/PATCH/DELETE coverage here. Focused tests and make test pass locally.
Fixes #73.
TestNonConformingRoutesnow rejects every method except GET/POST and rejects non-read RPCs served over GET. Existing read-over-non-GET and path checks continue to accumulate diagnostic reasons, and the exception map remains empty.HealthRoutes.Healthcheckis explicitly classified as a read because it only reports process health without touching the store. Its GET route and public RPC name stay unchanged; regression tests prevent this classification from extending to other services or similarly named RPCs.Validation: the new regression suite failed against the former checks, then passed with this change (
go test ./go/contract -run 'TestNonConformingRoutes' -count=1 -v).make testpassed for both the root module and the separately pinned routegen module using Go 1.27.1. Formatting andgit diff --checkpassed. Coverage includes valid reads/writes, PUT/PATCH/DELETE/HEAD/OPTIONS/unknown methods, accumulated reasons, and Healthcheck semantics.