java and python implementations from maxmind are very different #39
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi, I haven’t looked at the IPv4 root node computation in such a long time that it’s almost like someone else’s code to me at this point! From what I can gather out of your PR, the spec says one thing but implementations out there (official ones) sometimes (always?) do another. Are the two compatible by chance? I haven’t encountered any issues with this but I’ve only used MaxMind databases so far. Thanks for the heads up, ping me again if a fix is needed. |
|
Hi Guilherme thank you for looking at this! My PR needs a ton of work—that’s why I closed it. I’m going to test more with some of the databases I have and I’ll re-open if I can submit something that more closely matches the maxmind implementations
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Guilherme Andrade ***@***.***>
Sent: Monday, February 5, 2024 9:58:11 PM
To: g-andrade/locus ***@***.***>
Cc: Nick Jacob ***@***.***>; State change ***@***.***>
Subject: Re: [g-andrade/locus] java and python implementations from maxmind are very different (PR #39)
Hi,
I haven’t looked at the IPv4 root node computation in such a long time that it’s almost like someone else’s code to me at this point!
From what I can gather out of your PR, the spec says one thing but implementations out there (official ones) sometimes (always?) do another. Are the two compatible by chance? I haven’t encountered any issues with this but I’ve only used MaxMind databases so far.
Thanks for the heads up, ping me again if a fix is needed.
—
Reply to this email directly, view it on GitHub<#39 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAJDCSYYYLWAXW2BGSBUQFDYSGL4HAVCNFSM6AAAAABC3FGT7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRYGY4TGMRYGM>.
You are receiving this because you modified the open/close state.Message ID: ***@***.***>
|
This was referenced May 4, 2024
g-andrade
added a commit
that referenced
this pull request
May 4, 2024
--- Fix wrong root in MMDB tree parser, which failed IPv4 lookups in IPv6 databases that don't include IPv4-mapped IPv6 addresses. A bug that was part of locus since forever but didn't show up in MaxMind databases since those appear to always include the IPv4-mapped IPv6 addresses node. Thanks to https://github.com/nickjacob who left me the clues I needed in an earlier closed PR: * #39
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
yes.. to be fair the java & python implementation of this are more legit:
https://github.com/maxmind/MaxMind-DB-Reader-python/blob/main/maxminddb/reader.py#L128
(java the same).
I don't feel like it's fair to actually upstream this.. unless I implemented it that way & added tests. See https://maxmind.github.io/MaxMind-DB/#ipv4-addresses-in-an-ipv6-tree this spec sounds more like what locus did.. but what maxmind does is what the writer does.. so...
tested this on a few DBs that our code created & it's the same each time. Maybe I need to implement the offset thing.
With this patch/version.. we can produce a hybrid ipv4 & ipv6 DB from one input file from IP2location