Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
This achieves the essential ask of #9, but goes about it a bit differently in deference to various factors: * I don't like RemoteIp.last_forwarded_ip/2 as a public name. It doesn't really read well. I opted for RemoteIp.from/2 - you're fetching the remote IP _from_ the given headers. * More thorough documentation, including doctests. * Given the addition of the :clients option, the code wouldn't have merged cleanly anyway. * The pull request didn't handle option parsing correctly. The keyword extraction had been copied from RemoteIp.init/1 into RemoteIp.last_forwarded_ip/2, which was still being called by RemoteIp.call/2. This is unnecessary code duplication at best, since we could just call init/1 to get the extracted options from our public "gimme the IP" function. But the PR implementation was more subtly wrong than that: it wasn't parsing the :proxies option with InetCidr, but it was still appending the *raw* reserved addresses. So not only was it doubling up the length of the :proxies when RemoteIp was used as a plug (since init/1 already appends reserved addresses), it was mixing parsed IP ranges with strings. If it weren't for the fact that InetCidr.contains?/2 happens to have a catch-all false clause [1], the tests would have failed outright. So what if we just appended & parsed those IP ranges in the shared last_forwarded_ip/2 function and not in init/1? Also the wrong approach, because the pull request still had RemoteIp.call/2 calling that function at runtime. By default, init/1 is invoked at compile time [2] to pass the processed options into call/2. By pushing option parsing (including constructing a MapSet and invoking InetCidr.parse/1, which aren't necessarily trivial) out of init/1 and into last_forwarded_ip/2, we'd be forcing call/2 to do redundant runtime computation on every request through the pipeline. All of those issues are settled quite handily by instead adding a completely new function, RemoteIp.from/2. It parses the options with RemoteIp.init/1 (there's no real way of getting around doing this at runtime for such a function), then passes those along to the *private* RemoteIp.last_forwarded_ip/2, which now takes in request headers instead of a %Plug.Conn{} (which affects all the downstream functions, of course). But when RemoteIp is used as a plug, we don't have to recompute the options at runtime because (a) call/2 doesn't touch from/2 and (b) the call to init/1 is still where all the work is being done, and it'll be compiled into the pipeline as per usual. * Since test/remote_ip_test.exs was overhauled by prior work for the :clients option, it was easier to hook into a common helper function to make assertions about *every* call we make to RemoteIp.from/2, guaranteeing it outputs the same thing as RemoteIp.call/2. The output is only the same in these tests because we don't give the %Plug.Conn{} struct a default :remote_ip field, so it's just nil. In real life, this would be populated by the peer address, which would be a different return value from RemoteIp.from/2 (since it doesn't have access to the Conn). But in this test suite, it was an easier change for the sake of more thorough coverage with less test duplication (which is already honestly a bit rampant). This closes #9. [1] https://github.com/Cobenian/inet_cidr/blob/ffce709/lib/inet_cidr.ex#L78) [2] https://hexdocs.pm/plug/Plug.Builder.html#module-options
- Loading branch information