From 61a9a0689c78647c6e340aab52a6c4b29e950602 Mon Sep 17 00:00:00 2001 From: Simon J Mudd Date: Sat, 28 Mar 2026 11:21:40 +0100 Subject: [PATCH 1/2] refactor view Simplify view handling logic - no functional change --- connector/connector.go | 1 - view/access_info.go | 61 -------- view/view.go | 328 ++++++++++++++++------------------------- 3 files changed, 125 insertions(+), 265 deletions(-) delete mode 100644 view/access_info.go diff --git a/connector/connector.go b/connector/connector.go index a2ad4e7..606d28b 100644 --- a/connector/connector.go +++ b/connector/connector.go @@ -99,7 +99,6 @@ func (c *Connector) Connect() error { return nil } - // NewConnector returns a connected Connector given the provided configuration. // It returns an error if connection fails instead of calling os.Exit. func NewConnector(cfg Config) (*Connector, error) { // nolint:gocyclo diff --git a/view/access_info.go b/view/access_info.go deleted file mode 100644 index 85f1234..0000000 --- a/view/access_info.go +++ /dev/null @@ -1,61 +0,0 @@ -// Package view provides a simple way of checking access to a table -package view - -import ( - "database/sql" - - "github.com/sjmudd/ps-top/log" -) - -// AccessInfo holds a database and table name and information on whether the table is reachable -type AccessInfo struct { - Database string - Table string - checkedSelectError bool - selectError error -} - -// NewAccessInfo returns a new AccessInfo type -func NewAccessInfo(database, table string) AccessInfo { - log.Println("NewAccessInfo(", database, ",", table, ")") - return AccessInfo{Database: database, Table: table} -} - -// Name returns the fully qualified table name -func (ta AccessInfo) Name() string { - if len(ta.Database) > 0 && len(ta.Table) > 0 { - return ta.Database + "." + ta.Table - } - return "" -} - -// CheckSelectError returns whether SELECT works on the table -func (ta *AccessInfo) CheckSelectError(db *sql.DB) error { - // return cached result if we have one - if ta.checkedSelectError { - return ta.selectError - } - - var one int - err := db.QueryRow("SELECT 1 FROM " + ta.Name() + " LIMIT 1").Scan(&one) - - switch { - case err == sql.ErrNoRows: - ta.selectError = nil // no rows is fine - case err != nil: - ta.selectError = err // keep this - default: - ta.selectError = nil // select worked - } - ta.checkedSelectError = true - - return ta.selectError -} - -// SelectError returns the result of ta.selectError -func (ta AccessInfo) SelectError() error { - if !ta.checkedSelectError { - log.Fatal("table.AccessInfo.SelectError(", ta, ") called without having called CheckSelectError() first") - } - return ta.selectError -} diff --git a/view/view.go b/view/view.go index 7160ec3..f726645 100644 --- a/view/view.go +++ b/view/view.go @@ -1,13 +1,9 @@ -// package view holds the information related to the views -// -// - this includes the ordering of the view and whether they are -// SELECTable or not package view import ( "database/sql" - "errors" "fmt" + "strings" "github.com/sjmudd/ps-top/log" "github.com/sjmudd/ps-top/model/processlist" @@ -16,10 +12,6 @@ import ( // Code represents the type of information to view (as an int) type Code int -func (s Code) String() string { - return names[s] -} - // View* constants represent different views we can see const ( ViewNone Code = iota // view nothing (should never be set) @@ -33,251 +25,181 @@ const ( ViewMemory // view memory usage (5.7+) ) -// View holds the integer type of view (maybe need to fix this setup) +// viewDef holds the static and dynamic definition of a view +type viewDef struct { + code Code + name string // view name for display/URL + table string // fully qualified table name (database.table) + selectable bool // whether the table is accessible (determined at runtime) +} + +// View holds the current view state and a reference to the manager type View struct { - code Code + manager *viewManager + code Code } -var ( - setup bool // not protected by a mutex! - names map[Code]string // map a View to a string name - tables map[Code]AccessInfo // map a view to a table name and whether it's selectable or not +// viewManager manages all views and their ordering +type viewManager struct { + views []viewDef // all selectable views in display order + codeToIndex map[Code]int // quick lookup from code to index +} - nextView map[Code]Code // map from one view to the next taking into account invalid views - prevView map[Code]Code // map from one view to the next taking into account invalid views -) +// allViewsDef contains the static definition for every view code +// The order in this slice defines the default display order +var allViewsDef = []viewDef{ + {ViewLatency, "table_io_latency", "performance_schema.table_io_waits_summary_by_table", false}, + {ViewOps, "table_io_ops", "performance_schema.table_io_waits_summary_by_table", false}, + {ViewIO, "file_io_latency", "performance_schema.file_summary_by_instance", false}, + {ViewLocks, "table_lock_latency", "performance_schema.table_lock_waits_summary_by_table", false}, + {ViewUsers, "user_latency", "processlist", false}, // processlist table resolved later + {ViewMutex, "mutex_latency", "performance_schema.events_waits_summary_global_by_event_name", false}, + {ViewStages, "stages_latency", "performance_schema.events_stages_summary_global_by_event_name", false}, + {ViewMemory, "memory_usage", "performance_schema.memory_summary_global_by_event_name", false}, +} -// SetupAndValidate setups the view configuration and validates if access to the p_s tables is permitted. +// SetupAndValidate creates a new view manager, validates table access, +// and returns a View set to the requested name (or default if empty). +// It is the main entry point for initializing the view system. func SetupAndValidate(name string, db *sql.DB) (View, error) { - var v View - - log.Printf("view.SetupAndValidate(%q,%v)", name, db) - - if !setup { - names = map[Code]string{ - ViewLatency: "table_io_latency", - ViewOps: "table_io_ops", - ViewIO: "file_io_latency", - ViewLocks: "table_lock_latency", - ViewUsers: "user_latency", - ViewMutex: "mutex_latency", - ViewStages: "stages_latency", - ViewMemory: "memory_usage", - } - - // determine if we use information_schema.processslist or performance_schema.processslist - // - preferring access to performance_schema - var processlistSchema = "performance_schema" - havePS, err := processlist.HavePerformanceSchema(db) - if err != nil { - return v, fmt.Errorf("SetupAndValidate(%q,%v): %w", name, db, err) - } - if !havePS { - processlistSchema = "information_schema" - } - - tables = map[Code]AccessInfo{ - ViewLatency: NewAccessInfo("performance_schema", "table_io_waits_summary_by_table"), - ViewOps: NewAccessInfo("performance_schema", "table_io_waits_summary_by_table"), - ViewIO: NewAccessInfo("performance_schema", "file_summary_by_instance"), - ViewLocks: NewAccessInfo("performance_schema", "table_lock_waits_summary_by_table"), - ViewUsers: NewAccessInfo(processlistSchema, "processlist"), - ViewMutex: NewAccessInfo("performance_schema", "events_waits_summary_global_by_event_name"), - ViewStages: NewAccessInfo("performance_schema", "events_stages_summary_global_by_event_name"), - ViewMemory: NewAccessInfo("performance_schema", "memory_summary_global_by_event_name"), - } - - if err := validateViews(db); err != nil { - return v, fmt.Errorf("SetupAndValidate(%q,%v): %w", name, db, err) + log.Printf("view.SetupAndValidate(%q, db)", name) + + // Resolve processlist table schema (depends on MySQL version) + defs := make([]viewDef, len(allViewsDef)) + copy(defs, allViewsDef) // shallow copy so we can modify the table field + for i := range defs { + if defs[i].code == ViewUsers { + havePS, err := processlist.HavePerformanceSchema(db) + if err != nil { + return View{}, fmt.Errorf("SetupAndValidate: %w", err) + } + if havePS { + defs[i].table = "performance_schema.processlist" + } else { + defs[i].table = "information_schema.processlist" + } } } - v.SetByName(name) // if empty will use the default - return v, nil -} - -// validateViews check which views are readable. If none are we give a fatal error -func validateViews(db *sql.DB) error { - var count int - var isOrIsNot string - log.Println("Validating access to views...") - - // determine which of the defined views is valid because the underlying table access works - for v := range names { - ta := tables[v] - e := ta.CheckSelectError(db) - suffix := "" - if e == nil { - isOrIsNot = "is" - count++ + // Check which views are actually accessible + selectableViews := make([]viewDef, 0, len(defs)) + for i := range defs { + def := &defs[i] + if def.canSelect(db) { + def.selectable = true + selectableViews = append(selectableViews, *def) } else { - isOrIsNot = "IS NOT" - suffix = " " + e.Error() + log.Printf("View %s (%s) is NOT SELECTable", def.name, def.table) } - tables[v] = ta - log.Println(v.String() + ": " + ta.Name() + " " + isOrIsNot + " SELECTable" + suffix) } - if count == 0 { - return errors.New("none of the required tables are SELECTable. Giving up") + if len(selectableViews) == 0 { + return View{}, fmt.Errorf("no views are SELECTable") } - log.Println(count, "of", len(names), "view(s) are SELECTable, continuing") - - return setPrevAndNextViews() -} - -/* set the previous and next views taking into account any invalid views - -name selectable? prev next ----- ----------- ---- ---- -v1 false v4 v2 -v2 true v4 v4 -v3 false v2 v4 -v4 true v2 v2 -v5 false v4 v2 - -*/ -func setPrevAndNextViews() error { - var err error + log.Printf("%d of %d views are SELECTable, continuing", len(selectableViews), len(defs)) - log.Println("view.setPrevAndNextViews()...") - nextView = make(map[Code]Code) - prevView = make(map[Code]Code) - - // reset values - for v := range names { - nextView[v] = ViewNone - prevView[v] = ViewNone - } - - // Cleaner way to do this? Probably. Fix later. - prevCodeOrder := []Code{ViewMemory, ViewStages, ViewMutex, ViewUsers, ViewLocks, ViewIO, ViewOps, ViewLatency} - nextCodeOrder := []Code{ViewLatency, ViewOps, ViewIO, ViewLocks, ViewUsers, ViewMutex, ViewStages, ViewMemory} - prevView, err = setValidByValues(prevCodeOrder) - if err != nil { - return fmt.Errorf("setPrevAndNextViews: failed to set prevView: %w", err) + // Build the manager + manager := &viewManager{ + views: selectableViews, + codeToIndex: make(map[Code]int, len(selectableViews)), } - nextView, err = setValidByValues(nextCodeOrder) - if err != nil { - return fmt.Errorf("setPrevAndNextViews: failed to set nextView: %w", err) + for i, def := range selectableViews { + manager.codeToIndex[def.code] = i } - // print out the results - log.Println("Final mapping of view order:") - for i := range nextCodeOrder { - log.Println("view:", nextCodeOrder[i], ", prev:", prevView[nextCodeOrder[i]], ", next:", nextView[nextCodeOrder[i]]) + // Create initial view + v := View{ + manager: manager, } - return nil -} - -// setValidNextByValues returns a map of Code -> Code where the mapping points to the "next" -// Code. The order is determined by the input Code slice. Only Selectable Views are considered -// for the mapping with the other views pointing to the first Code provided. -func setValidByValues(orderedCodes []Code) (map[Code]Code, error) { - log.Println("view.setValidByValues()") - orderedMap := make(map[Code]Code) - - // reset orderedCodes - for i := range orderedCodes { - orderedMap[orderedCodes[i]] = ViewNone - } - - first, last := ViewNone, ViewNone - - // first pass, try to find values and point forward to next position if known. - // we must find at least one value view in the first pass. - for i := range []int{1, 2} { - for i := range orderedCodes { - currentPos := orderedCodes[i] - if tables[currentPos].SelectError() == nil { - if first == ViewNone { - first = currentPos - } - if last != ViewNone { - orderedMap[last] = currentPos - } - last = currentPos - } - } - if i == 1 { - // not found a valid view so something is up. Give up! - if first == ViewNone { - return orderedMap, fmt.Errorf("setValidByValues(%v+) cannot find a Selectable view! (should not happen)", orderedCodes) - } - } + // Set the requested view name (or default) + if err := v.SetByName(name); err != nil { + return View{}, err } - // final pass viewNone entries should point to first - for i := range orderedCodes { - currentPos := orderedCodes[i] - if tables[currentPos].SelectError() != nil { - orderedMap[currentPos] = first - } - } + return v, nil +} - return orderedMap, nil +// canSelect tests if the view's table can be queried +func (def *viewDef) canSelect(db *sql.DB) bool { + var dummy int + err := db.QueryRow("SELECT 1 FROM " + def.table + " LIMIT 1").Scan(&dummy) + return err == nil || err == sql.ErrNoRows } -// SetNext changes the current view to the next one +// SetNext changes to the next view (wraps around) func (v *View) SetNext() Code { - v.code = nextView[v.code] - + idx := v.manager.codeToIndex[v.code] + idx = (idx + 1) % len(v.manager.views) + v.code = v.manager.views[idx].code return v.code } -// SetPrev changes the current view to the previous one +// SetPrev changes to the previous view (wraps around) func (v *View) SetPrev() Code { - v.code = prevView[v.code] - + idx := v.manager.codeToIndex[v.code] + idx = (idx - 1 + len(v.manager.views)) % len(v.manager.views) + v.code = v.manager.views[idx].code return v.code } -// Set sets the view to the given view (by Code) +// Set sets the view to the specified code if it's selectable. +// If not selectable, falls back to the first selectable view. func (v *View) Set(viewCode Code) { - v.code = viewCode - - if tables[v.code].SelectError() != nil { - v.code = nextView[v.code] + if _, ok := v.manager.codeToIndex[viewCode]; ok { + v.code = viewCode + } else { + // fallback to first view + v.code = v.manager.views[0].code } } -// SetByName sets the view name to use based on its name. -// - If we provide an empty name then use the default. -// - If we don't provide a valid name then give an error -func (v *View) SetByName(name string) { +// SetByName sets the view by its string name. +// Empty name selects the first view (ViewLatency if available). +// Returns error if the name is not found or not selectable. +func (v *View) SetByName(name string) error { log.Println("View.SetByName(" + name + ")") + if name == "" { - log.Println("View.SetByName(): name is empty so setting to:", ViewLatency.String()) - v.Set(ViewLatency) - return + v.code = v.manager.views[0].code + log.Println("View.SetByName(): empty name, set to:", v.String()) + return nil } - for i := range names { - if name == names[i] { - v.code = i - log.Println("View.SetByName(", name, ")") - return + // Look up the code from the static allViewsDef list + for _, def := range allViewsDef { + if def.name == name { + if _, ok := v.manager.codeToIndex[def.code]; ok { + v.code = def.code + log.Println("View.SetByName(): set to", name) + return nil + } } } - // suggest what should be used - allViews := "" - for i := range names { - allViews = allViews + " " + names[i] + // not found or not selectable + allViews := make([]string, 0, len(v.manager.views)) + for _, def := range v.manager.views { + allViews = append(allViews, def.name) } - - // no need for now to strip off leading space from allViews. - log.Fatal("Asked for a view name, '", name, "' which doesn't exist. Try one of:", allViews) + return fmt.Errorf("view name '%s' not found or not selectable. Available: %s", name, strings.Join(allViews, ", ")) } -// Get returns the Code version of the current view +// Get returns the current view Code func (v View) Get() Code { return v.code } -// Name returns the string version of the current view +// Name returns the string name of the current view func (v View) Name() string { - return v.code.String() + if idx, ok := v.manager.codeToIndex[v.code]; ok { + return v.manager.views[idx].name + } + return "" +} + +// String returns the string name (same as Name) +func (v View) String() string { + return v.Name() } From a5ca23c0a1d6ca6fb2f442e598840612e4031cc7 Mon Sep 17 00:00:00 2001 From: Simon J Mudd Date: Sat, 28 Mar 2026 11:32:33 +0100 Subject: [PATCH 2/2] Add logic tests for view --- view/view_test.go | 192 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 192 insertions(+) create mode 100644 view/view_test.go diff --git a/view/view_test.go b/view/view_test.go new file mode 100644 index 0000000..5d58c97 --- /dev/null +++ b/view/view_test.go @@ -0,0 +1,192 @@ +package view + +import ( + "testing" +) + +// mockViewManager creates a viewManager with the given view definitions. +// Only selectable views are included, mirroring the real SetupAndValidate. +func mockViewManager(defs []viewDef) *viewManager { + manager := &viewManager{ + views: make([]viewDef, 0, len(defs)), + codeToIndex: make(map[Code]int, len(defs)), + } + for _, def := range defs { + if def.selectable { + manager.views = append(manager.views, def) + manager.codeToIndex[def.code] = len(manager.views) - 1 + } + } + return manager +} + +// mockView creates a View with the given manager and initial code. +// This is for testing only. +func mockView(manager *viewManager, initialCode Code) View { + return View{ + manager: manager, + code: initialCode, + } +} + +// TestViewSetNext tests that SetNext cycles through views correctly. +func TestViewSetNext(t *testing.T) { + defs := []viewDef{ + {code: ViewLatency, name: "table_io_latency", table: "performance_schema.table1", selectable: true}, + {code: ViewOps, name: "table_io_ops", table: "performance_schema.table2", selectable: true}, + {code: ViewIO, name: "file_io_latency", table: "performance_schema.table3", selectable: true}, + } + manager := mockViewManager(defs) + v := mockView(manager, ViewLatency) + + if v.SetNext() != ViewOps { + t.Errorf("SetNext from ViewLatency expected ViewOps, got %v", v.Get()) + } + if v.Get() != ViewOps { + t.Errorf("after SetNext, view code should be ViewOps, got %v", v.Get()) + } + + if v.SetNext() != ViewIO { + t.Errorf("SetNext from ViewOps expected ViewIO, got %v", v.Get()) + } + + // Should wrap around + if v.SetNext() != ViewLatency { + t.Errorf("SetNext should wrap around from ViewIO to ViewLatency, got %v", v.Get()) + } +} + +// TestViewSetPrev tests that SetPrev cycles through views correctly. +func TestViewSetPrev(t *testing.T) { + defs := []viewDef{ + {code: ViewLatency, name: "table_io_latency", table: "performance_schema.table1", selectable: true}, + {code: ViewOps, name: "table_io_ops", table: "performance_schema.table2", selectable: true}, + {code: ViewIO, name: "file_io_latency", table: "performance_schema.table3", selectable: true}, + } + manager := mockViewManager(defs) + v := mockView(manager, ViewOps) + + if v.SetPrev() != ViewLatency { + t.Errorf("SetPrev from ViewOps expected ViewLatency, got %v", v.Get()) + } + + // Should wrap around + if v.SetPrev() != ViewIO { + t.Errorf("SetPrev should wrap around from ViewLatency to ViewIO, got %v", v.Get()) + } +} + +// TestViewSet tests setting view code directly. +func TestViewSet(t *testing.T) { + defs := []viewDef{ + {code: ViewLatency, name: "table_io_latency", table: "performance_schema.table1", selectable: true}, + {code: ViewOps, name: "table_io_ops", table: "performance_schema.table2", selectable: true}, + {code: ViewIO, name: "file_io_latency", table: "performance_schema.table3", selectable: false}, // not selectable + } + manager := mockViewManager(defs) + + // Valid, selectable code + v := mockView(manager, ViewLatency) + v.Set(ViewOps) + if v.Get() != ViewOps { + t.Errorf("Set(ViewOps) failed, got %v", v.Get()) + } + + // Non-selectable code should fall back to first selectable (ViewLatency) + v.Set(ViewIO) + if v.Get() != ViewLatency { + t.Errorf("Set(non-selectable) should fall back to first selectable, got %v", v.Get()) + } +} + +// TestViewName tests that Name() returns the correct view name. +func TestViewName(t *testing.T) { + defs := []viewDef{ + {code: ViewLatency, name: "table_io_latency", table: "performance_schema.table1", selectable: true}, + {code: ViewOps, name: "table_io_ops", table: "performance_schema.table2", selectable: true}, + } + manager := mockViewManager(defs) + v := mockView(manager, ViewOps) + + if name := v.Name(); name != "table_io_ops" { + t.Errorf("View.Name() expected 'table_io_ops', got %q", name) + } +} + +// TestViewNameUnknown tests that Name() returns empty string for unknown code. +func TestViewNameUnknown(t *testing.T) { + defs := []viewDef{ + {code: ViewLatency, name: "table_io_latency", table: "performance_schema.table1", selectable: true}, + } + manager := mockViewManager(defs) + v := mockView(manager, ViewMutex) // not in manager + + if name := v.Name(); name != "" { + t.Errorf("View.Name() for unknown code should return empty string, got %q", name) + } +} + +// TestSetByNameInvalid tests that SetByName returns error for unknown view name. +// This test directly uses SetByName and checks error handling. +func TestSetByNameInvalid(t *testing.T) { + // The real SetByName depends on the global allViewsDef. For this test, + // we directly check that providing a name that doesn't exist in allViewsDef + // returns an error. Since we cannot easily mock allViewsDef, this test uses + // the real global but with a name that definitely doesn't match. + // However, we need a valid manager. We'll create one with a known selectable view. + defs := []viewDef{ + {code: ViewLatency, name: "table_io_latency", table: "performance_schema.table1", selectable: true}, + } + manager := mockViewManager(defs) + v := mockView(manager, ViewLatency) + + // Use a nonsense name + err := v.SetByName("this_name_does_not_exist") + if err == nil { + t.Fatal("SetByName(\"this_name_does_not_exist\") expected error, got nil") + } + // Should not change current view + if v.Get() != ViewLatency { + t.Errorf("SetByName error should not change view, got %v", v.Get()) + } +} + +// TestSetByNameEmpty tests that SetByName with empty string selects first view. +func TestSetByNameEmpty(t *testing.T) { + defs := []viewDef{ + {code: ViewLatency, name: "table_io_latency", table: "performance_schema.table1", selectable: true}, + {code: ViewOps, name: "table_io_ops", table: "performance_schema.table2", selectable: true}, + } + manager := mockViewManager(defs) + v := mockView(manager, ViewOps) + + err := v.SetByName("") + if err != nil { + t.Fatalf("SetByName(\"\") unexpected error: %v", err) + } + if v.Get() != ViewLatency { + t.Errorf("SetByName(\"\") should select first view (ViewLatency), got %v", v.Get()) + } +} + +// TestSetByNameValid tests that SetByName sets the correct view using a real view name. +// This test works with the global allViewsDef so we must use the exact names it defines. +func TestSetByNameValid(t *testing.T) { + // Build a manager containing a view that matches one in the real allViewsDef. + // We'll use ViewOps which has name "table_io_ops" in allViewsDef. + defs := []viewDef{ + {code: ViewLatency, name: "table_io_latency", table: "performance_schema.table_io_waits_summary_by_table", selectable: true}, + {code: ViewOps, name: "table_io_ops", table: "performance_schema.table_io_waits_summary_by_table", selectable: true}, + } + manager := mockViewManager(defs) + v := mockView(manager, ViewLatency) + + // Use the real view name from allViewsDef + err := v.SetByName("table_io_ops") + if err != nil { + t.Fatalf("SetByName(\"table_io_ops\") unexpected error: %v", err) + } + if v.Get() != ViewOps { + t.Errorf("SetByName(\"table_io_ops\") expected ViewOps, got %v", v.Get()) + } +}