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

HTTP::Client: check host is just a host #7958

Merged
merged 1 commit into from
Jul 11, 2019

Conversation

asterite
Copy link
Member

Fixes #7902

To be honest, I'd really like to change HTTP::Client to work like in other languages (Go, C#), like some of what is proposed in #6011

But until that happens I think it's good if we make it easier to use the existing HTTP::Client.

@bcardiff
Copy link
Member

I guess the def self.new(uri : URI, tls = nil) should also have a runtime check that the URI does not contains a path.

The protected method validate_host seems to be used in other places to ensure there is a host, but making it validate that the URI has no path will break some stuff probably.

@asterite
Copy link
Member Author

@bcardiff Yeah, I saw that too but I think that's a different issue to fix (I just wanted to fix one thing, the other one isn't reported so it might not be a problem, but we'd still want to do it). Plus maybe when you create an HTTP::Client with a path it makes sense to use that as a base path, even merging the query string. I don't know...

@asterite asterite added this to the 0.30.0 milestone Jul 11, 2019
@asterite asterite merged commit fa78362 into crystal-lang:master Jul 11, 2019
@asterite asterite deleted the http-client-check-host branch July 11, 2019 20:05
dnamsons pushed a commit to dnamsons/crystal that referenced this pull request Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Socket::Addrinfo::Error when using HTTP::Client constructor
2 participants