Skip to content
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.

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

JSONL support #487

Merged
merged 1 commit into from
Mar 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 4 additions & 3 deletions temporalcli/commands.batch.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ func (c TemporalBatchListCommand) run(cctx *CommandContext, args []string) error
}
defer cl.Close()

// This is a listing command subject to json vs jsonl rules
cctx.Printer.StartList()
defer cctx.Printer.EndList()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking maybe we should use the term "array" rather than "list" in the code since that's the correct JSON terminology.

Copy link
Member Author

@cretz cretz Feb 29, 2024

Choose a reason for hiding this comment

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

list is what we call the commands and I don't want users to even have to worry about whether JSON is enabled or not. I want people to say whether their jsonl list is starting/ending too. From a caller POV, these calls are general purpose, not only JSON (despite my comments above them, maybe I should remove those). Often people don't think of arrays as lazy/streaming.

Having said this, if the word "array" is important enough, I can change to get PR approval.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that's fine, especially if there's a connection with the use of "list" as a verb in the CLI.


pageFetcher := c.pageFetcher(cctx, cl)
var nextPageToken []byte
var jobsProcessed int
Expand All @@ -80,9 +84,6 @@ func (c TemporalBatchListCommand) run(cctx *CommandContext, args []string) error
}

if pageIndex == 0 && len(page.GetOperationInfo()) == 0 {
if cctx.JSONOutput {
_ = cctx.Printer.PrintStructured([]any{}, printer.StructuredOptions{})
}
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion temporalcli/commands.batch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func (s *SharedServerSuite) TestBatchJob_List() {
)
s.NoError(res.Err)
s.Empty(res.Stderr.String())
s.Equal("[]\n", res.Stdout.String())
s.Equal("[\n]\n", res.Stdout.String())
})
})

Expand Down
4 changes: 2 additions & 2 deletions temporalcli/commands.gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ func NewTemporalCommand(cctx *CommandContext) *TemporalCommand {
s.Command.PersistentFlags().Var(&s.LogLevel, "log-level", "Log level. Accepted values: debug, info, warn, error, never.")
s.LogFormat = NewStringEnum([]string{"text", "json"}, "text")
s.Command.PersistentFlags().Var(&s.LogFormat, "log-format", "Log format. Accepted values: text, json.")
s.Output = NewStringEnum([]string{"text", "json"}, "text")
s.Command.PersistentFlags().VarP(&s.Output, "output", "o", "Data output format. Accepted values: text, json.")
s.Output = NewStringEnum([]string{"text", "json", "jsonl"}, "text")
s.Command.PersistentFlags().VarP(&s.Output, "output", "o", "Data output format. Accepted values: text, json, jsonl.")
s.TimeFormat = NewStringEnum([]string{"relative", "iso", "raw"}, "relative")
s.Command.PersistentFlags().Var(&s.TimeFormat, "time-format", "Time format. Accepted values: relative, iso, raw.")
s.Color = NewStringEnum([]string{"always", "never", "auto"}, "auto")
Expand Down
9 changes: 7 additions & 2 deletions temporalcli/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -381,12 +381,17 @@ func (c *TemporalCommand) preRun(cctx *CommandContext) error {
}

// Configure printer if not already on context
cctx.JSONOutput = c.Output.Value == "json"
cctx.JSONOutput = c.Output.Value == "json" || c.Output.Value == "jsonl"
// Only indent JSON if not jsonl
var jsonIndent string
if c.Output.Value == "json" {
jsonIndent = " "
}
if cctx.Printer == nil {
cctx.Printer = &printer.Printer{
Output: cctx.Options.Stdout,
JSON: cctx.JSONOutput,
JSONIndent: " ",
JSONIndent: jsonIndent,
JSONPayloadShorthand: !c.NoJsonShorthandPayloads,
}
switch c.TimeFormat.Value {
Expand Down
4 changes: 4 additions & 0 deletions temporalcli/commands.operator_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ func (c *TemporalOperatorClusterListCommand) run(cctx *CommandContext, args []st
}
defer cl.Close()

// This is a listing command subject to json vs jsonl rules
cctx.Printer.StartList()
defer cctx.Printer.EndList()

var nextPageToken []byte
var execsProcessed int
for pageIndex := 0; ; pageIndex++ {
Expand Down
4 changes: 4 additions & 0 deletions temporalcli/commands.workflow_view.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ func (c *TemporalWorkflowListCommand) run(cctx *CommandContext, args []string) e
}
defer cl.Close()

// This is a listing command subject to json vs jsonl rules
cctx.Printer.StartList()
defer cctx.Printer.EndList()

// Build request and start looping. We always use default page size regardless
// of user-defined limit, because we're ok w/ extra page data and the default
// is not clearly defined.
Expand Down
2 changes: 1 addition & 1 deletion temporalcli/commandsmd/commands.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ This document has a specific structure used by a parser. Here are the rules:
* `--env-file` (string) - File to read all environments (defaults to `$HOME/.config/temporalio/temporal.yaml`).
* `--log-level` (string-enum) - Log level. Options: debug, info, warn, error, never. Default: info.
* `--log-format` (string-enum) - Log format. Options: text, json. Default: text.
* `--output`, `-o` (string-enum) - Data output format. Options: text, json. Default: text.
* `--output`, `-o` (string-enum) - Data output format. Options: text, json, jsonl. Default: text.
* `--time-format` (string-enum) - Time format. Options: relative, iso, raw. Default: relative.
* `--color` (string-enum) - Set coloring. Options: always, never, auto. Default: auto.
* `--no-json-shorthand-payloads` (bool) - Always all payloads as raw payloads even if they are JSON.
Expand Down
77 changes: 69 additions & 8 deletions temporalcli/internal/printer/printer.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,18 @@ type Colorer func(string, ...interface{}) string

type Printer struct {
// Must always be present
Output io.Writer
JSON bool
Output io.Writer
JSON bool
// This is unset/empty in JSONL mode
JSONIndent string
JSONPayloadShorthand bool
// Only used for non-JSON, defaults to RFC3339
FormatTime func(time.Time) string
// Only used for non-JSON, defaults to color.Magenta
TableHeaderColorer Colorer

listMode bool
listModeFirstJSON bool // True until first JSON printed
}

// Ignored during JSON output
Expand All @@ -50,6 +54,42 @@ func (p *Printer) Printlnf(s string, v ...any) {
p.Println(fmt.Sprintf(s, v...))
}

// When called for JSON with indent, this will create an initial bracket and
// make sure all [Printer.PrintStructured] calls get commas properly to appear
// as a list (but the indention and multiline posture of the JSON remains). When
// called for JSON without indent, this will make sure all
// [Printer.PrintStructured] is on its own line (i.e. JSONL mode). When called
// for non-JSON, this is a no-op.
Comment on lines +57 to +62
Copy link
Member

Choose a reason for hiding this comment

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

This (and the if checks below) says to me that rather than using an empty JSONIndent as a special indication to do JSONL, we should instead just have the JSON field be JSON | JSONL and just be explicit about what mode we're in.

Copy link
Member Author

Choose a reason for hiding this comment

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

we should instead just have the JSON field be JSON | JSONL

More like JSON | JSONL | Neither, but it's ugly in Go. I considered this (and I considered two booleans), but JSONIndent is how Go does it (where empty string means no multiline, ref https://pkg.go.dev/encoding/json#Encoder.SetIndent). Throughout the Go JSON printing code they check whether indentation is set. And this option gives the caller the freedom to set the indent they want, even though we're the caller. However, if the way Go did this is too confusing, I can make this a Go enum and add a isJSON() that checks for those two values.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer that I think but it's not a big deal. I'll approve and leave it up to you. Lack up sum types 😢

//
// [Printer.EndList] must be called at the end. If this is called twice it will
// panic. This and the end call are not safe for concurrent use.
func (p *Printer) StartList() {
if p.listMode {
panic("already in list mode")
}
p.listMode, p.listModeFirstJSON = true, true
// Write initial bracket when non-jsonl
if p.JSON && p.JSONIndent != "" {
// Don't need newline, we count on initial object to do that
p.Output.Write([]byte("["))
}
}

// Must be called after [Printer.StartList] or will panic. See Godoc on that
// function for more details.
func (p *Printer) EndList() {
if !p.listMode {
panic("not in list mode")
}
p.listMode, p.listModeFirstJSON = false, false
// Write ending bracket when non-jsonl
if p.JSON && p.JSONIndent != "" {
// We prepend a newline because non-jsonl list mode doesn't do so after each
// line to help with commas
p.Output.Write([]byte("\n]\n"))
}
}

type StructuredOptions struct {
// Derived if not present. Ignored for JSON printing.
Fields []string
Expand Down Expand Up @@ -171,19 +211,40 @@ func (p *Printer) writef(s string, v ...any) {
}

func (p *Printer) printJSON(v any, options StructuredOptions) error {
// Before printing, if we're in non-jsonl list mode, we must append a comma
// and a newline if we're not the first JSON seen.
nonJSONLListMode := p.listMode && p.JSON && p.JSONIndent != ""
if nonJSONLListMode {
var prepend string
if p.listModeFirstJSON {
p.listModeFirstJSON = false
prepend = "\n"
} else {
prepend = ",\n"
}
if _, err := p.Output.Write([]byte(prepend)); err != nil {
return err
}
}

// Print JSON
shorthandPayloads := p.JSONPayloadShorthand
if options.OverrideJSONPayloadShorthand != nil {
shorthandPayloads = *options.OverrideJSONPayloadShorthand
}
b, err := p.jsonVal(v, p.JSONIndent, shorthandPayloads)
if err != nil {
if b, err := p.jsonVal(v, p.JSONIndent, shorthandPayloads); err != nil {
return err
} else if _, err := p.Output.Write(b); err != nil {
return err
}
_, err = p.Output.Write(b)
if err == nil {
_, err = p.Output.Write([]byte("\n"))

// Do not print a newline if in non-jsonl list mode
if !nonJSONLListMode {
if _, err := p.Output.Write([]byte("\n")); err != nil {
return err
}
}
return err
return nil
}

func (p *Printer) jsonVal(v any, indent string, shorthandPayloads bool) ([]byte, error) {
Expand Down
69 changes: 68 additions & 1 deletion temporalcli/internal/printer/printer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (
// * Text printer specific and non-specific fields and all sorts of table options
// * JSON printer

func TestTextPrinter(t *testing.T) {
func TestPrinter_Text(t *testing.T) {
type MyStruct struct {
Foo string
Bar bool
Expand Down Expand Up @@ -73,3 +73,70 @@ func normalizeMultiline(s string) string {
}
return ret
}

func TestPrinter_JSON(t *testing.T) {
var buf bytes.Buffer

// With indentation
p := printer.Printer{Output: &buf, JSON: true, JSONIndent: " "}
p.Println("should not print")
require.NoError(t, p.PrintStructured(map[string]string{"foo": "bar"}, printer.StructuredOptions{}))
require.Equal(t, `{
"foo": "bar"
}
`, buf.String())

// Without indentation
buf.Reset()
p = printer.Printer{Output: &buf, JSON: true}
p.Println("should not print")
require.NoError(t, p.PrintStructured(map[string]string{"foo": "bar"}, printer.StructuredOptions{}))
require.Equal(t, "{\"foo\":\"bar\"}\n", buf.String())
}

func TestPrinter_JSONList(t *testing.T) {
var buf bytes.Buffer

// With indentation
p := printer.Printer{Output: &buf, JSON: true, JSONIndent: " "}
p.StartList()
p.Println("should not print")
require.NoError(t, p.PrintStructured(map[string]string{"foo": "bar"}, printer.StructuredOptions{}))
require.NoError(t, p.PrintStructured(map[string]string{"baz": "qux"}, printer.StructuredOptions{}))
p.EndList()
require.Equal(t, `[
{
"foo": "bar"
},
{
"baz": "qux"
}
]
`, buf.String())

// Without indentation
buf.Reset()
p = printer.Printer{Output: &buf, JSON: true}
p.StartList()
p.Println("should not print")
require.NoError(t, p.PrintStructured(map[string]string{"foo": "bar"}, printer.StructuredOptions{}))
require.NoError(t, p.PrintStructured(map[string]string{"baz": "qux"}, printer.StructuredOptions{}))
p.EndList()
require.Equal(t, "{\"foo\":\"bar\"}\n{\"baz\":\"qux\"}\n", buf.String())

// Empty with indentation
buf.Reset()
p = printer.Printer{Output: &buf, JSON: true, JSONIndent: " "}
p.StartList()
p.Println("should not print")
p.EndList()
require.Equal(t, "[\n]\n", buf.String())

// Empty without indentation
buf.Reset()
p = printer.Printer{Output: &buf, JSON: true}
p.StartList()
p.Println("should not print")
p.EndList()
require.Equal(t, "", buf.String())
}
Loading