Fix /api/search endpoint JSON encoding error #506
Conversation
DylanGriffith
force-pushed
the
fix-api-search-json-encoding
branch
3 times, most recently
from
b1e2261
to
a669cd1
Compare
Previously this endpoint was always returning an empty response for
valid requests. This is because the last line failed to encode the
SearchResult as JSON. After some investigation I can see that this is
because we default the Progress fields to `-Inf`. Since these progress
fields only seemm to be applicable for streaming use cases that use RPC
and don't encode as JSON then it seemed like the best fix was to simply
omit them in the JSON response.
This change also adds error handling to the JSON encoding so that these
errors will be simpler to debug in future. A test is also included that
reproduces the issue.
You can reproduce this issue and the fix locally by just curling the
search endpoint like:
```bash
curl -H 'Accept: application/json' -XPOST -d '{"Q":"a.*"}' 'http://127.0.0.1:6070/api/search' -i
```
DylanGriffith
force-pushed
the
fix-api-search-json-encoding
branch
from
a669cd1
to
1ac49ce
Compare
|
@keegancsmith I'm wondering if you might be able to review this small bugfix. I've started to base some of my code search work on top of this API but it's broken in |
|
@stefanhengl or perhaps I could trouble you for a review of this. I'm hoping this is a small and uncontroversial improvement to the API as it's currently broken. I have another addition I want to layer on top of this in another PR so hoping to get this merged first before reviewing the new functionality. |
|
Oh, thanks for fixing this! I just got done bisecting this to 022743f when I found your PR 😬. I didn't realize I was dropping an error on the floor there. Maybe you can fix the same problem in the list function near the bottom of the file. (I'm not a maintainer, so I can't help merge; good luck! 🌞) |
|
@beyang @keegancsmith @stefanhengl could you please take a look at this contribution or let me know if there is a different way you'd like to accept contributions. From your announcement of taking over maintainership of Zoekt https://about.sourcegraph.com/blog/sourcegraph-accepting-zoekt-maintainership I see:
and
As GitLab is planning on rolling out more Zoekt based features in the next few months we will likely be making many contributions to this project and we want to be confident that our contributions will be welcomed as this is a large investment for us. Of course I understand from maintaining an open source project that reviews do take time. I appreciate we may also have different priorities for features than Sourcegraph but we do share a common ambition to make this search engine flexible and performant at scale so I expect we'll be able to have a great collaboration with Sourcegraph on this project over time. We do have a backlog of a few other changes we'd like to make to Zoekt but in the meantime I'm hoping we can prove out our collaboration with this relatively small bug fix. |
@isker thanks for spotting that. I think I'd like to keep this change as small as possible to help out reviewers and will gladly fix that as well in a followup PR if this gets accepted. Do you agree it's fine to keep these separate or do you think combining them is a better idea? |
|
Apologies, PRs in this repo don't usually fester for so long. Bad mix of christmas holidays and me diving straight into a project outside of zoekt when I got back. I will get through my backlog of reviews today and be a better steward :) |
| Progress | ||
|
|
||
| // Do not encode this as we cannot encode -Inf in JSON | ||
| Progress `json:"-"` |
There was a problem hiding this comment.
@stefanhengl I'm assuming this fix is fine since we use gob in Sourcegraph for reading progress. This PR LGTM but maybe we can do something other than -Inf to make the API a little more ergonomic. Maybe Progress can be optional rather?
Repeat the fix from sourcegraph#506.
Repeat the fix from #506.
Previously this endpoint was always returning an empty response for valid requests. This is because the last line failed to encode the SearchResult as JSON. After some investigation I can see that this is because we default the Progress fields to
-Inf. Since these progress fields only seem to be applicable for streaming use cases, that use RPC, and don't encode as JSON then it seemed like the best fix was to simply omit them in the JSON response.This change also adds error handling to the JSON encoding so that these errors will be simpler to debug in future. A test is also included that reproduces the issue.
You can reproduce this issue and the fix locally by just curling the search endpoint like: