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

module :http_uri not available in OTP release #66

Closed
seanpascoe opened this issue May 19, 2022 · 19 comments
Closed

module :http_uri not available in OTP release #66

seanpascoe opened this issue May 19, 2022 · 19 comments

Comments

@seanpascoe
Copy link

I'm using the URLValidator.validate_url function and it works great in development. I created a phoenix production release and it is erroring at that function with ** (UndefinedFunctionError) function :http_uri.parse/1 is undefined (module :http_uri is not available). I'm fairly new to elixir and I'm having a hard time figuring out why that erlang module wouldn't be available in the release.

@aparod
Copy link

aparod commented Jun 16, 2022

You're probably running different versions of Erlang between your dev and prod environments. I'm on Erlang/OTP 24 and it gives me a warning that :http_uri.parse/1 is deprecated and will be removed in OTP 25.

@achedeuzot
Copy link
Owner

Hm indeed, it looks like a difference in OTP versions. Thanks @aparod ❤️
We'll have to look for a replacement of the :http_uri.parse function for this validator for OTP > 25.

@allenwyma
Copy link
Collaborator

What about URI.new/1 in Elixir?

@allenwyma
Copy link
Collaborator

hi @achedeuzot its' been quite some time and this issue still isn't resolved. I've also noticed you have quite a few dependabot PRs waiting to be updated.

Did you need some help with this project?

@achedeuzot
Copy link
Owner

@allenwyma I'd be happy to have a hand on it indeed, I've been quite busy lately so I've struggled to find time to maintain this even tough I still use it in production.

@allenwyma
Copy link
Collaborator

I'm more than happy to help out on this cause I'd hate to see such a useful package stalled out due to someone being busy. :)

@allenwyma
Copy link
Collaborator

@achedeuzot as a workaround, erlang uses to use :uri_string, so I was looking at :uri_string.parse/1 but these URIs are failing in the test to be marked as false:

rdar://1234
h:/test
ftps://foo.bar/

@achedeuzot
Copy link
Owner

They do look false though, don't they ? Well we can set them as truthy but it might provide a less strict validation which could have unintended consequences on end-users of the lib :/ This option would probably require a lib major version bump so users are aware of the breaking change.

What about using URI.parse/1 ?

@allenwyma
Copy link
Collaborator

URI.parse/1 will parse anything:

iex> URI.parse("foo/bar")
%URI{
  fragment: nil,
  host: nil,
  path: "foo/bar",
  port: nil,
  query: nil,
  scheme: nil,
  userinfo: nil
}

@allenwyma
Copy link
Collaborator

@achedeuzot let me know what you think of #88

I went back and looked at the code for http_uri and looks like they hard checked for certain ports if they are there or not, which is what i tried to emulate.

@achedeuzot
Copy link
Owner

#88 looks good :)

I'm just a bit unsure of using uri_string vs URI.parse, WDYT is more future proof ?

@allenwyma
Copy link
Collaborator

replaced with URI.parse/1 and seems to work fine, at least for the tests.

@achedeuzot
Copy link
Owner

Looks good with URI.parse/1 :)
It seems the CI script will need a bit of a refresher too for erlang / elixir compatibility matrix.

@allenwyma
Copy link
Collaborator

@achedeuzot no idea how this stuff works, can you take a moment to try to fix that?

@joeyates
Copy link
Contributor

Hi @achedeuzot (pinging also @allenwyma and @esacteksab)

I'd be willing to sort out the URI stuff and the CI matrix, but I think it would be a good idea to clarify a couple of things first...

URI Parsing

Firstly, uri_string is not available in all OTP versions: "Module uri_string was introduced in OTP 21.0"

Secondly, it is not possible to specify a minimum OTP version in mix.exs - it's an explicit policy.

But, URI.parse/1 has been around since well before Elixir 1.9.0. And this library declares 1.9 as it's minimal supported version.

So, I think the best way forward with URI parsing is to switch to using Elixir's URI.parse/1.

Build Matrix

The current CI configuration runs 3*4 - 5 = 7 builds.

In comparison, Ecto runs 3.

I propose reducing down to 3. I would keep the same minimum versions as now (Elixir 1.9.4 and OTP 20.3), plus the latest releases of both (currently Elixir 1.16.0 and OTP 26.2.1) plus one more in the middle (Elixir 1.13, OTP 23).

@joeyates
Copy link
Contributor

Additional note: it would be necessary to implement the failure cases of http_uri.parse/1 as URI.parse/1 accepts any String.

@achedeuzot
Copy link
Owner

Hi @joeyates 👋

I haven't had the time lately to give this lib the love it deserves. I'm happy to merge PRs that fix or improve the CI as well as the compatibility matrix and support :)

@joeyates
Copy link
Contributor

I've made a PR (#90) with the introduction of URI.new/1 to replace calls to the OTP function :http_uri.parse/1.

@achedeuzot
Copy link
Owner

All the pending pull requests have been merged and/or the corresponding issue fixed.
There's a new release: 0.3.5 🎉

Thank you all for your contributions ❤️🤗

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

No branches or pull requests

5 participants