Generic http/httpAsync DSL #189

Hakky54 · 2020-05-15T00:27:43Z

This is a pull request to enable dynamic http request, see issue mentioned here: #169

Summary:
With the current version the end-user needs to explicitly specify which http context to use for each http method type, see here code snippet from the issue #169:

package it.skrape.core.fetcher

import io.github.rybalkinsd.kohttp.client.defaultHttpClient
import io.github.rybalkinsd.kohttp.client.fork
import io.github.rybalkinsd.kohttp.dsl.*
import io.github.rybalkinsd.kohttp.dsl.context.HttpContext
import io.github.rybalkinsd.kohttp.ext.url
import it.skrape.core.Request
import it.skrape.core.Result
import org.jsoup.Connection

class KoFetcher(private val request: Request): Fetcher {
	private val client = defaultHttpClient.fork {
		followRedirects = request.followRedirects
		readTimeout = request.timeout.toLong()
	}

	override fun fetch(): Result {
		val requester = when(request.method) {
			Connection.Method.GET -> httpGet(client, getContext())
			Connection.Method.POST -> httpPost(client, getContext())
			Connection.Method.PUT -> httpPut(client, getContext())
			Connection.Method.DELETE -> httpDelete(client, getContext())
			Connection.Method.PATCH -> httpPatch(client, getContext())
			Connection.Method.HEAD -> httpHead(client, getContext())
			else -> throw UnsupportedOperationException("Method is not supported by KoHttp")
		}
		requester.use {
			return Result(
				responseBody = it.body()?.string() ?: "",
				statusCode = it.code(),
				statusMessage = it.message(),
				contentType = it.header("Content-Type"),
				headers = it.headers().names().associateBy({item -> item}, {item -> it.header(item, "")!!}),
				request = request
			)
		}
	}

	private fun getContext(): HttpContext.() -> Unit = {
		url(request.url)
		header {
			request.headers
			"User-Agent" to request.userAgent
			cookie {
				request.cookies
			}
		}
	}
}

Provided solution
Within this pull request a http request can be executed by either specifying the http method or http context. The underlying function will determine during runtime which http context to use by the provided arguments.

The above snippet can be rewritten with the following snippet:

package it.skrape.core.fetcher

import io.github.rybalkinsd.kohttp.client.defaultHttpClient
import io.github.rybalkinsd.kohttp.client.fork
import io.github.rybalkinsd.kohttp.dsl.*
import io.github.rybalkinsd.kohttp.dsl.context.HttpContext
import io.github.rybalkinsd.kohttp.ext.url
import it.skrape.core.Request
import it.skrape.core.Result
import org.jsoup.Connection

class KoFetcher(private val request: Request): Fetcher {
	private val client = defaultHttpClient.fork {
		followRedirects = request.followRedirects
		readTimeout = request.timeout.toLong()
	}

	override fun fetch(): Result {
	    method: Method = Method.valueOf(request.method.toString)
	    val requester = http(method = method, init = getContext)

		requester.use {
			return Result(
				responseBody = it.body()?.string() ?: "",
				statusCode = it.code(),
				statusMessage = it.message(),
				contentType = it.header("Content-Type"),
				headers = it.headers().names().associateBy({item -> item}, {item -> it.header(item, "")!!}),
				request = request
			)
		}
	}

	private fun getContext(): HttpContext.() -> Unit = {
		url(request.url)
		header {
			request.headers
			"User-Agent" to request.userAgent
			cookie {
				request.cookies
			}
		}
	}
}

Note
This pull request is currently WIP to get review input on the initial solution. Some unit tests are already added to provide example usages.

…amic HttpContext or HttpMethod

Hakky54 · 2020-05-15T00:31:29Z

@rybalkinsd This feature was in my opinion quite challenging. It works but I don't think it is the most clever solution. What do you think?

I still need to add all the unit test, javadoc and do some additional clean-up.

codecov · 2020-05-15T00:33:21Z

Codecov Report

Merging #189 into master will decrease coverage by 0.21%.
The diff coverage is 84.61%.

@@             Coverage Diff              @@
##             master     #189      +/-   ##
============================================
- Coverage     91.50%   91.28%   -0.22%     
  Complexity      127      127              
============================================
  Files            41       44       +3     
  Lines           400      413      +13     
  Branches         49       52       +3     
============================================
+ Hits            366      377      +11     
  Misses            9        9              
- Partials         25       27       +2

Impacted Files	Coverage Δ	Complexity Δ
...github/rybalkinsd/kohttp/dsl/async/HttpAsyncDsl.kt	`33.33% <33.33%> (ø)`	`0.00 <0.00> (?)`
.../kotlin/io/github/rybalkinsd/kohttp/dsl/httpDsl.kt	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
...o/github/rybalkinsd/kohttp/util/HttpContextUtil.kt	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8232a1a...29fe8c1. Read the comment docs.

rybalkinsd · 2020-05-15T11:03:32Z

@Hakky54

Wow! I also expected it to be more challenging.

I like the API

http(method = ... ) {
    contextAction()
}

A couple of things to think about:

Method and Context are strongly connected. Is there a way to connect them as Types?
fun <T> http(..., method: Method<T>, init: T.() -> Unit) ...
It will probably remove a declaration duality
We recently discussed with @IVSivak about integration with converters/deserialisation [kohttp-moshi] Introduce integration #176
Both tasks could affect our high-level DSL, and it's better to have a coordinated view here. One of the options we discussed:
val r: ResultType = httpGet<ResultType> { }

@IVSivak could you please share your thoughts?

Hakky54 · 2020-05-15T11:42:09Z

Sure, I will check if it is possible to combine the method and the context with generics.

By the way I have the feeling that HttpContextUtil is a bit over complex, it is in my opinion not very readable. What do you guys think? Do you think there is a better way with plain kotlin to have something like that working?

rybalkinsd · 2020-05-18T10:36:51Z

By the way I have the feeling that HttpContextUtil is a bit over complex, it is in my opinion not very readable. What do you guys think? Do you think there is a better way with plain kotlin to have something like that working?

It looks like a bit complicated, imo. But yeah, as I mentioned, maybe it's possible to resolve this using types.

Another option to consider is to make Method a sort of context factory.

Hakky54 · 2020-05-19T10:21:26Z

I refactored a bit and the resulting code is simplified compared to the initial commit. Functionality wise it is exact the same as the previous commit, the only difference is that with this commit a http method is required when calling the http function.

The flows will be:

For Get request

- http function can be called with Method.GET and HttpContext
- http function can be called with Method.GET and HttpGetContext
- http function will throw class cast exception when calling with Method.GET and HttpPostContext

For Post request

- http function can be called with Method.POST and HttpContext
- http function can be called with Method.POST and HttpPostContext
- http function will throw class cast exception when calling with Method.POST and HttpGetContext

For Delete request

- http function can be called with Method.DELETE and HttpContext
- http function can be called with Method.DELETE and HttpDeleteContext
- http function will throw class cast exception when calling with Method.DELETE and HttpGetContext

etc...

I also wanted to check the type of init: T.() -> Unit within the when statement, but couldn't find a good way. So what I wanted to do is for example is to create only a HttpGetContext when the method is GET and the init variable is instance of HttpGetContext.() -> Unit or is instance of HttpContext.() -> Unit or else throw a specific runtime exception as it does not make sense to pass a HttpPostContext when creating a GET request. Another option could be that it is still allowed to pass a HttpPostContext when creating a GET request but behind the scenes we create a new HttpContext instance from the HttpPostContext and supply it to the HttpGetContext. In that way it will be save to pass any kind of sub class of HttpContext to the http function.

What do you think of the code change and of the flows? Is it ok to throw a class cast exception when someone is being funny by passing a HttpPostContext while executing a Get request?

rybalkinsd · 2020-05-19T11:54:38Z

I like the simplified version!

I was thinking about when it's possible to use exactly same request with any request method.
And it looks like strict typing is probably the thing we're looking for

fun http(client: Call.Factory = defaultHttpClient, method: Method, init: HttpContext.() -> Unit): Response {
    return client.newCall(method.makeContext().apply(init).makeRequest()).execute()
}

It will only allow to use the context initialisers applicable to any context.
It's possible to make an extension function makeContext based on createHttpContext
It could look like

internal fun Method.makeContext(): HttpContext {
    return when (this) {
        Method.GET -> HttpGetContext()
        Method.POST -> HttpPostContext()
        ...
    }
}

Another opportunity here is to support methods with body (inherited from HttpPostContext)

Hakky54 · 2020-05-19T13:09:47Z

It looks like I need to learn a lot about Kotlin, it is quite different compared to Java. I didn't know that an existing enum could be enriched with a function outside the enum declaration. Awesome!

I applied your feedback, but when I remove the generic type <T : HttpContext> the test class fails to compile. It doesn't accept that I provide HttpPostContext to a function which accepts HttpContext.() -> Unit In my opinion it doesn't make sense because I am sending an instance of a child class of a HttpContext and the function parameter should be saying that it should accept HttpContext and all child classes of HttpContext. Or do you think I am missing something?
I committed the changes up till now.

Your other option regarding supporting method with body by inheritance from HttpPostContext gives me the idea that it is the best option to move from here on, but looking at OOP aren't we misusing HttpPostContext? What do you think of moving the body property to HttpContext (the base class) and disable it at the child classes which don't have a body, such as the HttpGetContext.

…d GET request

Hakky54 · 2020-05-19T14:05:27Z

I Added both cases mentioned within my precious comment as two separate commits:

Cases:

Extending Method with a function: f383088
Move body to base class and disable it for GET and HEAD requests: 92cd43a

As a mentioned earlier, I couldn't remove the generic type within the http function for case 1 as it fail to compile wen passing a sub class of HttpContext. For example if I want to make a http post request with a body. I am forced to use the sub class HttpPostContext because the body isn't available within the base class. But without the generics I am not able to pass HttpPostContext.

I am curious what you think of the both cases.

rybalkinsd · 2020-05-19T18:52:58Z

@Hakky54

Move body to base class and disable it for GET and HEAD requests: 92cd43a

We tried it once in a while. Users are getting frustrated :) So I would try to avoid it.

Extending Method with a function: f383088

It looks good for me!
Just want to make sure what is the reason of T in

internal fun <T : HttpContext> Method.createHttpContext(): T

the test class fails to compile
Maybe I'm missing something, but
I also expected test class not to compile and It's valid imo

    http(method = Method.DELETE, init = fun HttpContext.() {
        host = "postman-echo.com"
        path = "/delete"
        // body should not compile
        body { }
    }

Imagine our use case: we have an identical request and we want time to time use POST or GET to for it.

I would go with a code like

    val ctx = {
        host = "..."
        port = "..."
        param { }
    }

   http(method = variableMethod, ctx)

In this case the only possible ctx type that will make sense for both cases is HttpContext.() -> Unit

kohttp-test/src/test/kotlin/io/github/rybalkinsd/kohttp/dsl/HttpDslKtTest.kt

… HEAD and GET request" This reverts commit 92cd43a

Hakky54 · 2020-05-20T13:17:45Z

Thanks again for the feedback!

Just want to make sure what is the reason of T in

internal fun Method.createHttpContext(): T

The reason was to support child classes of HttpContext such as HttPostContext and HttpGetContext. I removed the generic type T as you already mentioned that it is fine for the initial version to only support the base class HttpContext. I dropped case number 2 and refactored it. I have tried to finalize it. Added unit tests and updated the documentation. Let me know if you are ok with the changes.

By the way I also added unit tests for all the async http requests, but somehow CodeCov isn't recognises it

rybalkinsd

Looks amazing! And it's a huge amount of work done!
I's supper happy with this pull request, there is just a couple of things to clean before merging.

Please have a look at inline comments
I see new /dsl/async/ test, that's great we'll have them, but let's move them to another Pull Request to simplify history.
Let's also move typo/grammar/import fixes(like ClientBuilder.kt or HttpPutDsl.kt) in one another PR.

I'll be happy to merge all 3 (or more if you would go for) Pull Request as soon as possible!

rybalkinsd · 2020-05-20T16:09:26Z

kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/dsl/async/HttpAsyncDsl.kt

+ * @see HttpContext
+ * @see Method
+ *
+ * @since 0.11.2


Let's put 0.12.0 for all new API features!

kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/dsl/async/HttpAsyncDsl.kt

kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/dsl/context/HttpContext.kt

kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/dsl/httpDsl.kt

rybalkinsd · 2020-05-20T16:11:24Z

kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/dsl/httpDsl.kt

+ * @since 0.11.2
+ * @author hakky54
+ */
+fun http(client: Call.Factory = defaultHttpClient, method: Method, init: HttpContext.() -> Unit): Response {


Let's use a new line for each parameter, as in httpAsync

kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/util/HttpContextUtil.kt

rybalkinsd · 2020-05-20T16:12:52Z

kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/util/HttpContextUtil.kt

+
+internal fun Method.makeHttpContext(): HttpContext {
+    return when (this) {
+        Method.GET -> HttpGetContext()


Let's import Method to use them like Post -> HttpPostContext()

rybalkinsd · 2020-05-20T16:20:26Z

By the way I also added unit tests for all the async http requests, but somehow CodeCov isn't recognises it

No worries at all, I'll check codecov later this week

…ntextUtil.kt Co-authored-by: Sergei Rybalkin <yan.brikl@gmail.com>

Co-authored-by: Sergei Rybalkin <yan.brikl@gmail.com>

rybalkinsd · 2020-05-21T10:25:44Z

I created #190 to follow import optimisations, so the impact will be wider!
@Hakky54

…/HttpContext.kt Co-authored-by: Sergei Rybalkin <yan.brikl@gmail.com>

…ttpAsyncDsl.kt Co-authored-by: Sergei Rybalkin <yan.brikl@gmail.com>

Hakky54 · 2020-05-21T10:49:26Z

It looks like you are enjoying all these changes 😄
Sure I will extract the additional unit test and some code cleanup to a separate branch and scope down this branch to get more focus on the actual feature.

Hakky54 · 2020-05-21T10:49:43Z

Any idea when you will be releasing by the way?

rybalkinsd · 2020-05-21T10:51:51Z

Any idea when you will be releasing by the way?

Want to put 0.12.0 this Saturday

rybalkinsd

Hakky54 · 2020-05-21T11:21:26Z

Hehehe, firing all the commits to make this baby ready to ship. It think I applied all your remarks. I will also push the other branches

Hakky54 added 2 commits May 15, 2020 02:09

Removed unused imports

9ee332d

Added initial support for executing http request based on generic/dyn…

a6ce76a

…amic HttpContext or HttpMethod

Removed suppression

48bf49b

Hakky54 changed the title ~~Feature/support for generic http request~~ Add support for generic http request May 15, 2020

rybalkinsd changed the title ~~Add support for generic http request~~ WIP: Add support for generic http request May 15, 2020

rybalkinsd self-assigned this May 15, 2020

rybalkinsd requested a review from IVSivak May 15, 2020 11:03

Simplified handling dynamic http request

f8e3f62

Applied feedback

f383088

Moved body field to base class and added body validations for HEAD an…

92cd43a

…d GET request

rybalkinsd reviewed May 19, 2020

View reviewed changes

kohttp-test/src/test/kotlin/io/github/rybalkinsd/kohttp/dsl/HttpDslKtTest.kt Outdated Show resolved Hide resolved

Hakky54 added 8 commits May 19, 2020 23:43

Revert "Moved body field to base class and added body validations for…

dc97f5f

… HEAD and GET request" This reverts commit 92cd43a

Added unit tests

18260a5

Added additional unit tests for async requests

0a7ce06

Added javadoc

88c75ea

Fixed typo and reformatted

02f6d06

Added documentation

054205a

Renamed HttpMethods

87dc4e6

Refactored a bit more

73ac1ce

Hakky54 added 2 commits May 20, 2020 15:20

Refactored a bit more

bb08a65

Added some authors to the unit tests

0afa9ec

rybalkinsd requested changes May 20, 2020

View reviewed changes

Hakky54 and others added 2 commits May 20, 2020 19:46

Update kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/util/HttpCo…

7e0fc13

…ntextUtil.kt Co-authored-by: Sergei Rybalkin <yan.brikl@gmail.com>

Update kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/dsl/httpDsl.kt

63c805a

Co-authored-by: Sergei Rybalkin <yan.brikl@gmail.com>

rybalkinsd changed the title ~~WIP: Add support for generic http request~~ Generic http/httpAsync DSL May 20, 2020

rybalkinsd added the enhancement New feature or request label May 20, 2020

rybalkinsd added this to the 0.12.0 milestone May 20, 2020

Hakky54 and others added 4 commits May 21, 2020 12:28

Update kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/dsl/context…

e0e72a5

…/HttpContext.kt Co-authored-by: Sergei Rybalkin <yan.brikl@gmail.com>

Update kohttp/src/main/kotlin/io/github/rybalkinsd/kohttp/dsl/async/H…

24249ae

…ttpAsyncDsl.kt Co-authored-by: Sergei Rybalkin <yan.brikl@gmail.com>

Applied feedback

2323d80

Moved additional tests to a separate branch

ff4a015

Hakky54 added 4 commits May 21, 2020 13:08

Reverted cleanup of unused import statements

4b6daf6

Reverted typos and moved to a separate branch

9aadf5e

Reverted new lines

2f5d6bd

Reverted new lines

29fe8c1

rybalkinsd approved these changes May 21, 2020

View reviewed changes

rybalkinsd removed the request for review from IVSivak May 21, 2020 11:18

rybalkinsd merged commit 2cf3700 into rybalkinsd:master May 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic http/httpAsync DSL #189

Generic http/httpAsync DSL #189

Hakky54 commented May 15, 2020 •

edited

Hakky54 commented May 15, 2020 •

edited

codecov bot commented May 15, 2020 •

edited

rybalkinsd commented May 15, 2020 •

edited

Hakky54 commented May 15, 2020

rybalkinsd commented May 18, 2020

Hakky54 commented May 19, 2020 •

edited

rybalkinsd commented May 19, 2020

Hakky54 commented May 19, 2020

Hakky54 commented May 19, 2020 •

edited

rybalkinsd commented May 19, 2020 •

edited

Hakky54 commented May 20, 2020 •

edited

rybalkinsd left a comment

rybalkinsd May 20, 2020

rybalkinsd May 20, 2020

rybalkinsd May 20, 2020

rybalkinsd commented May 20, 2020

rybalkinsd commented May 21, 2020

Hakky54 commented May 21, 2020

Hakky54 commented May 21, 2020

rybalkinsd commented May 21, 2020

rybalkinsd left a comment

Hakky54 commented May 21, 2020

Generic http/httpAsync DSL #189

Generic http/httpAsync DSL #189

Conversation

Hakky54 commented May 15, 2020 • edited

Hakky54 commented May 15, 2020 • edited

codecov bot commented May 15, 2020 • edited

Codecov Report

rybalkinsd commented May 15, 2020 • edited

Hakky54 commented May 15, 2020

rybalkinsd commented May 18, 2020

Hakky54 commented May 19, 2020 • edited

rybalkinsd commented May 19, 2020

Hakky54 commented May 19, 2020

Hakky54 commented May 19, 2020 • edited

rybalkinsd commented May 19, 2020 • edited

Hakky54 commented May 20, 2020 • edited

rybalkinsd left a comment

Choose a reason for hiding this comment

rybalkinsd May 20, 2020

Choose a reason for hiding this comment

rybalkinsd May 20, 2020

Choose a reason for hiding this comment

rybalkinsd May 20, 2020

Choose a reason for hiding this comment

rybalkinsd commented May 20, 2020

rybalkinsd commented May 21, 2020

Hakky54 commented May 21, 2020

Hakky54 commented May 21, 2020

rybalkinsd commented May 21, 2020

rybalkinsd left a comment

Choose a reason for hiding this comment

Hakky54 commented May 21, 2020

Hakky54 commented May 15, 2020 •

edited

Hakky54 commented May 15, 2020 •

edited

codecov bot commented May 15, 2020 •

edited

rybalkinsd commented May 15, 2020 •

edited

Hakky54 commented May 19, 2020 •

edited

Hakky54 commented May 19, 2020 •

edited

rybalkinsd commented May 19, 2020 •

edited

Hakky54 commented May 20, 2020 •

edited