From 391493f065337a4814f49e91deddaf60f242a819 Mon Sep 17 00:00:00 2001 From: davidovich Date: Fri, 13 Oct 2023 13:39:38 -0400 Subject: [PATCH] Fix potential panic on AdjacencyMap Because the store could theoretically be updated without the use of the main `AddVertex` and `AddEdge` APIs (which does some input validation), calculating the `AdjencyMap` could provoke an "assignment to entry in nil map" panic if the store reported edges that were between unknown vertices. Since the edges are always between known vertices, it is an error to try and match a missing source vertex from the vertex set. If that happens, it is a data desynchronization error that we report to the user. In order to test this pathological case, we needed to modify the test to use the store directly because the "front-facing" API would not allow us to add Edges on missing vertex. fixes #102 Signed-off-by: davidovich --- directed.go | 3 +++ directed_test.go | 35 ++++++++++++++++++++++++++++------- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/directed.go b/directed.go index 95d67f52..84bb47a9 100644 --- a/directed.go +++ b/directed.go @@ -205,6 +205,9 @@ func (d *directed[K, T]) AdjacencyMap() (map[K]map[K]Edge[K], error) { } for _, edge := range edges { + if _, ok := m[edge.Source]; !ok { + return nil, fmt.Errorf("unknown source vertex in edge: %v", edge.Source) + } m[edge.Source][edge.Target] = edge } diff --git a/directed_test.go b/directed_test.go index 34a9c8da..3f559e14 100644 --- a/directed_test.go +++ b/directed_test.go @@ -918,9 +918,10 @@ func TestDirected_RemoveEdge(t *testing.T) { func TestDirected_AdjacencyList(t *testing.T) { tests := map[string]struct { - vertices []int - edges []Edge[int] - expected map[int]map[int]Edge[int] + vertices []int + edges []Edge[int] + expected map[int]map[int]Edge[int] + wantError bool }{ "Y-shaped graph": { vertices: []int{1, 2, 3, 4}, @@ -964,22 +965,42 @@ func TestDirected_AdjacencyList(t *testing.T) { 4: {}, }, }, + "missing-vertice-in-edge": { + vertices: []int{1, 2}, + edges: []Edge[int]{ + {Source: 3, Target: 2}, + {Source: 1, Target: 3}, + }, + expected: map[int]map[int]Edge[int]{}, + wantError: true, + }, } for name, test := range tests { graph := newDirected(IntHash, &Traits{}, newMemoryStore[int, int]()) + // we populate the graph with direct store access to simulate + // pathological cases where the store could be populated out of band + // of the normal graph construction API for _, vertex := range test.vertices { - _ = graph.AddVertex(vertex) + _ = graph.store.AddVertex(graph.hash(vertex), vertex, VertexProperties{}) } for _, edge := range test.edges { - if err := graph.AddEdge(edge.Source, edge.Target, EdgeWeight(edge.Properties.Weight)); err != nil { - t.Fatalf("%s: failed to add edge: %s", name, err.Error()) + err := graph.store.AddEdge(edge.Source, edge.Target, + Edge[int]{ + Source: edge.Source, Target: edge.Target, + Properties: EdgeProperties{Weight: edge.Properties.Weight}, + }) + if err != nil { + t.Fatalf("%s: failed to add edge to store: %s", name, err.Error()) } } - adjacencyMap, _ := graph.AdjacencyMap() + adjacencyMap, err := graph.AdjacencyMap() + if test.wantError && err == nil { + t.Fatalf("%s: should have failed test", name) + } for expectedVertex, expectedAdjacencies := range test.expected { adjacencies, ok := adjacencyMap[expectedVertex]