kotlin: add library response interfaces by buildbreaker · Pull Request #260 · envoyproxy/envoy-mobile

buildbreaker · 2019-07-15T19:25:59Z

#119
Adding a primitive Request and RequestBuilder with basic features:

body
headers
trailers

Kotlin usages:

val result: Result = ...

val body = result.fold(
	{ response -> 
		response.body 
	}, 
	{ failure -> 
		throw Exception(failure.cause) 
	})

// Default to a value
val value = result.fold(
	{ response -> 
		1 
	}, 
	{ failure -> 
		0 
	})

// Side effect response
result.response({ response ->
	val body = response.body
	// do something
})

// Side effect failure
result.failure({ failure ->
	val message = failure.message
	// do something
})

Java usages:

byte[] body = result.fold(response -> {
		return response.body;
	},
	failure -> {
		throw new Exception(failure.cause)
	});

// Default to a value
byte[] body = result.fold(response -> {
		return 1;
	},
	failure -> {
		return 0;
	});

// Side effect response
result.response(response -> {
		byte[] body = response.body;
		// do something
		return null;
	});

// Side effect failure
result.failure(failure -> {
		String message = failure.message
		// do something
		return null;
	})

Tests are basic setting a builder and ensuring the result is expected. Additionally, the response builder transform creates an equivalent object when built back. These are basically testing the exact same behavior as the requests

Signed-off-by: Alan Chiu achiu@lyft.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Add response objects
Risk Level: low
Testing: unit
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Alan Chiu <achiu@lyft.com>

Adds Swift interfaces for responses within Envoy. These should be at parity with the interfaces being added for Kotlin in #260. Resolves #118. Doc for reference: https://docs.google.com/document/d/1N0ZFJktK8m01uqqgfDRVB9mpC1iEn9dqkQaa_yMn_kE/edit#heading=h.i6ky65xaa9va Signed-off-by: Michael Rebello <mrebello@lyft.com>

rebello95

Looks good overall! Docstrings need updating, and can we also add the Result/Error types being added in #261?

library/kotlin/src/io/envoyproxy/envoymobile/ResponseBuilder.kt

Signed-off-by: Alan Chiu <achiu@lyft.com>

goaway · 2019-07-16T22:55:39Z

This echoes my question on the Request interfaces, but have we thought at all about how might expose response data for streamed processing?

This is standard in most HTTP library interfaces, and generally processing/handling of responses tends to be something that can make progress in an online fashion faster than bytes come over the network (i.e. you shouldn't wait to start processing until all bytes have arrived).

goaway · 2019-07-16T22:59:33Z

I was sort of thinking that an Observer pattern (similar to the c-APIs, but with platform-specific facilities and sugar) would be our first-class response interface, given the flexibility it affords.

goaway · 2019-07-16T23:01:05Z

Something very roughly like:

onResponse(StatusCode x, ResponseHeaders y)
onData(ByteBuffer b)
onCompletion(ResponseTrailers t)
onError(Error e)

junr03

In addition to @goaway's more general comment, a couple more specific comments/questions.

junr03 · 2019-07-16T22:54:30Z

library/kotlin/src/io/envoyproxy/envoymobile/Result.kt

+    if (status != other.status) return false
+    if (body != null) {
+      if (other.body == null) return false
+      if (!body.contentEquals(other.body)) return false


does contentEquals not deal with null content?

The primary issue is that we are using a ByteArray and that class doesn't have a predefined equals/hashcode method.

We can create our own byte array class/wrapper which handles this and then we'll be fine using that going forward. Since we are planning to swap this out with some input stream which is more common, I was thinking of addressing this issue then

junr03 · 2019-07-16T22:55:32Z

library/kotlin/src/io/envoyproxy/envoymobile/Result.kt

+    return true
+  }
+
+  override fun hashCode(): Int {


interesting there is no default hashing on user defined classes?

junr03 · 2019-07-16T23:01:16Z

library/kotlin/src/io/envoyproxy/envoymobile/Result.kt

+
+
+  /**
+   * A side effect function for successful Results


comment needs updating

junr03 · 2019-07-16T23:05:56Z

library/kotlin/src/io/envoyproxy/envoymobile/Result.kt

+   * result.fold({ response -> response.body }, { failure -> throw Exception(failure.cause) })
+   *
+   * // To return a default value of 0 when a failure occurs
+   * result.fold({ response -> 1 }, { failure -> 0 })


Sorry, this is my confused eyes as a kotlin ignorant, but how does the user interact with the result. Do they write complex folds where the logic is after -> for response, and failure? And they have access to the Response and the NetworkError.

I have an example usage of this up in the description but you do have the right idea

goaway · 2019-07-17T02:10:52Z

I should say that I do think the Response object in here is a useful interface, and that the usage examples are elegant.

But I'd like to maybe take some time to think about how it can work as part of or alongside an interface that allows application processing to proceed incrementally as data comes in. (This is possible for headers as well, but generally somewhat less important than for the body payload.)

rebello95 · 2019-07-17T04:45:02Z

This echoes my question on the Request interfaces, but have we thought at all about how might expose response data for streamed processing?

Definitely a great callout, and I think it's worth discussing prior to merging the Swift/Kotlin initial interface implementations. Some options @buildbreaker and I were discussing earlier, just to outline them here:

The ergonomic Result types available in Swift/Kotlin are unavailable in Objective-C/Java, so unfortunately we need to come up with an option that supports this functionality in all 4 languages.

Adding a Callback class that can be initialized with the success/failure lambdas
Passing success/failure lambdas directly in to the request function, one of which will be called on completion
Using the custom Result types here, providing a single lambda which gets called on completion with the result

That said, we could also go down a path like what you suggested @goaway and essentially expose the interfaces available from C that are checked in today, and provide a bit of ergonomics on top of those (i.e., using Response, parsing out status codes, etc.). To be honest, I think this is probably the most practical way to support streaming.

If this is a path we want to explore, @buildbreaker and I can definitely take a stab at it

rebello95 · 2019-07-18T06:18:41Z

@buildbreaker opened another PR with some options for response interfaces based on the discussion above: #265

I went ahead and left some comments if you want to also take a look @goaway @junr03

buildbreaker · 2019-07-18T23:52:58Z

Going to go in a different direction: #265

Alan Chiu added 5 commits July 15, 2019 12:25

Add response objects

fef9085

Signed-off-by: Alan Chiu <achiu@lyft.com>

add to build

51bf907

Signed-off-by: Alan Chiu <achiu@lyft.com>

Add empty tests

f77d945

Signed-off-by: Alan Chiu <achiu@lyft.com>

Add tests

b5aabde

Signed-off-by: Alan Chiu <achiu@lyft.com>

fmt

c7046ff

Signed-off-by: Alan Chiu <achiu@lyft.com>

buildbreaker changed the title ~~kotlin: Add response objects~~ kotlin: add library response interfaces Jul 15, 2019

buildbreaker marked this pull request as ready for review July 15, 2019 19:40

buildbreaker requested review from goaway, junr03 and rebello95 July 15, 2019 19:41

buildbreaker mentioned this pull request Jul 15, 2019

android: Unary HTTP request interfaces #119

Closed

Alan Chiu added 2 commits July 15, 2019 17:29

forgot status

7681e7c

Signed-off-by: Alan Chiu <achiu@lyft.com>

add status test for builder

4b6ac88

Signed-off-by: Alan Chiu <achiu@lyft.com>

rebello95 mentioned this pull request Jul 16, 2019

swift: add response interfaces #261

Closed

rebello95 reviewed Jul 16, 2019

View reviewed changes

library/kotlin/src/io/envoyproxy/envoymobile/ResponseBuilder.kt Outdated Show resolved Hide resolved

Alan Chiu added 4 commits July 16, 2019 12:03

comment update

0f2d9ee

Signed-off-by: Alan Chiu <achiu@lyft.com>

add result

a26b08f

Signed-off-by: Alan Chiu <achiu@lyft.com>

tools

6286b10

Signed-off-by: Alan Chiu <achiu@lyft.com>

fix test

6209fac

Signed-off-by: Alan Chiu <achiu@lyft.com>

junr03 reviewed Jul 16, 2019

View reviewed changes

buildbreaker closed this Jul 18, 2019

buildbreaker deleted the add-response-objects branch July 18, 2019 23:53

Conversation

buildbreaker commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebello95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goaway commented Jul 16, 2019

Uh oh!

goaway commented Jul 16, 2019

Uh oh!

goaway commented Jul 16, 2019

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

junr03 Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

buildbreaker Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

junr03 Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

junr03 Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

buildbreaker Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

goaway commented Jul 17, 2019

Uh oh!

rebello95 commented Jul 17, 2019

Uh oh!

rebello95 commented Jul 18, 2019

Uh oh!

buildbreaker commented Jul 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

buildbreaker commented Jul 15, 2019 •

edited

Loading

buildbreaker Jul 16, 2019 •

edited

Loading