From 95e2b4462f2ea39a6bbaaeaf2af43192caeb12f3 Mon Sep 17 00:00:00 2001
From: Bryan Fox <39674+bryfox@users.noreply.github.com>
Date: Tue, 9 Jul 2024 08:51:40 -0400
Subject: [PATCH] Fix table output with wide columns (#1191)

Fix: table output from list commands supports wide columns such as large
amounts of metadata

Previously, listing the metadata for an mcap file with many (>64kb)
key/value pairs failed silently.

The Scanner used in the table formatter has a buffer limit of 64kb, and
the error result was being ignored. This removes the scanner entirely
from most table output. It was introduced to trim leading whitespace
from lines, but the formatter accepts other options to make this happen.

I've given the info command its own formatter, which I think is
appropriate because it's formatting data differently, even including
tabs in its row data. The scanner's error result is now checked, though
we shouldn't see this in practice for `mcap info`.

This also makes a couple of copy fixes to the inline help for the
metadata commands.

Fixes #1189.
---
 go/cli/mcap/cmd/info.go     | 30 ++++++++++++++++++++++++++++--
 go/cli/mcap/cmd/metadata.go |  4 ++--
 go/cli/mcap/utils/utils.go  | 14 ++++----------
 3 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/go/cli/mcap/cmd/info.go b/go/cli/mcap/cmd/info.go
index e78e49aa62..5051da7d56 100644
--- a/go/cli/mcap/cmd/info.go
+++ b/go/cli/mcap/cmd/info.go
@@ -1,6 +1,7 @@
 package cmd
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	"fmt"
@@ -13,6 +14,7 @@ import (
 
 	"github.com/foxglove/mcap/go/cli/mcap/utils"
 	"github.com/foxglove/mcap/go/mcap"
+	"github.com/olekukonko/tablewriter"
 	"github.com/spf13/cobra"
 )
 
@@ -89,7 +91,9 @@ func printInfo(w io.Writer, info *mcap.Info) error {
 			header = addRow(header, "end:", "%s", decimalTime(endtime))
 		}
 	}
-	utils.FormatTable(buf, header)
+	if err := printSummaryRows(buf, header); err != nil {
+		return err
+	}
 	if len(info.ChunkIndexes) > 0 {
 		compressionFormatStats := make(map[mcap.CompressionFormat]struct {
 			count            int
@@ -166,7 +170,9 @@ func printInfo(w io.Writer, info *mcap.Info) error {
 		}
 		rows = append(rows, row)
 	}
-	utils.FormatTable(buf, rows)
+	if err := printSummaryRows(buf, rows); err != nil {
+		return err
+	}
 	if info.Statistics != nil {
 		fmt.Fprintf(buf, "attachments: %d\n", info.Statistics.AttachmentCount)
 		fmt.Fprintf(buf, "metadata: %d\n", info.Statistics.MetadataCount)
@@ -178,6 +184,26 @@ func printInfo(w io.Writer, info *mcap.Info) error {
 	return err
 }
 
+// Similar to utils.FormatTable, but optimized for 'expanded' display of nested data.
+func printSummaryRows(w io.Writer, rows [][]string) error {
+	buf := &bytes.Buffer{}
+	tw := tablewriter.NewWriter(buf)
+	tw.SetBorder(false)
+	tw.SetAutoWrapText(false)
+	tw.SetAlignment(tablewriter.ALIGN_LEFT)
+	tw.SetHeaderAlignment(tablewriter.ALIGN_LEFT)
+	tw.SetColumnSeparator("")
+	tw.AppendBulk(rows)
+	tw.Render()
+	// This tablewriter puts a leading space on the lines for some reason, so
+	// remove it.
+	scanner := bufio.NewScanner(buf)
+	for scanner.Scan() {
+		fmt.Fprintln(w, strings.TrimLeft(scanner.Text(), " "))
+	}
+	return scanner.Err()
+}
+
 var infoCmd = &cobra.Command{
 	Use:   "info",
 	Short: "Report statistics about an MCAP file",
diff --git a/go/cli/mcap/cmd/metadata.go b/go/cli/mcap/cmd/metadata.go
index f46a547183..b3475b6323 100644
--- a/go/cli/mcap/cmd/metadata.go
+++ b/go/cli/mcap/cmd/metadata.go
@@ -134,7 +134,7 @@ var addMetadataCmd = &cobra.Command{
 
 var getMetadataCmd = &cobra.Command{
 	Use:   "metadata",
-	Short: "get metadata by name",
+	Short: "Get metadata by name",
 	Run: func(_ *cobra.Command, args []string) {
 		ctx := context.Background()
 		if len(args) != 1 {
@@ -214,7 +214,7 @@ func init() {
 	}
 
 	getCmd.AddCommand(getMetadataCmd)
-	getMetadataCmd.PersistentFlags().StringVarP(&getMetadataName, "name", "n", "", "name of metadata record to create")
+	getMetadataCmd.PersistentFlags().StringVarP(&getMetadataName, "name", "n", "", "name of metadata record to get")
 	err = getMetadataCmd.MarkPersistentFlagRequired("name")
 	if err != nil {
 		die("failed to mark --name flag as required: %s", err)
diff --git a/go/cli/mcap/utils/utils.go b/go/cli/mcap/utils/utils.go
index 0af2ad0ca3..be9853223c 100644
--- a/go/cli/mcap/utils/utils.go
+++ b/go/cli/mcap/utils/utils.go
@@ -1,7 +1,6 @@
 package utils
 
 import (
-	"bufio"
 	"bytes"
 	"context"
 	"encoding/json"
@@ -9,7 +8,6 @@ import (
 	"io"
 	"os"
 	"regexp"
-	"strings"
 	"time"
 
 	"cloud.google.com/go/storage"
@@ -107,21 +105,17 @@ func WithReader(ctx context.Context, filename string, f func(remote bool, rs io.
 }
 
 func FormatTable(w io.Writer, rows [][]string) {
-	buf := &bytes.Buffer{}
-	tw := tablewriter.NewWriter(buf)
+	tw := tablewriter.NewWriter(w)
 	tw.SetBorder(false)
 	tw.SetAutoWrapText(false)
 	tw.SetAlignment(tablewriter.ALIGN_LEFT)
 	tw.SetHeaderAlignment(tablewriter.ALIGN_LEFT)
 	tw.SetColumnSeparator("")
+	tw.SetTablePadding("\t")
+	tw.SetNoWhiteSpace(true)
+
 	tw.AppendBulk(rows)
 	tw.Render()
-	// This tablewriter puts a leading space on the lines for some reason, so
-	// remove it.
-	scanner := bufio.NewScanner(buf)
-	for scanner.Scan() {
-		fmt.Fprintln(w, strings.TrimLeft(scanner.Text(), " "))
-	}
 }
 
 func Keys[T any](m map[string]T) []string {