Code deduplication + IPv6 HTTP client fix #2346

denizzzka · 2019-08-10T07:07:43Z

Removes multiple copy-and-pasted lines from HTTP client module.
Fixes ommited support of IPv6 addresses in two of four requestHTTP fuctions
Adds module vibe.http.auth.basic_auth_client to avoid cyclic dependency of ctors/dtors between HTTP client and server code.
@safe coverage slightly increased

Space ommited

Geod24

Made a few comments.

Geod24 · 2019-08-10T11:05:09Z

http/vibe/http/client.d

+		enforce(url.schema == "http" || url.schema == "https", "URL schema must be http(s).");
+	}
+	enforce(url.host.length > 0, "URL must contain a host name.");
+	bool use_tls;


Instead of creating a variable, you can just return when it is set, because the variable is only ever assigned to once

I want to avoid changes as much as possible so as not to add any error here. It's okay if such a variable stays here for a while. And personally I do not like many exit points.

Geod24 · 2019-08-10T11:08:08Z

http/vibe/http/client.d

@@ -173,6 +134,38 @@ private bool isTlsNeed(in URL url, in HTTPClientSettings settings)
 	return use_tls;
 }

+//TODO: remove @trusted
+private void httpRequesterDg(scope HTTPClientRequest req, in URL url, in HTTPClientSettings settings, scope void delegate(scope HTTPClientRequest req) requester) @trusted


If the delegate aren't @safe, this can't be marked @trusted, ever

This is already tagged as "Outdated".

I used @trusted here temporary to not to make a mistake by making a big change right away

In that case you could add @system to httpRequesterDg instead. @trusted is bad, because it communicates that the contained code has been manually verified to be safe, which is of course not the case with a user supplied callback.

Of course the next step at some point will be to templatize the callback parameter and let the compiler infer the safety of the function (which currently requires putting it in a different scope or replacing the @safe: at the beginning of the file).

EDIT: I just saw what you mean with the wohle block being trusted before, so I'm fine with the current intermediate step!

http/vibe/http/auth/basic_auth.d

Geod24 · 2019-08-10T11:12:06Z

http/vibe/http/auth/basic_auth_client.d

+
+	License: Subject to the terms of the MIT license, as written in the included LICENSE.txt file.
+*/
+module vibe.http.auth.basic_auth_client;


Why the new module ? If it's only for one function I am not sure it's worth it.

It is need to avoid cyclic dependency of ctors/dtors between HTTP client and server modules what both use basic_auth module otherwise

Geod24 · 2019-08-10T11:12:50Z

http/vibe/http/client.d


-	if (requester) requester(req);
+	if (requester) () @trusted{ requester(req); } ();


There's no point in using @safe if we trust a random external delegate.

In fact, if look carefully, initially all of this code block was unsafe - otherwise, how would such call marked here by red color work at all? Now only one line of it marked as trusted.

In other words, we previously trusted to it too and nothing changed.

http/vibe/http/auth/basic_auth.d

denizzzka · 2019-08-11T12:20:03Z

One test failed on CI side again

s-ludwig · 2019-08-14T06:46:24Z

http/vibe/http/client.d

+	assert(!cli.m_responding, "HTTP client still responding after return!?");
+}
+
+private bool isTlsNeed(in URL url, in HTTPClientSettings settings)


Naming: isTLSNeeded or isTLSRequired

s-ludwig · 2019-08-14T06:51:40Z

http/vibe/http/client.d

@@ -173,6 +134,38 @@ private bool isTlsNeed(in URL url, in HTTPClientSettings settings)
 	return use_tls;
 }

+//TODO: remove @trusted
+private void httpRequesterDg(scope HTTPClientRequest req, in URL url, in HTTPClientSettings settings, scope void delegate(scope HTTPClientRequest req) requester) @trusted


In that case you could add @system to httpRequesterDg instead. @trusted is bad, because it communicates that the contained code has been manually verified to be safe, which is of course not the case with a user supplied callback.

Of course the next step at some point will be to templatize the callback parameter and let the compiler infer the safety of the function (which currently requires putting it in a different scope or replacing the @safe: at the beginning of the file).

EDIT: I just saw what you mean with the wohle block being trusted before, so I'm fine with the current intermediate step!

s-ludwig

About the auth_basic changes, the fact that it creates a new public mini-module is really unfortunate. A more suitable solution may be to create a new vibe.http.internal.* module that contains the implementation, which is in turn used by vibe.http.client and exposed though vibe.http.auth.auth_basic. That just also requires taking care of the ddox-filter arguments in the main dub.sdl file.

I'm not sure about the name of that internal module, since it will only contain one almost trivial function, but since it's internal, it doesn't really matter that much and auth_basic is just fine for now. The function itself could even be marked as package(vibe.http) in addition to not being documented.

denizzzka · 2019-08-14T07:38:12Z

I'm not sure about the name of that internal module, since it will only contain one almost trivial function

Maybe some another functions will be moved to it too? I just moved this one because compilation issue. (But this change can be left for the future.)

s-ludwig · 2019-08-14T07:47:47Z

Maybe some another functions will be moved to it too? I just moved one because compilation issue.
(But this change can be left for the future.)

Agreed, but we can freely change the module name later as required, so it really just shouldn't become a reason to worry about now.

denizzzka · 2019-08-17T04:46:28Z

A more suitable solution may be to create a new vibe.http.internal.*

~~This solution may lead to same cyclic dependency in the future when some another functions will be moved to it. (Because internal.* sounds like that it should contain both client and server code)~~

(I did not notice the asterisk.)

It turns out to not very look good (as for me) with the same result:vibe.http.internal.basic_auth_client. Moreover, in fact, addBasicAuth aren't internal function - this is a function that is designed to call by users.

I think it's better to leave it as it is now.

s-ludwig · 2019-08-20T11:21:49Z

I'm mostly concerned with the public API. The internal module is a little silly for the amount of code it contains, but that's fine if the alternative is to have a public module with the same issue, plus making a pointless breaking change.

denizzzka · 2019-08-20T13:10:47Z

What if I am just remove deprecated from alias addBasicAuth = vibe.http.auth.basic_auth_client.addBasicAuth;? Will documentation also be rendered for vibe.http.auth.basic_auth module? In this case no one will notice anything and nothing will need to be changed in the user code.

s-ludwig · 2019-08-20T13:25:49Z

Yes, it's still a normal public module in that case. The convention is that any *.internal.* modules will be hidden from the documentation.

denizzzka added 4 commits August 10, 2019 12:33

Code duplication eliminated: isTlsNeed added

d48cc58

Code duplication eliminated: httpRequesterDg implemented

756f664

Removed basic auth code duplication

c58ec21

Comment moved to suitable place

23deade

Space ommited

denizzzka force-pushed the code_deduplication branch from b5f4580 to 23deade Compare August 10, 2019 07:59

Geod24 requested changes Aug 10, 2019

View reviewed changes

Geod24 reviewed Aug 10, 2019

View reviewed changes

http/vibe/http/auth/basic_auth.d Outdated Show resolved Hide resolved

addBasicAuth: deprecation message added

0ff7610

denizzzka force-pushed the code_deduplication branch from 029a0ae to 0ff7610 Compare August 10, 2019 12:08

Merge branch 'master' into code_deduplication

29dc4c5

denizzzka mentioned this pull request Aug 13, 2019

Fixup HTTP client timeout #2347

Merged

s-ludwig reviewed Aug 14, 2019

View reviewed changes

s-ludwig requested changes Aug 14, 2019

View reviewed changes

Naming: isTlsNeed -> isTLSRequired

32ac213

denizzzka added 2 commits August 20, 2019 21:00

vibe.http.auth.basic_auth_client -> vibe.http.internal.basic_auth_client

f8cee45

Tests fix: outdated imports removed

0f2cb89

s-ludwig approved these changes Aug 21, 2019

View reviewed changes

s-ludwig merged commit 4bf3f58 into vibe-d:master Aug 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code deduplication + IPv6 HTTP client fix #2346

Code deduplication + IPv6 HTTP client fix #2346

denizzzka commented Aug 10, 2019 •

edited

Loading

Geod24 left a comment

Geod24 Aug 10, 2019

denizzzka Aug 10, 2019

Geod24 Aug 10, 2019

denizzzka Aug 10, 2019 •

edited

Loading

s-ludwig Aug 14, 2019 •

edited

Loading

Geod24 Aug 10, 2019

denizzzka Aug 10, 2019 •

edited

Loading

Geod24 Aug 10, 2019

denizzzka Aug 10, 2019 •

edited

Loading

denizzzka Aug 10, 2019 •

edited

Loading

denizzzka commented Aug 11, 2019

s-ludwig Aug 14, 2019

s-ludwig Aug 14, 2019 •

edited

Loading

s-ludwig left a comment

denizzzka commented Aug 14, 2019 •

edited

Loading

s-ludwig commented Aug 14, 2019

denizzzka commented Aug 17, 2019 •

edited

Loading

s-ludwig commented Aug 20, 2019

denizzzka commented Aug 20, 2019

s-ludwig commented Aug 20, 2019


		if (requester) requester(req);
		if (requester) () @trusted{ requester(req); } ();

Code deduplication + IPv6 HTTP client fix #2346

Code deduplication + IPv6 HTTP client fix #2346

Conversation

denizzzka commented Aug 10, 2019 • edited Loading

Geod24 left a comment

Choose a reason for hiding this comment

Geod24 Aug 10, 2019

Choose a reason for hiding this comment

denizzzka Aug 10, 2019

Choose a reason for hiding this comment

Geod24 Aug 10, 2019

Choose a reason for hiding this comment

denizzzka Aug 10, 2019 • edited Loading

Choose a reason for hiding this comment

s-ludwig Aug 14, 2019 • edited Loading

Choose a reason for hiding this comment

Geod24 Aug 10, 2019

Choose a reason for hiding this comment

denizzzka Aug 10, 2019 • edited Loading

Choose a reason for hiding this comment

Geod24 Aug 10, 2019

Choose a reason for hiding this comment

denizzzka Aug 10, 2019 • edited Loading

Choose a reason for hiding this comment

denizzzka Aug 10, 2019 • edited Loading

Choose a reason for hiding this comment

denizzzka commented Aug 11, 2019

s-ludwig Aug 14, 2019

Choose a reason for hiding this comment

s-ludwig Aug 14, 2019 • edited Loading

Choose a reason for hiding this comment

s-ludwig left a comment

Choose a reason for hiding this comment

denizzzka commented Aug 14, 2019 • edited Loading

s-ludwig commented Aug 14, 2019

denizzzka commented Aug 17, 2019 • edited Loading

s-ludwig commented Aug 20, 2019

denizzzka commented Aug 20, 2019

s-ludwig commented Aug 20, 2019

denizzzka commented Aug 10, 2019 •

edited

Loading

denizzzka Aug 10, 2019 •

edited

Loading

s-ludwig Aug 14, 2019 •

edited

Loading

denizzzka Aug 10, 2019 •

edited

Loading

denizzzka Aug 10, 2019 •

edited

Loading

denizzzka Aug 10, 2019 •

edited

Loading

s-ludwig Aug 14, 2019 •

edited

Loading

denizzzka commented Aug 14, 2019 •

edited

Loading

denizzzka commented Aug 17, 2019 •

edited

Loading