Skip to content

Graph<T>::removeNode has potential to throw due to optional being accessed early #418

@nolankramer

Description

@nolankramer

The implementation that added isolated nodes seems to be assuming that nodeOpt will always have a value. This is untrue if the node does not exist:

template <typename T>
void Graph<T>::removeNode(const std::string &nodeUserId) {
  auto nodeOpt = getNode(nodeUserId);
  auto isolatedNodeIt = isolatedNodesSet.find(nodeOpt.value());
  ...

nodeOpt.value() is called before verifying the optional has something in it.

This should be updated to something like this:

template <typename T>
void Graph<T>::removeNode(const std::string &nodeUserId) {
  auto nodeOpt = getNode(nodeUserId);
  auto isolatedNodeIt = isolatedNodesSet.end();
  if (nodeOpt) {
    isolatedNodeIt  = isolatedNodeIt.find(nodeOpt.value());
  }
  ...

The tests should also be updated to test the case where we try to remove a node that does not exist in the graph.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Priority:HighPriority Label for high priority issuebugSomething isn't workingcoresomething about coreenhancementNew feature or requestgood first issueGood for newcomers

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions