Generic HTTP bindings #225

matejvasek · 2020-09-01T11:41:07Z

resolves #212

http/restful-ws/src/main/java/io/cloudevents/http/restful/ws/impl/RestfulWSMessageWriter.java

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

matejvasek · 2020-09-01T11:52:36Z

relates to #212

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

slinkydeveloper

I saw you did some refactoring of restful-ws impl inside this PR, can you push it in a separate PR?

http/restful-ws/src/main/java/io/cloudevents/http/restful/ws/impl/RestfulWSMessageWriter.java

matejvasek · 2020-09-01T13:30:29Z

@slinkydeveloper Sorry for mixing up things:
Re: restful-ws changes -- commit just an example of usage it there only to demonstrate usage, it is not supposed to be merged.
BTW it also shows that generic interface is not most efficient since I had to convert Multi Map to ordinary map iterator.

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

slinkydeveloper · 2020-09-01T13:29:31Z

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

+        );
+    }
+
+    public static MessageWriter createWriter(BiConsumer<String, String> putHeader, Consumer<byte[]> putBody) {


That's a cool idea!

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

slinkydeveloper · 2020-09-01T13:38:34Z

@slinkydeveloper Sorry for mixing up things:
Re: restful-ws changes -- commit just an example of usage it there only to demonstrate usage, it is not supposed to be merged.
BTW it also shows that generic interface is not most efficient since I had to convert Multi Map to ordinary map iterator.

For demonstration purpose, can you add a snippet to the module readme? (also with the tests, the usage should be pretty clear)

eamonnmcmanus

Thanks for putting this together! It's very close to what I was imagining.

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

http/basic/src/main/java/io/cloudevents/http/HttpMessageReader.java

http/basic/src/main/java/io/cloudevents/http/HttpMessageWriter.java

http/restful-ws/src/main/java/io/cloudevents/http/restful/ws/CloudEventsProvider.java

...l-ws/src/main/java/io/cloudevents/http/restful/ws/impl/BinaryRestfulWSMessageReaderImpl.java

http/basic/src/main/java/io/cloudevents/http/HttpMessageReader.java

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

matejvasek · 2020-09-01T16:06:50Z

I added examples using java.net.HttpURLConnection and com.sun.net.httpserver.HttpServer.

matejvasek · 2020-09-01T16:08:08Z

Speaking of header: I must not forget about their case-insensitivity.

slinkydeveloper · 2020-09-01T16:55:52Z

Speaking of header: I must not forget about their case-insensitivity.

On the write side, i guess you should just take a convention and use it... Then the internal representation is up to the single http libraries

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

matejvasek · 2020-09-02T09:24:30Z

http/basic/src/test/java/io/cloudevents/http/impl/HttpMessageWriterTest.java

+        }
+    }
+
+    public static Stream<Arguments> binaryTestArguments() {


This code is already copypasted with little changes at multiple places. We should refactor that latter (not in this PR) if possible.

Maybe add a todo to the code itself?

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

matejvasek · 2020-09-03T17:09:20Z

I added README and some examples HttpURLConnection, Servlet API, com.sun.net.httpserver.HttpServer.

matejvasek · 2020-09-03T17:12:09Z

TODO: add Javadocs

matejvasek · 2020-09-03T17:39:15Z

Should I call it basic http or generic http?

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

slinkydeveloper · 2020-09-04T07:27:17Z

For me the name of the module should be cloudevents-http-basic

examples/basic-http/pom.xml

matejvasek · 2020-09-04T15:53:17Z

@eamonnmcmanus pleas give it another look.

http/basic/README.md

eamonnmcmanus

This is looking good. Thanks for putting in all that work!

examples/basic-http/src/main/java/io/cloudeventrs/examples/http/HttpURLConnectionClient.java

examples/basic-http/src/main/java/io/cloudeventrs/examples/http/IOUtils.java

examples/basic-http/src/main/java/io/cloudeventrs/examples/http/JettyServer.java

http/basic/README.md

eamonnmcmanus · 2020-09-04T16:29:01Z

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

+     * Example of usage with <a href="https://docs.oracle.com/javaee/6/api/javax/servlet/http/HttpServletRequest.html">HttpServletRequest</a>:
+     * {@code
+     * Consumer<BiConsumer<String,String>> forEachHeader = processHeader -> {
+     *     Enumeration<String> headerNames = httpServletRequest.getHeaderNames();


for (String name : Collections.list(httpServletRequest.getHeaderNames())) {

Definitely would work, but why allocate new List just to iterate over it?

Collections.list was introduced specifically to get the legacy type Enumeration out of the way as quickly as possible. For me this is the idiomatic way to iterate over the Enumeration<String> type that the ancient Servlet API imposes. It's also a lot more readable for the example code.

Just found out that there is asIterator() then I could call forEachRemaining unfortunately the asIterator() method is available since Java 9+ 😞

I agree that

for (String name : Collections.list(httpServletRequest.getHeaderNames())) {}

is probably more readable.

But I find ArrayList allocation (and possible internal reallocation on its growth) unnecessary.
I specifically chosen that obsolete type to show that it works well with Consumer<BiConsumer<String,String>> forEachHeader. @slinkydeveloper wdyt?

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

eamonnmcmanus · 2020-09-04T16:40:03Z

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

+     * @return a message reader implementation with potentially an unknown encoding
+     * @throws IllegalArgumentException If, in case of binary mode, the spec version is invalid
+     */
+    public static MessageReader createReaderFromMultiMap(Map<String, List<String>> headers, byte[] body) {


I think the spelling Multimap rather than MultiMap is more usual.

I prefer MultiMap because to me it's a reasoning like TreeMap (first part of the name tells you the impl detail, the second one the interface it implements), but it's a matter of taste

A TreeMap is just a kind of Map, but a Multimap is a different sort of thing. As words we would write "tree map" but "multimap". https://en.wikipedia.org/wiki/Multimap

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

http/basic/src/main/java/io/cloudevents/http/impl/CloudEventsHeaders.java

slinkydeveloper

I did a first pass, on monday i'll do another one 😄

examples/basic-http/src/main/java/io/cloudeventrs/examples/http/HttpServer.java

examples/basic-http/src/main/java/io/cloudeventrs/examples/http/HttpURLConnectionClient.java

slinkydeveloper · 2020-09-04T17:40:18Z

examples/basic-http/src/main/java/io/cloudeventrs/examples/http/IOUtils.java

+import java.io.IOException;
+import java.io.InputStream;
+
+// based on apache commons io


add a link to the source code

better now?

slinkydeveloper

Because the basic http example contains examples with sun http server, http url connection and jetty server, maybe you should add a basic readme that explains the three different samples here?

examples/basic-http/src/main/java/io/cloudeventrs/examples/http/HttpServer.java

slinkydeveloper · 2020-09-07T10:41:16Z

examples/basic-http/src/main/java/io/cloudeventrs/examples/http/HttpServer.java

+                    httpExchange.sendResponseHeaders(200, body.length);
+                    httpExchange.getResponseBody().write(body);
+                } catch (IOException t) {
+                    throw new UncheckedIOException(t);


waaaaat, i didn't knew it 😍

http/basic/README.md

slinkydeveloper · 2020-09-07T10:46:12Z

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

+     *     }
+     * };
+     * byte[] body;
+     * int contentLength = httpServletRequest.getContentLength();


remove this

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java

http/basic/src/test/java/io/cloudevents/http/HttpMessageReaderWriterTest.java

http/basic/README.md

matejvasek · 2020-09-07T15:57:52Z

@eamonnmcmanus please give it another look.

examples/basic-http/src/main/java/io/cloudevents/examples/http/basic/HttpServer.java

examples/basic-http/src/main/java/io/cloudevents/examples/http/basic/IOUtils.java

eamonnmcmanus · 2020-09-08T19:30:47Z

http/basic/src/test/java/io/cloudevents/http/HttpMessageReaderWriterTest.java

+            // V03
+            Arguments.of(
+                V03_MIN,
+                new HashMap<String,String>() {{


Probably OK in a test, but you could also write a helper function that takes a String[] or a String[][] and makes a Map out of it. Better still would be to use Map.of(...), but that requires Java 9+.

@eamonnmcmanus Originally I used something different: c09ef8c#diff-da38f4f0dad4b560cd509239f96a762eL241 was it better?

I didn't realize it uses anon classes. Gee couldn't they just already add proper literals to the language.

Of course this is only a test, so the kittens God is killing are only test kittens. Still, I think I preferred the way you had it before, which was at least as readable without triggering the kitten-killing fear in seasoned Java developers.

For literals, I think List.of(...) and Set.of(...), added in Java 9, are good enough for those types without needing a special syntax. There's also Map.of(...), which you could have used here except for the desire to work on Java 8. But that one is a bit dicey anyway since there's no obvious syntactic difference between keys and values, and it arbitrarily stops at 10 key-value pairs.

slinkydeveloper · 2020-09-09T16:52:02Z

@matejvasek can you fix the dco?

slinkydeveloper

last change and we're ready to go

slinkydeveloper · 2020-09-09T16:54:30Z

examples/basic-http/pom.xml

+            <artifactId>cloudevents-json-jackson</artifactId>
+            <version>${project.version}</version>
+        </dependency>
+        <dependency>


You don't need that, because it's brought in by cloudevents-json-jackson

also should be in cloudevents-http-basic

Signed-off-by: Matej Vasek <mvasek@redhat.com>

slinkydeveloper · 2020-09-09T17:29:09Z

Thanks @matejvasek for the great work with this PR and thanks to @eamonnmcmanus for the fantastic job with the reviews!

matejvasek mentioned this pull request Sep 1, 2020

Much harder with V2 than V1 to send binary CloudEvents #212

Closed

matejvasek commented Sep 1, 2020

View reviewed changes

http/restful-ws/src/main/java/io/cloudevents/http/restful/ws/impl/RestfulWSMessageWriter.java Outdated Show resolved Hide resolved

matejvasek commented Sep 1, 2020

View reviewed changes

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java Outdated Show resolved Hide resolved

matejvasek commented Sep 1, 2020

View reviewed changes

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java Outdated Show resolved Hide resolved

matejvasek commented Sep 1, 2020

View reviewed changes

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java Outdated Show resolved Hide resolved

slinkydeveloper reviewed Sep 1, 2020

View reviewed changes

http/restful-ws/src/main/java/io/cloudevents/http/restful/ws/impl/RestfulWSMessageWriter.java Outdated Show resolved Hide resolved

slinkydeveloper added the enhancement New feature or request label Sep 1, 2020

slinkydeveloper added this to the 2.0.0-milestone3 milestone Sep 1, 2020

slinkydeveloper reviewed Sep 1, 2020

View reviewed changes

eamonnmcmanus reviewed Sep 1, 2020

View reviewed changes

matejvasek commented Sep 1, 2020

View reviewed changes

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java Outdated Show resolved Hide resolved

eamonnmcmanus reviewed Sep 1, 2020

View reviewed changes

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java Outdated Show resolved Hide resolved

matejvasek commented Sep 2, 2020

View reviewed changes

matejvasek commented Sep 3, 2020

View reviewed changes

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java Outdated Show resolved Hide resolved

matejvasek commented Sep 3, 2020

View reviewed changes

http/basic/src/main/java/io/cloudevents/http/HttpMessageFactory.java Outdated Show resolved Hide resolved

slinkydeveloper reviewed Sep 4, 2020

View reviewed changes

examples/basic-http/pom.xml Outdated Show resolved Hide resolved

matejvasek requested a review from slinkydeveloper September 4, 2020 15:50

matejvasek changed the title ~~Http basic~~ Generic HTTP bindings Sep 4, 2020

matejvasek marked this pull request as ready for review September 4, 2020 15:52

matejvasek commented Sep 4, 2020

View reviewed changes

http/basic/README.md Show resolved Hide resolved

eamonnmcmanus reviewed Sep 4, 2020

View reviewed changes

slinkydeveloper reviewed Sep 4, 2020

View reviewed changes

slinkydeveloper reviewed Sep 7, 2020

View reviewed changes

matejvasek commented Sep 7, 2020

View reviewed changes

http/basic/README.md Show resolved Hide resolved

eamonnmcmanus reviewed Sep 8, 2020

View reviewed changes

examples/basic-http/src/main/java/io/cloudevents/examples/http/basic/HttpServer.java Outdated Show resolved Hide resolved

eamonnmcmanus reviewed Sep 8, 2020

View reviewed changes

examples/basic-http/src/main/java/io/cloudevents/examples/http/basic/IOUtils.java Show resolved Hide resolved

eamonnmcmanus reviewed Sep 8, 2020

View reviewed changes

slinkydeveloper reviewed Sep 9, 2020

View reviewed changes

simple http binding

4072f15

Signed-off-by: Matej Vasek <mvasek@redhat.com>

slinkydeveloper merged commit 6eecf29 into cloudevents:master Sep 9, 2020

Generic HTTP bindings #225

Generic HTTP bindings #225

Uh oh!

Conversation

matejvasek commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matejvasek commented Sep 1, 2020

Uh oh!

Uh oh!

Uh oh!

slinkydeveloper left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matejvasek commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slinkydeveloper commented Sep 1, 2020

Uh oh!

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matejvasek commented Sep 1, 2020

Uh oh!

matejvasek commented Sep 1, 2020

Uh oh!

slinkydeveloper commented Sep 1, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matejvasek commented Sep 3, 2020

Uh oh!

matejvasek commented Sep 3, 2020

Uh oh!

matejvasek commented Sep 3, 2020

Uh oh!

Uh oh!

slinkydeveloper commented Sep 4, 2020

Uh oh!

Uh oh!

matejvasek commented Sep 4, 2020

Uh oh!

Uh oh!

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matejvasek commented Sep 1, 2020 •

edited

Loading

matejvasek commented Sep 1, 2020 •

edited

Loading

slinkydeveloper Sep 4, 2020 •

edited

Loading

matejvasek Sep 8, 2020 •

edited

Loading