Add iteration mechanism #131

faktas2 · 2023-11-02T06:08:30Z

No description provided.

oschwald

I am going to have to do another closer pass, but here are some initial comments.

src/main/java/com/maxmind/db/BadVersionException.java

oschwald · 2023-11-02T19:16:14Z

src/main/java/com/maxmind/db/Reader.java

+     * Please note that a MaxMind DB may map IPv4 networks into several locations
+     * in an IPv6 database. This iterator will iterate over all of these locations
+     * separately. To only iterate over the IPv4 networks once, use the
+     * SkipAliasedNetworks option.


I think I mentioned this in the story, but I think we should make skipping the aliased networks the default. It is what most users want. The Go library is the other way only for backwards compatibility. Perhaps we could rename it to be something like includeAliasedNetworks as well.

Sorry it is enabled by default below. I think the comment is misleading. I've fixed it.

src/main/java/com/maxmind/db/Networks.java

oschwald · 2023-11-02T19:21:28Z

src/main/java/com/maxmind/db/Networks.java

+    }
+
+    /**
+     * Returns the next NetworksItem. You need to set the class using


At first, I thought "NetworksItem" referred to a particular type, but I don't see it used anywhere else. Maybe we could just say "the next item".

Sorry, that is supposed to be DataRecord. I had created another class just like DataRecord until realizing DataRecord did the job.

oschwald · 2023-11-02T19:29:11Z

src/main/java/com/maxmind/db/Networks.java

+     * Returns if Networks had any errors.
+     * @return Exception The exception to the Networks iteration.
+     */
+    public Exception getErr() {


Do other iterators in Java use this pattern? I don't recall seeing it before. I am a bit concerned people will not bother checking it and miss that there was an exception. Perhaps a better alternative would be to create a NetworksIterationException (or something similar) that extends RuntimeException and then wrap the checked exceptions in that and rethrow them.

oschwald · 2023-11-02T19:52:40Z

src/main/java/com/maxmind/db/Networks.java

+        }
+        while (!this.nodes.isEmpty()) {
+            // Pop the last one.
+            NetworkNode node = this.nodes.remove(this.nodes.size() - 1);


I wonder if a Stack would be a more natural data structure than an ArrayList.

oschwald · 2023-11-02T19:59:41Z

src/main/java/com/maxmind/db/Reader.java

+     * @throws ClosedDatabaseException Exception for a closed databased.
+     * @throws InvalidDatabaseException Exception for an invalid database.
+     */
+    public <T> Networks<T> networksWithIn(Network network, boolean skipAliasedNetworks) 


Suggested change

public <T> Networks<T> networksWithIn(Network network, boolean skipAliasedNetworks)

public <T> Networks<T> networksWithin(Network network, boolean skipAliasedNetworks)

src/main/java/com/maxmind/db/Reader.java

oschwald · 2023-11-02T20:04:26Z

src/main/java/com/maxmind/db/Reader.java

+     * @param bitCount The prefix.
+     * @return int[]
+     */
+    public int[] traverseTree(byte[] ip, int node, int bitCount)


I think this should be private. It doesn't make sense for it to be part of the public API.

oschwald · 2023-11-02T20:07:43Z

src/main/java/com/maxmind/db/Reader.java

@@ -149,6 +153,10 @@ public <T> T get(InetAddress ipAddress, Class<T> cls) throws IOException {
        return getRecord(ipAddress, cls).getData();
    }

+    protected int getIpv4Start() {


I think it would make more sense if this and the other methods were package private rather than protected. I suppose it doesn't matter too much as the class is final, but I think it would make it clearer that it isn't part of the API.

oschwald

I had a few more comments. Also, could you add something to the README.md and the CHANGELOG.md files?

oschwald · 2023-11-06T18:48:10Z

src/main/java/com/maxmind/db/Networks.java

+
+            return new DatabaseRecord<T>(data, InetAddress.getByAddress(ip), prefixLength);
+        } catch (IOException e) {
+            throw new NetworksIterationException(e.getMessage());


I think we should wrap the whole and set the cause to the original exception. This will allow users to access all the data in the original.

Suggested change

throw new NetworksIterationException(e.getMessage());

throw new NetworksIterationException(e);

oschwald · 2023-11-06T18:48:32Z

src/main/java/com/maxmind/db/Networks.java

+                    this.nodes.push(new NetworkNode(ipRight, node.prefix, rightPointer));
+                    node.pointer = this.reader.readNode(this.buffer, node.pointer, 0);
+                } catch (InvalidDatabaseException e) {
+                    throw new NetworksIterationException(e.getMessage());


Suggested change

throw new NetworksIterationException(e.getMessage());

throw new NetworksIterationException(e);

src/main/java/com/maxmind/db/NetworksIterationException.java

oschwald · 2023-11-06T18:53:25Z

src/main/java/com/maxmind/db/Networks.java

+     * Sets the Class for the data type in DataRecord.
+     * @param cls The class object. ( For example, Map.class )
+     */
+    public void setDataClass(Class<T> cls) {


Is there any reason why this can't just be part of the constructor?

If we do need it, it should be package private, but I don't think we need it.

The code in go lets people retrieve data into different types when network(result any) is called. I thought we would want keep that. But, I agree. I'll move it to the constructor.

I don't think that would have worked here anyway given that the class Networks<T> has a type parameter and the T here needs to match that.

oschwald · 2023-11-06T18:54:21Z

src/main/java/com/maxmind/db/Networks.java

+        }
+    }
+
+    public boolean isInIpv4Subtree(byte[] ip) {


I think this should be private.

oschwald · 2023-11-06T18:56:58Z

src/main/java/com/maxmind/db/Networks.java

+         * @param prefix The prefix of the node.
+         * @param pointer The node number
+         */
+        public NetworkNode(byte[] ip, int prefix, int pointer) {


I think you can drop public here.

oschwald · 2023-11-06T18:57:25Z

src/main/java/com/maxmind/db/Networks.java

+ * 
+ * @param <T> The type of data returned by the iterator.
+ */
+public class Networks<T> implements Iterator<DatabaseRecord<T>> {


We should make this final.

src/main/java/com/maxmind/db/Networks.java

oschwald · 2023-11-07T20:15:54Z

src/main/java/com/maxmind/db/Networks.java

+     * For example,
+     *  networks.prepareForClass(Map.Class);
+     *  Map test = networks.next();


The references to prepareForClass should be removed.

oschwald · 2023-11-07T20:18:16Z

src/main/java/com/maxmind/db/Networks.java

+    }
+
+    /*
+    * Next prepares the next network for reading with the Network method. It


oschwald · 2023-11-07T20:18:58Z

src/main/java/com/maxmind/db/Networks.java

+    /*
+    * Next prepares the next network for reading with the Network method. It
+    * returns true if there is another network to be processed and false if there
+    * are no more networks or if there is an error.


"or if there is an error" can probably be removed now.

I think we should document that it throws NetworksIterationException.

oschwald · 2023-11-07T20:21:08Z

src/main/java/com/maxmind/db/Reader.java

+    /**
+     * Creates a Networks iterator and skips aliased networks.
+     * Please note that a MaxMind DB may map IPv4 networks into several locations
+     * in an IPv6 database. This iterator will iterate over all of these locations


I think this is wrong now. It will only iterate over the canonical location, not the aliases.

oschwald · 2023-11-07T20:22:07Z

src/main/java/com/maxmind/db/Reader.java

+     * Please note that a MaxMind DB may map IPv4 networks into several locations
+     * in an IPv6 database. This iterator will iterate over all of these locations
+     * separately. To disable skipAliasedNetworks, which iterates over the IPv4 networks 
+     * only once, use the SkipAliasedNetworks parameter.


Suggested change

* only once, use the SkipAliasedNetworks parameter.

* only once, use the skipAliasedNetworks parameter.

oschwald · 2023-11-07T20:22:40Z

src/main/java/com/maxmind/db/Reader.java

+     * Please note that a MaxMind DB may map IPv4 networks into several locations
+     * in an IPv6 database. This iterator will iterate over all of these locations
+     * separately. To set the iteration over the IPv4 networks once, use the
+     * SkipAliasedNetworks option.


Suggested change

* SkipAliasedNetworks option.

* skipAliasedNetworks option.

oschwald · 2023-11-07T20:27:07Z

src/main/java/com/maxmind/db/Reader.java

+            InetAddress ipv4 = InetAddress.getByAddress(new byte[4]);
+            InetAddress ipv6 = InetAddress.getByAddress(new byte[16]);
+            Network ipAllV4 = new Network(ipv4, 0); // Mask 32.
+            Network ipAllV6 = new Network(ipv6, 0); // Mask 128.


It would save allocations to only create these when you actually need them (i.e., move the v6 stuff to the if block and the v4 stuff after the if block).

src/main/java/com/maxmind/db/Reader.java

oschwald · 2023-11-07T20:38:27Z

src/main/java/com/maxmind/db/Reader.java

+     * @param bitCount The prefix.
+     * @return int[]
+     */
+    private int[] traverseTree(byte[] ip, int node, int bitCount)


It seems like this could be shared with getRecord. Also, that has some improvements over this, e.g., the use of startNode.

faktas2 · 2023-11-10T15:36:56Z

src/test/java/com/maxmind/db/ReaderTest.java

+            "mixed",
+            new String[]{
+                // ::/96
+                "1.1.1.1/32",


I had to change the ::/96 results to the ipv4 versions after making the change you suggested. Could you please check if this is correct?

This is right based on the database specification, but the ones at ::ffff:0:0/96 should look like ::ffff:1.1.1.1/128 or ::ffff:101:101/128. This is what I was referring to when we talked.

However, I just looked at the InetAddress docs, and it looks like this has behavior similar to net.IP in Go:

In InetAddress and Inet6Address, it is used for internal representation; it has no functional role. Java will never return an IPv4-mapped address. These classes can take an IPv4-mapped address as input, both in byte array and text representation. However, it will be converted into an IPv4 address.

Given that it isn't possible to return ::ffff:1.1.1.1 or whatever, I suppose it would be best to just embrace it. I think we should switch it back to something more similar to what you had. Sorry about that!

Another option would be to just remove the option. Including the aliased networks seems kind of a niche need. I didn't include them with the Python version.

oschwald · 2023-11-13T18:10:43Z

README.md

+
+```java
+Reader reader = new Reader(file);
+Networks networks = reader.networks(false, Map.class);


Suggested change

Networks networks = reader.networks(false, Map.class);

Networks networks = reader.networks(Map.class);

Given that we have the overload, we may as well use it here.

oschwald · 2023-11-13T18:19:57Z

README.md

+Reader reader = new Reader(file);
+Networks networks = reader.networks(false, Map.class);
+
+while(networks.hasNext()) {


It would have been nice if we could implement Iterable, but I guess that isn't really possible given that we need to pass the class through. I guess it doesn't matter too much.

oschwald · 2023-11-13T18:29:23Z

src/test/java/com/maxmind/db/ReaderTest.java

+            "mixed",
+            new String[]{
+                // ::/96
+                "1.1.1.1/32",


This is right based on the database specification, but the ones at ::ffff:0:0/96 should look like ::ffff:1.1.1.1/128 or ::ffff:101:101/128. This is what I was referring to when we talked.

However, I just looked at the InetAddress docs, and it looks like this has behavior similar to net.IP in Go:

In InetAddress and Inet6Address, it is used for internal representation; it has no functional role. Java will never return an IPv4-mapped address. These classes can take an IPv4-mapped address as input, both in byte array and text representation. However, it will be converted into an IPv4 address.

Given that it isn't possible to return ::ffff:1.1.1.1 or whatever, I suppose it would be best to just embrace it. I think we should switch it back to something more similar to what you had. Sorry about that!

Another option would be to just remove the option. Including the aliased networks seems kind of a niche need. I didn't include them with the Python version.

oschwald

Looks good. I had a couple of small style changes.

oschwald · 2023-11-15T16:43:20Z

src/main/java/com/maxmind/db/NetworksIterationException.java

+/**
+ * This class represents an error encountered while iterating over the networks.
+ * The most likely causes are corrupt data in the database, or a bug in the reader code.
+ * 


Did you mean to have a <p> here?

oschwald · 2023-11-15T16:43:53Z

src/main/java/com/maxmind/db/Reader.java

+    /**
+     * Returns the node number and the prefix for the network.
+     * @param ip The ip address to travese.
+     * @param node The number of the node.


This param doesn't exist.

oschwald · 2023-11-15T16:45:46Z

src/main/java/com/maxmind/db/Reader.java

+
+    /**
+     * Returns the node number and the prefix for the network.
+     * @param ip The ip address to travese.


oschwald · 2023-11-15T16:47:43Z

src/main/java/com/maxmind/db/Networks.java

+        this.reader = reader;
+        this.includeAliasedNetworks = includeAliasedNetworks;
+        this.buffer = reader.getBufferHolder().get();
+        this.nodes = new Stack<NetworkNode>();


Suggested change

this.nodes = new Stack<NetworkNode>();

this.nodes = new Stack<>();

oschwald · 2023-11-15T16:48:38Z

src/main/java/com/maxmind/db/Networks.java

+                prefixLength -= 96;
+            }
+
+            return new DatabaseRecord<T>(data, InetAddress.getByAddress(ip), prefixLength);


Suggested change

return new DatabaseRecord<T>(data, InetAddress.getByAddress(ip), prefixLength);

return new DatabaseRecord<>(data, ipAddr, prefixLength);

oschwald · 2023-11-15T16:49:17Z

src/main/java/com/maxmind/db/Reader.java

+        Networks<T> networks = new Networks<T>(this, includeAliasedNetworks,
+            new Networks.NetworkNode[]{new Networks.NetworkNode(ipBytes, prefix, node)},
+                typeParameterClass);
+
+        return networks;


Suggested change

Networks<T> networks = new Networks<T>(this, includeAliasedNetworks,

new Networks.NetworkNode[]{new Networks.NetworkNode(ipBytes, prefix, node)},

typeParameterClass);

return networks;

return new Networks<>(this, includeAliasedNetworks,

new Networks.NetworkNode[]{new Networks.NetworkNode(ipBytes, prefix, node)},

typeParameterClass);

faktas2 force-pushed the fatih/add-iterator branch 3 times, most recently from 9d31fc8 to b801661 Compare November 2, 2023 15:10

Add iteration mechanism

fb269dd

faktas2 force-pushed the fatih/add-iterator branch from b801661 to fb269dd Compare November 2, 2023 15:11

oschwald requested changes Nov 2, 2023

View reviewed changes

faktas2 added 7 commits November 3, 2023 12:53

Rename BadVersionException to InvalidNetworkException

0991705

Add javadocs for Networks

717eb70

Throw RuntimeException

1578f18

Use Stack instead of ArrayList

76092fb

Swap protected with no-modifier

6c3a972

Change docs on Reader.networks()

29b5c1a

Rename to NetworksWithin

34c911f

faktas2 force-pushed the fatih/add-iterator branch from 3563ca7 to 34c911f Compare November 3, 2023 20:47

oschwald requested changes Nov 6, 2023

View reviewed changes

faktas2 added 3 commits November 7, 2023 09:48

Remove setDataClass

be0e074

Make Networks final, drop public

4d93a71

Add README and CHANGELOG

899d6ec

faktas2 force-pushed the fatih/add-iterator branch from 6a54119 to 899d6ec Compare November 7, 2023 20:30

oschwald requested changes Nov 7, 2023

View reviewed changes

faktas2 added 4 commits November 9, 2023 16:47

Rename skipAliasedNetworks to includeAliasedNetworks

acb2459

Make less allocations

5a5e7bc

Remove converting to 16 bytes

ae8ca1c

Share traverseTree

7c6a5e1

faktas2 force-pushed the fatih/add-iterator branch from 9b5ec03 to 7c6a5e1 Compare November 10, 2023 15:34

faktas2 commented Nov 10, 2023

View reviewed changes

oschwald requested changes Nov 13, 2023

View reviewed changes

faktas2 added 2 commits November 14, 2023 14:21

Add ipv4 handling back

326e6ea

Use the overload in README

38076c2

oschwald approved these changes Nov 15, 2023

View reviewed changes

oschwald requested changes Nov 15, 2023

View reviewed changes

Add <p>, remove IPV6_LEN, return directly

e59f46d

oschwald approved these changes Nov 16, 2023

View reviewed changes

oschwald merged commit 47492ef into main Nov 16, 2023

oschwald deleted the fatih/add-iterator branch November 16, 2023 18:19

oschwald mentioned this pull request Nov 21, 2023

Is it possible to iterate over all records of a GeoIP database #42

Closed

	public <T> Networks<T> networksWithIn(Network network, boolean skipAliasedNetworks)
	public <T> Networks<T> networksWithin(Network network, boolean skipAliasedNetworks)

	throw new NetworksIterationException(e.getMessage());
	throw new NetworksIterationException(e);

	* only once, use the SkipAliasedNetworks parameter.
	* only once, use the skipAliasedNetworks parameter.

	Networks networks = reader.networks(false, Map.class);
	Networks networks = reader.networks(Map.class);

	this.nodes = new Stack<NetworkNode>();
	this.nodes = new Stack<>();

	return new DatabaseRecord<T>(data, InetAddress.getByAddress(ip), prefixLength);
	return new DatabaseRecord<>(data, ipAddr, prefixLength);

Add iteration mechanism #131

Add iteration mechanism #131

Conversation

faktas2 commented Nov 2, 2023

oschwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faktas2 Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oschwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faktas2 Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

oschwald Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oschwald Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

oschwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faktas2 Nov 3, 2023 •

edited

Loading

faktas2 Nov 10, 2023 •

edited

Loading

oschwald Nov 13, 2023 •

edited

Loading

oschwald Nov 13, 2023 •

edited

Loading