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

iplookup #18

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

iplookup #18

wants to merge 16 commits into from

Conversation

jnovack
Copy link

@jnovack jnovack commented Jul 26, 2018

parse-raw-data.js was b0rked after the concat patch so this fixes that fix for domain lookups and corrects base output for ip lookups. (#3 #4 #17).

An IP Lookup can return MULTIPLE entries (when domains are reallocated or reassigned). This PR returns all the IP ranges in reverse order so the smallest/most accurately allocated range is returned as .[0], the largest range or owner will return as .[length-1].

When there is only 1 return (for a domain or ip lookup), it returns an object {}, however, when there are multiple returns, it returns an array of objects [ {}, ... ]. I do not especially like this, as the upstream code then must figure out if .key or .[0].key is the right key they require. HOWEVER, this is a backwards-compatible change.

I HEAVILY RECOMMEND fully bumping the version 2.x.x -> 3.0.0 and always implementing this as an array of objects, so regardless of a single or multiple returns, a key in the first result will always be [0].key. This would be a breaking change, thus it warrants a major version bump. I will start on that with your permission.

I have added tests for everything (my new adds for ip lookups and tests for verbose lookups (as the structure is different)).

@mikemaccana
Copy link
Owner

mikemaccana commented Jul 27, 2018

Thanks Justin! I don't do IP lookups much so this deserves some attantion. I appreciate the seperation of patches too.

Can you add a test that fails on the current code (with the concat b0rking) and is fixed by your changes?

I'd totally support always returning an awway of results. Happy to accept a patch, with a mjaor version bump, as long as you update the docs too.

@jnovack
Copy link
Author

jnovack commented Jul 27, 2018

My use case was entirely ip lookups, so it was either write from scratch or modify. I appreciate you being amenable. :) I shall complete the v3.0.0 tasks as part of this PR.

The test.js file from 01cb33c has "converts raw ip data into JS" which was created to address the b0rking.

https://github.com/jnovack/whois-json/blob/01cb33c07059c129df7ee7fe03740e7d009e65b9/test/tests.js#L161

For the test, I queried 104.144.194.1.

@jnovack
Copy link
Author

jnovack commented Oct 15, 2019

resolved merge conflicts.

@jnovack
Copy link
Author

jnovack commented Oct 15, 2019

To summarize, v3.0.0 (this PR) is about returning ALL data from whois, including redirects and reallocations in order to find the smallest range possible and owner.

This was a BREAKING change because whois() is now returning an array of objects [ {}, ... ] rather than just an object {}.

Tests have been updated to compensate.

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