Approvals: 0/1
[14:21:45] <AlexLehm> temporalfox: I posted a pr yesterday where i noticed a bug in it immediately after I posted it. I have fixed that now, I will think about that a bit further (since i will be offline anyway for the afternoon)
[14:22:03] <AlexLehm> the weather is way too good to spend the afternoon programming
[14:22:04] <temporalfox> AlexLehm in which repo ?
[14:22:11] <AlexLehm> vertx-mail-clinet
[14:22:17] <temporalfox> ok
[14:22:22] <temporalfox> so you're coming to F2F ?
[14:22:57] <AlexLehm> yes, i have registered the two days as vacation time, everything is checked
[14:23:01] <temporalfox> great
[14:23:08] <temporalfox> it's really cool you come
[14:23:28] <AlexLehm> yes, i think so too
[14:23:45] <AlexLehm> (not that I think it is cool i am coming, its cool that we can meet in general)
[14:38:11] <temporalfox> AlexLehm I think we should have more documentation about proxy support for HttpClient
[14:38:24] <temporalfox> document better what is possible and what is not
[14:38:43] <temporalfox> given taht NetClient and HttpClient don't exactly work the same
[14:38:49] <temporalfox> specially with TLS
[19:46:11] <BadApe> I am writing some integration tests, the strange thing is i don't seem to be able to make multiple calls from an http client, i am guessing i need to do something more after calling end()
[21:10:02] <AlexLehm> temporalfox: i think the operations now all work with proxy
[21:10:15] <AlexLehm> there should be no difference between the clients
[21:10:40] <temporalfox> but I think we said with non https we don't support connect for http client
[21:10:40] <temporalfox> don't we ?
[21:11:14] <temporalfox> ah
[21:11:59] <AlexLehm> yes, but the http client does a “normal” connection to the proxy, which isn't done in netclient, since it doesn't http
[21:13:23] <AlexLehm> I am almost sure that we have convered all use-cases necessary
[21:17:10] <temporalfox> but the issue we had recently in the vertx google group
[21:17:26] <temporalfox> wasn't about someone that wanted to use http with connect ?
[21:17:42] <temporalfox> I always mix-up these …
[21:18:21] <temporalfox> for httpclient and connect what are the rules ? can you remind me these ?
[21:18:25] <AlexLehm> yes, we fixed that in the 3.3.3 version
[21:19:20] <AlexLehm> httpclient uses connect when calling a https server by requesting the host/port address like CONNECT www.google.com:443
[21:20:18] <temporalfox> but the proxy server is called in clear ?
[21:20:26] <temporalfox> or it does not matter ?
[21:20:38] <AlexLehm> for http servers, a request for GET / HTTP/1.0 for www.google.de is translated to GET http://www.google.de/ HTTP/1.0 to the proxy
[21:21:13] <AlexLehm> the CONNECT request to the proxy is sent in clear but the ssl connection is then established to the origin server with ssl so that this doesn't matter
[21:21:15] <aesteve> evening everyone
[21:21:23] <temporalfox> hi arnayd
[21:21:24] <temporalfox> arnaud
[21:21:34] <temporalfox> AlexLehm ok
[21:21:40] <AlexLehm> that is the common use, https connections to the proxy are not used
[21:21:50] <temporalfox> so how it works depends on the HttpClient ssl=true|false configuration
[21:21:58] <aesteve> I made two PRs, not sure if this is interesting for other users too, but that's a pattern I'm facing quite often
[21:22:00] <temporalfox> ssl=true + proxy=http ⇒ CONNECT
[21:22:06] <AlexLehm> yes
[21:22:15] <temporalfox> ssl=false + proxy=http ⇒ rewrite requestURI
[21:22:22] <AlexLehm> es
[21:22:23] <AlexLehm> yes
[21:22:40] <temporalfox> I think AlexLehm it's not clear in documentation
[21:22:42] <temporalfox> do we document that properly ?
[21:23:27] <AlexLehm> sure, i will take a look
[21:23:32] <temporalfox> thanks
[21:23:48] <temporalfox> @aesteve in the vertx core PR , I've seen a put with Map.Entry<String, T>
[21:23:57] <temporalfox> I believe you should remove T and use “?” instead
[21:24:12] <temporalfox> that would be correct way to do in java
[21:24:21] <temporalfox> I can comment in the PR
[21:24:46] <aesteve> I'll correct it
[21:24:58] <aesteve> I'm always abit confused when it comes to generics
[21:25:18] <temporalfox> aesteve everyone is!
[21:26:21] <aesteve> I recently created a thin layer of the JPA Criteria API it drove me crazy with generics…. public <T, R extends Collection<? super V>, V> …
[21:26:46] <temporalfox> I hate super/extends
[21:26:51] <AlexLehm> its mentioned in the section about http client. but we can probably improve that http://vertx.io/docs/vertx-core/java/#_using_a_proxy_for_http_https_connections
[21:26:54] <temporalfox> we don't support them in codegen
[21:26:58] <temporalfox> and I think it's a good thing
[21:27:45] <temporalfox> AlexLehm I think that we should detail each case in a subsections
[21:27:49] <temporalfox> all this mix-up to me
[21:27:54] <temporalfox> with an example
[21:28:09] <aesteve> mmh actually temporalfox I broke my own stuff
[21:28:16] <temporalfox> like
[21:28:24] <temporalfox> client.get(…)
[21:28:25] <aesteve> don't merge it, it doesn't compile anylonger
[21:28:28] <temporalfox> commented as
[21:29:00] <temporalfox> client.get(443, “www.google.com”, “/”, …);
[21:29:03] <temporalfox> commented as
[21:29:26] <temporalfox> “ make a connect request on the proxy to establish a tunnel to https://www.google.com:443”
[21:29:33] <temporalfox> so it is clear
[21:29:40] <temporalfox> etc…
[21:29:45] <temporalfox> and we make 4 sections
[21:29:50] <temporalfox> I mean one for each sock
[21:30:10] <temporalfox> one with connect proxy and one with rewrite proxy
[21:30:38] <temporalfox> repetition and consistency I think is a good thing in documentation
[21:30:44] <temporalfox> in such case
[21:31:30] <aesteve> ok fixed
[21:31:35] <aesteve> hope this is working now
[21:31:51] <aesteve> it's weird though : assertEquals(json.getInteger(1), (Integer)3);
[21:32:02] <temporalfox> that's the wonder of java boxing
[21:37:53] <aesteve> I was wondering about the name of the method
[21:38:06] <aesteve> collector() in both classes can clash with static imports
[21:38:18] <aesteve> + isn't java “standard” but vert.x standard
[21:38:40] <temporalfox> what do you mean ?
[21:38:53] <temporalfox> how about instead having a constructor with a stream in JsonObject and JsonArray ?
[21:38:59] <aesteve> it's not getXXX but since it's a static method I think it shouldn't anyway
[21:39:25] <temporalfox> maybe we can look into other existing libraries to see how they name collectors
[21:39:46] <aesteve> yes maybe
[21:40:06] <aesteve> and about the Stream constructor let me check, I guess JsonObject has one
[21:41:59] <aesteve> no, just JsonObject.stream() and Json.asStream(iterable)
[21:42:42] <aesteve> would it be better with a stream constructor or the collector ? wdyt ?
[21:43:36] <aesteve> new JsonObject(list.stream().filter(…).map(…)); or list.stream().filter(…).map(…).collect(JsonObject.collector());
[21:44:04] <aesteve> or maybe both
[21:44:23] <temporalfox> using collect(..) seems more like the usual pattern I think
[21:44:53] <temporalfox> so we should rather go with that
[21:46:20] <aesteve> oh
[21:46:41] <aesteve> I named it entryCollector()
[21:46:45] <aesteve> forgot about that
[21:46:53] <aesteve> so no clash within static imports
[21:48:24] <temporalfox> why ?
[21:48:42] <temporalfox> ah it collect Map.Entry streams ?
[21:50:08] <aesteve> yes
[21:50:26] <aesteve> best would have been Tuple<String, ?>
[21:50:38] <aesteve> actually, I was wondering
[21:50:43] <temporalfox> not for java
[21:51:02] <aesteve> instead of doing a collector, what about implementing Collector directly ?
[21:51:14] <temporalfox> in ?
[21:51:18] <aesteve> list.stream().filter(…).map(…).collect(new JsonObject());
[21:51:29] <aesteve> not sure it makes sense
[21:51:46] <temporalfox> hard to tell
[21:52:12] <temporalfox> there must be something wrong with that no ?
[21:52:12] <aesteve> mmh it's absolutely better !!
[21:52:17] <aesteve> check
[21:52:27] <temporalfox> because it measn that the same object could be used several times
[21:52:41] <temporalfox> i.e it couples the collector and the object itself
[21:52:51] <aesteve> JsonObject myJson = new JsonObject().put(“something”, a); list.stream().filter().map().collect(myJson);
[21:53:19] <aesteve> you can compose an existing jsonobject with the result of filtering a list
[21:53:31] <temporalfox> we could also have
[21:53:40] <temporalfox> collector(jsonObject)
[21:53:42] <temporalfox> with an existing json object
[21:53:54] <aesteve> yes why not
[21:54:16] <temporalfox> I think just collector is fine
[21:54:28] <temporalfox> but I'm not a fan of static import
[21:54:35] <temporalfox> except for junit
[21:54:38] <temporalfox> Assert.*
[21:54:59] <aesteve> I'm using it a lot with HttpMethods.* and HttpHeaders.*
[21:55:24] <aesteve> (GET, POST, PUT, DELETE) is so much better than (HttpMethod.GET, HttpMethod.POST, …)
[21:55:43] <aesteve> so we're going with collector()
[21:55:48] <aesteve> and collector(JsonObject)
[21:56:06] <aesteve> ?
[21:56:38] <aesteve> (actually I can't see the problem of having it implement the interface directly) but I'm far from being used to collectors
[21:57:25] <temporalfox> aesteve can you have a look at the HttpClientBuilder thread on vertx-dev ?
[21:57:35] <aesteve> sure
[21:57:37] <temporalfox> there are corresponding branches in a few projects
[21:57:51] <temporalfox> it's a usability feature for http client
[21:57:58] <temporalfox> that will benefit to rxjava
[21:58:40] <aesteve> never used RxJava yet
[21:58:49] <aesteve> but it'll benefit for Java users too ?
[21:59:06] <temporalfox> yes
[22:01:09] <aesteve> https://github.com/eclipse/vert.x/blob/http-client-request-builder/src/test/java/io/vertx/test/core/HttpClientRequestBuilderTest.java#L61
[22:01:15] <aesteve> createPost, I guess
[22:02:27] <temporalfox> yes
[22:02:34] <temporalfox> but now it's really something not polished
[22:03:08] <aesteve> I have to remember the traditional way of issuing a request
[22:03:32] <aesteve> request.end(Buffer), right ?
[22:03:45] <temporalfox> yes
[22:03:51] <aesteve> the main change is that you can add the handler when you issue the request ?
[22:03:57] <temporalfox> the idea here is to give a ReadStream<Buffer>
[22:04:10] <temporalfox> and also to copy the builder
[22:04:21] <temporalfox> so you can reuse builders several times
[22:04:32] <temporalfox> so
[22:04:33] <temporalfox> client.createGet(DEFAULT_HTTP_PORT, DEFAULT_HTTP_HOST, “/somepath”);
[22:04:34] <temporalfox> post.putHeader(“Content-Length”, “” + expected.length())
[22:04:34] <aesteve> that's a very nice idea, it'll help for dealing with proxies
[22:04:40] <temporalfox> can be reused multiples times
[22:04:51] <temporalfox> with different ReadStream<Buffer>
[22:04:57] <temporalfox> and also other idea yes is that
[22:04:58] <temporalfox> you use
[22:05:04] <aesteve> OK I get it
[22:05:12] <temporalfox> request(Handler<AsyncResult<HttpClientResponse»)
[22:05:24] <temporalfox> so you don't need to set exceptionHandler on HttpClientRequest
[22:05:32] <temporalfox> and only manage a single error in the async result
[22:06:02] <temporalfox> so simplyfing
[22:06:11] <aesteve> oh it's an asyncresult
[22:06:14] <temporalfox> yes
[22:06:20] <aesteve> that'll be a pain in some cases though
[22:06:25] <aesteve> can't we have both ?
[22:06:29] <temporalfox> both ?
[22:06:38] <aesteve> oh no obviously
[22:06:46] <aesteve> the type is Handler<…>
[22:06:52] <temporalfox> if you don't use AsyncResult then you ignore failure
[22:07:18] <aesteve> In fact I'm thinking about my use cases
[22:07:32] <aesteve> I'm using clientRequests for 2 things : proxies and tests
[22:08:03] <aesteve> for testing, idc about the exception, that'll crash the test, exactly what I need
[22:08:36] <aesteve> for proxies actually, having the readstream and the re-usable stuff is a bless
[22:09:16] <temporalfox> yes for proxies it make simple indeed
[22:09:33] <temporalfox> inside it takes care of setting up the pump
[22:09:42] <temporalfox> and stopping if if there is an error
[22:10:19] <aesteve> if you want a veryconcrete use case, have a look at how : https://github.com/aesteve/vertx-proxy/blob/master/src/main/java/io/vertx/examples/proxy/handler/ProxyHandler.java#L52-L88 would be simplified
[22:10:33] <aesteve> pure awesomeness
[22:10:51] <aesteve> just create a builder in the constructor and use it for every incomingRequest
[22:11:45] <temporalfox> indeed very similar the same code I have in the HttpClientBuilder
[22:11:54] <aesteve> for sure
[22:12:04] <temporalfox> now I'm working on HttpClientRequest.reset()
[22:12:24] <temporalfox> to abort the request if something goes wrong in the ReadStream
[22:12:33] <temporalfox> it works for Http2 as it's an HTTP2 feature
[22:12:41] <temporalfox> I'm trying to get something similar for HTTP1
[22:12:47] <temporalfox> i.e close gracefully the connection
[22:12:54] <temporalfox> specially if it's pipelined
[22:13:49] <aesteve> sounds difficult
[22:14:08] <aesteve> mmh the first commit I made was with the wrong git account
[22:14:43] <temporalfox> with pipelined it shall try to wait until all previous responses have been received and then close the connection
[22:14:54] <temporalfox> and not put it back in the pool
[22:15:58] <aesteve> mmh and I can't squash because there's a merge commit in between
[22:16:52] <temporalfox> redo a new PR with cherry-pick
[22:19:51] <aesteve> yes I'm goind to do that
[22:32:27] <aesteve> mmh actually, the third possible implementation for JsonObject
[22:32:33] <aesteve> is to create a class…
[22:32:44] <aesteve> the most obvious solution I didn't even think about
[22:32:52] <aesteve> new JsonObjectCollector()
[22:33:12] <aesteve> or new JsonObjectCollector(existingJsonObject)
[22:43:24] <aesteve> temporalfox: http://stackoverflow.com/questions/22753755/how-to-add-elements-of-a-java8-stream-into-an-existing-list
[22:43:33] <aesteve> I think it's a matter of parallelism
[22:44:26] <aesteve> (the same question I was asking about JsonObject is accurate for Collection<?> too, why can't we collect() into an existing Collection)
[22:47:09] <temporalfox> “The short answer is no, at least, not in general, you shouldn't use a Collector to modify an existing collection.”
[22:47:11] <temporalfox> you mean that ?
[22:47:48] <temporalfox> looks like it's quite over-engineered though
[22:49:29] <temporalfox> interesting read though
[22:49:54] <temporalfox> aesteve I would rather prefer the static collector()
[22:50:03] <temporalfox> as it does not introduce a new visible type to the user
[22:50:17] <aesteve> ok
[22:50:33] <temporalfox> also one possible thing is to create a new JsonObject with a collected Map ?
[22:50:45] <temporalfox> although I find that collect map is quite difficult
[22:50:54] <temporalfox> collect(Collectors.toMap)
[22:51:00] <temporalfox> because it's generic ?
[22:51:03] <temporalfox> I mean not for entries
[22:52:02] <temporalfox> for list cone could do
[22:52:24] <temporalfox> new JsonArray(stream….collect(Collectors.toList()));
[22:53:23] <temporalfox> my question is : is it really important to provide a collector over just having a constructor that takes a stream ?
[22:56:17] <aesteve> i don't know :\
[22:57:35] <aesteve> quite often, I hate instantiating an object just for the sake of fitting a method signature
[22:57:50] <temporalfox> ok
[22:58:10] <aesteve> I'm telling myself : why do I have to create a map, when a JsonObject already knows how to deal with map entries
[22:58:46] <aesteve> but for the collector(existingJson) I'm not really sure
[22:59:07] <aesteve> because it's gonna both mutate the original object and return it, maybe it's confusing
[23:07:54] <temporalfox> aesteve stackoverload say that collector(existing) is bad
[23:08:22] <aesteve> ok
[23:09:01] <temporalfox> because of collectors multithreaded
[23:09:48] <aesteve> yes that's what I thought
[23:09:51] <aesteve> I removed it
[23:12:10] <temporalfox> ok
[23:12:43] <aesteve> https://github.com/eclipse/vert.x/pull/1645
[23:12:49] <aesteve> that should be OK now
[23:13:06] <aesteve> correct username for the CLA, correct method names, correct tests
[23:14:11] <aesteve> as for the PR re. vertx-web I'm not sure this will be useful, but I've already implemented it in many projects and what decided me was this article : https://dzone.com/articles/new-in-spring-5-functional-web-framework?edition=216172&utm_source=Daily%20Digest&utm_medium=email&utm_campaign=dd%202016-09-25
[23:15:06] <aesteve> the “then” style is sometimes useful
[23:15:49] <aesteve> router.route(“/api/1”).handler(CorsHandler.create(“*”)).then(BodyHandler.create()).then(this::setContentType)…
[23:16:04] <aesteve> maybe “and” is a better keyword but since handlers are chained…
[23:18:39] <temporalfox> what does the then do ?
[23:19:00] <aesteve> just create another route with the same path, same http methods
[23:19:30] <aesteve> that's something quite surprising when you first deal with vertx-web
[23:19:56] <aesteve> you can't do : Router.route(…).handler(1stHandler).handler(2ndHandler).handler(3rdHandler)
[23:20:19] <aesteve> you have to repeat the router.route(…) part
[23:20:31] <aesteve> (everytime you attach a handler)
[23:21:00] <aesteve> So I'm giving this solution a try, but maybe that's not the good one
[23:21:34] <aesteve> let's see what Paulo says
[23:21:45] <temporalfox> yep
[23:24:56] <aesteve> you could also imagine writing something like : router.routes(“/api/v1/*”, POST, PUT).handler(BodyHandler.create()).onlyFor(POST, this::addContentType)
[23:25:10] <aesteve> when you think about it that's abit like your httpclientbuilder
[23:25:26] <aesteve> I've already written a RouterBuilder but Groovy-DSL oriented
[23:25:35] <aesteve> maybe that's the idea, a RouterBuidler