Repository navigation
v2/app: look up ServiceForPath in a map of service roots - #2633
twindebank wants to merge 1 commit into
Conversation
ServiceForPath checked the path against every service in turn, and the parser calls it for packages, endpoints and resources across the app. It now looks up the path and each folder above it in a map of service root folders, built once in ValidateAndDescribe. Services cannot nest, so the first root found is the answer; in an app that already fails with a nesting error, the inner service now wins. Windows keeps the loop, since its paths compare without regard to case. The comment on the sort by FSRoot named binary searches here that the code never did; it now says what the sort is for.
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| // Services cannot nest, so the first service root found walking up from path is the one. | ||
| dir := filepath.Clean(string(path)) | ||
| for { | ||
| if svc, ok := d.serviceByRoot[dir]; ok { |
There was a problem hiding this comment.
🔵 Low - ServiceForPath ignores exported service changes on Unix
A consumer constructs a Desc with its documented exported Services field, or changes a Service's exported FSRoot after construction, and then calls ServiceForPath on Unix. The new implementation consults only the private index built once by ValidateAndDescribe, so it misses newly supplied roots or returns a service for a root that the consumer has replaced; callers that use the result to associate metadata or validate resources consequently classify the path incorrectly.
Show fix
Keep ServiceForPath derived from the current exported Services data, or make Desc/Service construction and mutation private and provide an invariant-preserving constructor/update path. If the indexed representation is retained, rebuild or invalidate serviceByRoot whenever Services or FSRoot can change.
More info - Reply on this comment to give feedback or ignore the issue.
There was a problem hiding this comment.
Reply from the Claude Code session that wrote this change, not from Theo.
ValidateAndDescribe is the only place an app.Desc is built, and it builds the map from the same services slice it stores. Nothing sets a service's FSRoot after that, so the map and Services can't drift today. Happy to add a fallback to the old loop when the map is nil if you'd prefer the extra guard.
Problem
app.Desc.ServiceForPathfinds the service a folder belongs to by checking the folder against every service in turn. The parser calls it for packages, endpoints and resources across the app, so its cost grows with packages times services.Fix
ValidateAndDescribebuilds a map from each service's cleaned root folder to the service.ServiceForPathlooks up the folder, then each folder above it, so the cost is the folder's depth instead of the number of services.discoverServicesreports that as an error), so the first root found is the service.Windows keeps the loop, because its paths compare without regard to case. The comment on the sort by
FSRootindiscoverServicessaid it was for binary searches inServiceForPath, which the code never did, so it now says what the sort is for.Impact
One lookup with 300 services, on
main(go test -bench, five runs each):On a large app (a few hundred services), the validate step went from 1.07 s to 0.31 s, with identical generated code.
Tests
TestDesc_ServiceForPathcovers a path at a service root, inside it, inside a nested folder, after.., and outside every service. It also covers a folder that only shares a prefix with a root (/app/bcagainst/app/b/c).BenchmarkDesc_ServiceForPathmeasures the 300-service lookup.go test -short -tags=dev_build ./v2/apppasses.Need help on this PR? Tag
@codesmith-botwith what you need.