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.

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

Bug Fix for #418 - Graph<T>::removeNode has potential to throw due to optional being accessed early #430

Merged
merged 6 commits into from
Jun 20, 2024
Merged

Bug Fix for #418 - Graph<T>::removeNode has potential to throw due to optional being accessed early #430

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0be3518
optional should first check whether the node indeed exists
Ajay-26 May 6, 2024
5ced829
fixed typo in impl and also added test for removing node that was nev…
Ajay-26 May 6, 2024
d8d07c1
incorporated review into PR
Ajay-26 May 6, 2024
7f1efe6
resolved use after free for map iterator
Ajay-26 May 7, 2024
4489104
reformatted code as per PR feedback
Ajay-26 May 7, 2024
6f4e654
reformatted code as per PR feedback
Ajay-26 May 7, 2024
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: 5 additions & 2 deletions include/CXXGraph/Graph/Graph_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,11 @@ void Graph<T>::removeEdge(const CXXGraph::id_t edgeId) {
template <typename T>
void Graph<T>::removeNode(const std::string &nodeUserId) {
auto nodeOpt = getNode(nodeUserId);
auto isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value());

auto isolatedNodeIt = isolatedNodesSet.end();
if (nodeOpt) {
isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value());
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems ok, but it would be more concise if the entire downstream logic were gated by nodeOpt, i.e.:

auto nodeOpt = getNode(nodeUserId);

if (nodeOpt)
{
  auto isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value());

  if (isolatedNodeIt != isolatedNodesSet.end()) {
    // Remove the isolated node
     ...
  } else {
    // Remove the node and connected edges
     ...
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback, I made a commit with the suggested edits - let me know if it looks okay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good. Approved

if (nodeOpt.has_value() && isolatedNodeIt != isolatedNodesSet.end()) {
// The node is isolated
isolatedNodesSet.erase(isolatedNodeIt);
Expand Down
39 changes: 39 additions & 0 deletions test/GraphTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1340,6 +1340,45 @@ TEST(TestRemoveNode, Test_connectedNode) {
ASSERT_EQ(graph.getEdgeSet().size(), 1);
}

TEST(TestRemoveNode, Test_removeInvalidNode) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

/** Test to call the remove_node function on a node that was never added. In this case getNode will return an optional that is nullptr*/
// Create a graph with 3 nodes and 3 edges.
CXXGraph::Node<int> node1("1", 1);
CXXGraph::Node<int> node2("2", 2);
CXXGraph::Node<int> node3("3", 3);
CXXGraph::DirectedEdge<int> edge1(1, node1, node2);
CXXGraph::DirectedEdge<int> edge2(2, node2, node1);
CXXGraph::DirectedEdge<int> edge3(3, node1, node3);
CXXGraph::T_EdgeSet<int> edgeSet;
// Add the 3 edges into the graph.
edgeSet.insert(make_shared<CXXGraph::Edge<int>>(edge1));
edgeSet.insert(make_shared<CXXGraph::Edge<int>>(edge2));
edgeSet.insert(make_shared<CXXGraph::Edge<int>>(edge3));
// Initialise the graph
CXXGraph::Graph<int> graph(edgeSet);

// Check the initial number of edges and nodes. Everything should be okay so far
ASSERT_EQ(graph.getNodeSet().size(), 3);
ASSERT_EQ(graph.getEdgeSet().size(), 3);

// Remove a node that was never in the graph
graph.removeNode("4");

// Number of nodes and edges in the graph should remain the same
ASSERT_EQ(graph.getNodeSet().size(), 3);
ASSERT_EQ(graph.getEdgeSet().size(), 3);

// Remove an existing node, the edge associated with that node should also be removed. Node "3" had just outgoing edge, so there should now be 2 nodes and 2 edges.
graph.removeNode("3");
ASSERT_EQ(graph.getNodeSet().size(), 2);
ASSERT_EQ(graph.getEdgeSet().size(), 2);

// Remove the node that had already been removed. Should not change anything about the graph now, similar to when "4" was removed above
graph.removeNode("3");
ASSERT_EQ(graph.getNodeSet().size(), 2);
ASSERT_EQ(graph.getEdgeSet().size(), 2);
}

TEST(TestGetNode, Test_1) {
CXXGraph::Node<int> node1("1", 1);
CXXGraph::Node<int> node2("2", 2);
Expand Down
Loading