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

[Ajay-26]

Pull Request for Bug #418.

• Issue: Graph::removeNode does not check whether the node to be removed is actually present in the graph.
• Changes: Graph::removeNode now checks whether the std::optional node obtained from Graph::getNode() is indeed present in the Graph or not. If not present, the function does nothing. There is also a test called Test_removeInvalidNode within GraphTest.cpp.

[nolankramer]

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

[Ajay-26]

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

[nolankramer]

Looks good. Approved

[nolankramer]

👍

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.87%. Comparing base (1b31e63) to head (6f4e654).
Report is 23 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   97.58%   97.87%   +0.29%     
==========================================
  Files          87       87              
  Lines        9492    10083     +591     
  Branches        0      666     +666     
==========================================
+ Hits         9263     9869     +606     
+ Misses        229      214      -15     

Flag	Coverage Δ
unittests	97.87% <100.00%> (+0.29%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sbaldu]

Here I would use const auto & instead of auto

[Ajay-26]

Hi, thank you for your reply. Yes makes sense to use const auto & - no need to make a copy of the edge set, accidentally missed it. When I make that change and use const auto & edgeset = edgeSet, I run into a segmentation fault on the Test_connectedNode test. Not sure how to deal with it. Trying to look through the code to see what the problem could be

[nolankramer]

This is likely due to the iterator being invalidated after a successful removal (https://stackoverflow.com/a/16904496/1078527).

The solution is to increment the iterator before removing the element.

[nolankramer]

Note, that you'll likely need to use the explicit for loop, instead of iterator-enhanced:

for (auto edgeIt = edgeSet.begin(); edgeIt != edgeSet.end(); ++edgeIt) { ...

This is necessary because std::unordered_set automatically dereferences iterators in iterator-enhanced for loops (which is why it's crashing).

This also means you'll need a dereference on every iterator access, i.e. (*edgeIt)->

[nolankramer]

See https://stackoverflow.com/questions/8234779/how-to-remove-from-a-map-while-iterating-it

[Ajay-26]

Ah! That's awesome, I hadn't seen that! Thank you!

[ZigRazor]

It's ok, we can wait to merge this due to the license change?
What do you think @sbaldu @nolankramer ?