Skip to content

search: add support for streaming search #485

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 9, 2021
Merged

search: add support for streaming search #485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9acfa41
search: add support for streaming search
stefanhengl Mar 4, 2021
07b7051
use flagSet.Outpout().Write instead of fmt.Printf
stefanhengl Mar 4, 2021
2020d66
use switch instead of if-continue
stefanhengl Mar 4, 2021
cc73500
add tests
stefanhengl Mar 5, 2021
fcf959b
count diff/commit results just like on Cloud
stefanhengl Mar 5, 2021
5c951db
address PR comments
stefanhengl Mar 9, 2021
b6017ee
minor: add newline before trace output
stefanhengl Mar 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions cmd/src/format.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ func parseTemplate(text string) (*template.Template, error) {
"addFloat": func(x, y float64) float64 {
return x + y
},
"addInt32": func(x, y int32) int32 {
return x + y
},
"debug": func(v interface{}) string {
data, _ := marshalIndent(v)
fmt.Println(string(data))
Expand Down Expand Up @@ -91,6 +94,13 @@ func parseTemplate(text string) (*template.Template, error) {
"buildVersionHasNewSearchInterface": searchTemplateFuncs["buildVersionHasNewSearchInterface"],
"renderResult": searchTemplateFuncs["renderResult"],

// Register stream-search specific template functions.
"streamSearchSequentialLineNumber": streamSearchTemplateFuncs["streamSearchSequentialLineNumber"],
"streamSearchHighlightMatch": streamSearchTemplateFuncs["streamSearchHighlightMatch"],
"streamSearchHighlightCommit": streamSearchTemplateFuncs["streamSearchHighlightCommit"],
"streamSearchRenderCommitLabel": streamSearchTemplateFuncs["streamSearchRenderCommitLabel"],
"matchOrMatches": streamSearchTemplateFuncs["matchOrMatches"],

// Alert rendering
"searchAlertRender": func(alert searchResultsAlert) string {
if content, err := alert.Render(); err != nil {
Expand Down
16 changes: 14 additions & 2 deletions cmd/src/search.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (

isatty "github.com/mattn/go-isatty"
"github.com/sourcegraph/src-cli/internal/api"
"github.com/sourcegraph/src-cli/internal/streaming"
"jaytaylor.com/html2text"
)

Expand Down Expand Up @@ -50,17 +51,28 @@ Other tips:

flagSet := flag.NewFlagSet("search", flag.ExitOnError)
var (
jsonFlag = flagSet.Bool("json", false, "Whether or not to output results as JSON")
jsonFlag = flagSet.Bool("json", false, "Whether or not to output results as JSON.")
explainJSONFlag = flagSet.Bool("explain-json", false, "Explain the JSON output schema and exit.")
apiFlags = api.NewFlags(flagSet)
lessFlag = flagSet.Bool("less", true, "Pipe output to 'less -R' (only if stdout is terminal, and not json flag)")
lessFlag = flagSet.Bool("less", true, "Pipe output to 'less -R' (only if stdout is terminal, and not json flag).")
streamFlag = flagSet.Bool("stream", false, "Consume results as stream. Streaming search only supports a subset of flags and parameters: trace, insecure-skip-verify, display.")
display = flagSet.Int("display", -1, "Limit the number of results that are displayed. Only supported together with stream flag. Statistics continue to report all results.")
)

handler := func(args []string) error {
if err := flagSet.Parse(args); err != nil {
return err
}

if *streamFlag {
opts := streaming.Opts{
Display: *display,
Trace: apiFlags.Trace(),
}
client := cfg.apiClient(apiFlags, flagSet.Output())
return streamSearch(flagSet.Arg(0), opts, client, os.Stdout)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you define -stream as a noop on the other flagset that you then use in streamHandler you wouldn't have to do this here, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but then we mess with the auto-documentation. Suddenly the "streaming search" has a "-stream" flag which is odd. I traded odd code against odd documentation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not create a separate stream-search command?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is based on a discussion we had around exhaustive search which is documented in RFC 288. At some point we want phase out -stream and replace search with streaming search. That is part of the reason why I wrote it as handler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of weird though:

$ ./src search -stream -get-curl foo
flag provided but not defined: -get-curl
Usage of streaming search:
  -display int
    	Limit the number of results that are displayed. Note that the statistics continue to report all results. (default -1)
  -insecure-skip-verify
    	Skip validation of TLS certificates against trusted chains
  -trace
    	Log the trace ID for requests. See https://docs.sourcegraph.com/admin/observability/tracing

Given we intend on replacing search with stream search in the future, can't we make the stream search stuff not use its own flagset. I would think an approach like

streaming.Search(streaming.Opts{
  Query: flagSet.Args(),
  Trace: *trace,
  Display: *display,
  InsecureSkipVerify: *insecureSkipVerify,
})

would make sense. Or even you pass in the http client you use that hides the trace/insecure stuff.

That way you can also move the large amounts of business logic out of ./cmd/src into ./internal/streaming

Copy link
Member Author

@stefanhengl stefanhengl Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually like this behavior, but since both reviewers commented on this I may be on the wrong track here. I refactored it to be a simple function call instead of a handler and moved parts of the logic to internal/streaming.

if *explainJSONFlag {
fmt.Println(searchJSONExplanation)
return nil
Expand Down
11 changes: 7 additions & 4 deletions cmd/src/search_alert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,18 @@ func init() {
}
}

// ProposedQuery is a suggested query to run when we emit an alert.
type ProposedQuery struct {
Description string
Query string
}

// searchResultsAlert is a type that can be used to unmarshal values returned by
// the searchResultsAlertFragment GraphQL fragment below.
type searchResultsAlert struct {
Title string
Description string
ProposedQueries []struct {
Description string
Query string
}
ProposedQueries []ProposedQuery
}

// Render renders an alert to a string ready to be output to a console,
Expand Down
5 changes: 1 addition & 4 deletions cmd/src/search_alert_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,7 @@ func TestRender(t *testing.T) {
full := &searchResultsAlert{
Title: "foo",
Description: "bar",
ProposedQueries: []struct {
Description string
Query string
}{
ProposedQueries: []ProposedQuery{
{
Description: "quux",
Query: "xyz:abc",
Expand Down
Loading