Skip to content

Return err when lookup fails. #52

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

Closed

Conversation

rowland
Copy link

@rowland rowland commented Mar 6, 2019

One would hope to be able to distinguish between an IP address with no registered country (or city) without examining each and every member of the returned struct, but that does not appear to be possible, because maxminddb.Reader.Lookup obscures the difference.

This change breaks many tests that currently expect nil, even though a record was not found.

If it is desired that errors be reserved for operational issues, this information needs to be made available to geoip2 from maxminddb so that geoip2 can return a nil for *City or *Country instead of an error.

It would be possible to work around this issue without breaking the existing API by using LookupOffset and Decode in maxminddb, but the maxminddb.Reader instance is private in geoip2, so 3rd-party code can't reach it.

Alternatively, in order to maintain strict backwards-compatibility, I could implement new methods in geoip2.Reader that would lookup records and behave as desired, but that would mean adding 7 new methods or a new mode for the 7 existing methods.

@oschwald
Copy link
Owner

oschwald commented Mar 6, 2019

This is a dupe of #16. I am not looking to do a breaking release at this time.

Generally, I would recommend checking whether the data you are interested in is present rather than relying on whether the lookup succeeded. Even if the lookup succeeds, it doesn't mean that any particular piece of data will be present in the result.

@oschwald
Copy link
Owner

#59 addresses this issue in a different way with the new LookupNetwork method. Closing in favor of that.

@oschwald oschwald closed this Aug 23, 2019
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