Dynamic node removal

[Totodore]

Add a remove fn to the radix tree

For the moment this feature consist only of :

• Finding the node that need to be removed
• Once found, if the node has any children, only the value si taken back
• Otherwise the entire leaf is dropped.

Remaining things to do, are:

• [x] Testing node removal and ensuring that there is no false positive for the node research
• [ ] Removing parent nodes if they became empty / useless (maybe with backtracking ?)

@ibraheemdev I would like your feedback on the implementation as I don't master radix tree.

Side note: The Display implementation is only for testing purposes and can be gated with a CFG(test) or removed

• Fixes #44

[Totodore]

I tried to think about the "node shrinking optimisation" when we remove a node (merging nodes together). What we could do is having a visited node stack and then unwind the stack and check if nodes can be merged. My main issue is that it requires to hold multiple mutable references so the rust borrowing rules don't let me do that...

[Totodore]

Hello @ibraheemdev, do you have an idea when you will be able to review the tests I added after your feedback? Thanks! :)

[ibraheemdev]

The changes look good, thanks. I'll try to have this merged as part of 0.8, which should be finished soon.

[ibraheemdev]

@Totodore I think you need to make sure the denormalized params match before doing a removal. Right now remove("/{x}") will match and remove a route registered as "/{id}". If you can fix that bug and the merge conflicts (the integration test setup was reworked), I think this can be merged.

[Totodore]

@ibraheemdev I have updated the remove fn to work with {x} params. I have two remaining questions:

• If an error occurs when normalizing the path, it is discarded and None is returned as if the value was not found. I think it is ok to do that but maybe you would prefer to return a Result<Option<T>>?
• Because the path is normalized and that currently I don't check the original param name provided. If I insert /foo/{bar}/test and that I want to remove it later I can do it with any param name, e.g. /foo/{azd}/test. Should I check the original one when searching the node?

Also I added some tests but I'll try to add some more as soon as possible.

[ibraheemdev]

If an error occurs when normalizing the path, it is discarded and None is returned as if the value was not found.

I think that's fine, an invalid route could never be in the router. Maybe add a note in the documentation that the function also returns None for invalid routes.

Should I check the original one when searching the node?

Yes, I think this would be more intuitive behavior. You can just check that the parameter remapping on the final node is equal to the remapping created for the route.

[Totodore]

Done :)

[Totodore]

@ibraheemdev I'm currently working on optimizing the tree after a node removal. It consists of two actions:

• Rewind the branch and check if nodes can be removed (only one child without value). Stop at the first non-removable node.
• Rewind the branch and check if nodes can be merged together (only one, non-wild child that doesn't have a value).

These two actions are a bit more difficult to implement and might compromise the tree in case of error. Therefore do you think it is necessary to have them for this PR or it is better to implement this later in another one?

[ibraheemdev]

@Totodore thanks for continuing to work on this. It looks good for now, I'll take a closer look and try to have it merged this weekend.

[Totodore]

Hey @ibraheemdev any knews ? If you don't have the time to review this at the moment, I'll would still be interested to work on these two features:

• Rewind the branch and check if nodes can be removed (only one child without value). Stop at the first non-removable node.
• Rewind the branch and check if nodes can be merged together (only one, non-wild child that doesn't have a value).

These two actions are a bit more difficult to implement and might compromise the tree in case of error. Therefore do you think it is necessary to have them for this PR or it is better to implement this later in another one?

[ibraheemdev]

This looks great, thanks! Feel free to open a new PR if you are interested in working on this further, but you can just leave an issue for now describing the optimizations.