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.

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

Add within support to allow iterating over IpNetworks (cidrs) #50

Closed
wants to merge 18 commits into from

Conversation

ross
Copy link
Contributor

@ross ross commented Jul 20, 2021

This is akin to golang's within support, mostly implemented in https:/oschwald/maxminddb-golang/blob/main/traverse.go, but the implementation itself is done from scratch to better match the style and details of this rust implementation.

It's still a work in progress, mostly kicking the tires and iterating towards a working/clean solution. The status is roughly:

  • Implement the algorithm to search for the starting node
  • Stack based in-order traversal (which will be well suited to convert to an iterator)
  • Implement the iterator with an item that that includes both the network and info
  • Make a quick example in the spirit of lookup.rs
  • Make sure this is on the right track and is something that would be shippable
  • Figure out if there's a cleaner way to get the Reader's type parameter into Within to avoid having to _ it.
  • Error handling
  • Organize/layout the code better
  • Write tests & search for edge cases
    • Especially the IPv6 bits
  • Cleanup/remove prints and other debugging bits
  • ...

}

impl<'de, T: Deserialize<'de>, S: AsRef<[u8]>> Iterator for Within<'de, T, S> {
type Item = Result<WithinItem<T>, MaxMindDBError>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fairly new to rust (in case it's not obvious) and I'm not finding a clean way to go about returning errors that happen during the walking of nodes. My searching for idiomatic patterns didn't turn anything up, but it's a tough thing to search for.

This results in some pretty ugly code below to map errors in the helper functions into Some(Err(e)). That seems bad enough, but currently have have things in match Ok/Err conditionals that make it tough to read. Things like .map_err won't let me wrap with Some so I don't see a cleaner way. Seems like either a shortcoming of iterators or I'm just going about this all wrong 😁

Input/thoughts/suggestions welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like there may be a crate, https://docs.rs/fallible-iterator/0.2.0/fallible_iterator/ that calls out the situation.

Seems like using it might be the cleaner way to go, but will defer to more experienced opinions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a FallibleIterator based solution and it cleans up pretty much all of the ugliness in error handling here. It's not perfect since you can't use for x in y, and have to go with while let Some(x) = y.next(), but that's a limitation of rust and seems like a reasonable tradeoff for cleaner error handling.

Changes for that can be seen with https:/ross/maxminddb-rust/compare/within-support...ross:within-support-fallibleiterator?expand=1. Thoughts welcome.

Copy link
Owner

Choose a reason for hiding this comment

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

Although it does clean up the code somewhat, I think I'd prefer not having the dependency, especially given that the fallible iterator crate hasn't seen an update in two years and doesn't seem that widely used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it hasn't seen any updates b/c it does what it needs to do. Similar to how I wrote requests-futures 8 years ago and haven't really touched it meaningfully in most of that time.

As for not wanting the dep, I don't have a response to that. The code to implement the non-fallible iterator is ugly as hell, but using it isn't THAT bad. Kind of seems like a shortcoming of Rust itself that iterators can't fail.

I'll stick this back on my TODO list to pick back up and see if I can get it over the line.

@ross ross marked this pull request as ready for review August 8, 2021 19:38
@ross ross changed the title [Draft] Add within support to allow iterating over IpNetworks (cidrs) Add within support to allow iterating over IpNetworks (cidrs) Aug 8, 2021
@ross
Copy link
Contributor Author

ross commented Aug 8, 2021

I think this is ready for 👀 now.

@oschwald
Copy link
Owner

Sorry for the delay! I forgot about this PR. I've merged it via the CLI. One notable change that I made is that it now skips aliases of the IPv4 network (besides ::/96). I think this is the behavior that generally makes the most sense.

I plan on releasing later today.

@oschwald oschwald closed this Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants