search

golang / gddo

Go Doc Dot Org
https://godoc.org
BSD 3-Clause "New" or "Revised" License
1.1k stars 266 forks source link

/packages API endpoint doesn't compute import_count, yet it includes it in JSON output (always 0). #542

Open rhenwood-arm opened 6 years ago

rhenwood-arm commented 6 years ago

Hi All,

Looking at the results of https://api.godoc.org/packages it seems that import_count is always 0. If this is confirmed, it seems this is not very useful and might be a bug. It would be much more useful to me to have the import_count values that are included in individual results (i.e. import_count: 326958, name: 'fmt' at time of writing).

rsc commented 6 years ago

Alternately, it may be that packages is meant to list only the paths, not any other metadata, and the ImportCount field in the corresponding struct is missing a `json:",omitempty"`.

dmitshur commented 6 years ago

I've investigated this to try to find out the intention of the /packages API endpoint. It's implemented here:

https://github.com/golang/gddo/blob/052378f5476869804f44ab61df2243a80ff3518f/gddo-server/main.go#L634-L646

I've looked at the code of Database.AllPackages method, and I can confirm it does not try to populate the import count nor any other fields aside from the package import path.

Other API endpoints do populate this field (as well as synopsis, etc.).

So it appears this is a bug in the sense that the /packages endpoint JSON should not include the import_count field. We need to take care when fixing it, because simply adding a omitempty to ImportCount int `json:"import_count"` would be problematic for other endpoints that do populate import counts. Having zero import count is a valid value and shouldn't be omitted for other endpoints.

I suspect the right fix would be to make a separate struct for JSON encoding packages for the /packages endpoint, one that includes only Path field.

Changing /packages endpoint to include import counts would be a larger change, one that can't be done without evaluating whether the performance would be acceptable. /packages already does a lot of work listing all known import paths.