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

[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.

[Ajay-26]

Hi, I would like to have a crack at this issue. I'm pretty new to the open source world, and this would be my first contribution. I am currently reading through the documentation and contribution guidelines. Thank you for your patience :)

[nolankramer]

@Ajay-26 Thanks! Go for it!

[Ajay-26]

Hi @nolankramer, I just made a PR from my branch to the head of the repo. I see that the test for Labeler / triage (pull_request_target) has failed, but I can't make much sense of the feedback from the test. I'm trying to fix that before I complete my PR, could you guide me on how to do it ? I can retract this PR and work on those changes.

[nolankramer]

@Ajay-26 No need to retract the PR. It seems like an unrelated issue. Feel free to ignore it.

[nolankramer]

I've added some feedback on the PR to point you in the right direction :)