feat: http response as endpoint return type #1965

aludwiko · 2024-01-11T12:36:16Z

References

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

octonato

Looking good.

I think we will need some more iterations to explore some API ergonomics, but this is moving now into an interesting direction.

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

sdk/java-sdk-protobuf/src/main/scala/kalix/javasdk/impl/HttpResponseImpl.scala

sdk/java-sdk-spring/src/it/java/com/example/wiring/actions/echo/ActionWithHttpResponse.java

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

octonato · 2024-01-12T15:44:03Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JsonSupport.java

@@ -39,6 +39,7 @@
 import com.google.protobuf.UnsafeByteOperations;
 import kalix.javasdk.annotations.Migration;
 import kalix.javasdk.impl.ByteStringEncoding;
+import org.jetbrains.annotations.NotNull;


jetbrains dependency?

removed, and content type is once again optional.

Using optional will be annoying for Scala and for users as well.

Since the API is controlling the payload already, we can always set the content-type behind the scene.

when user passes String it will be text/plain

when user passes Object it will be application/json

when user passes bytes[] it will be application/octet-stream

The NoContent one can default to application/octet-stream which is not wrong since we are sending byte[0].

Jetbrains dependency still being imported here.

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/StatusCode.java

sdk/java-sdk-protobuf/src/main/scala/kalix/javasdk/impl/HttpResponseImpl.scala

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

octonato · 2024-01-15T15:03:50Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/JsonSupport.java

@@ -39,6 +39,7 @@
 import com.google.protobuf.UnsafeByteOperations;
 import kalix.javasdk.annotations.Migration;
 import kalix.javasdk.impl.ByteStringEncoding;
+import org.jetbrains.annotations.NotNull;


Using optional will be annoying for Scala and for users as well.

Since the API is controlling the payload already, we can always set the content-type behind the scene.

when user passes String it will be text/plain

when user passes Object it will be application/json

when user passes bytes[] it will be application/octet-stream

The NoContent one can default to application/octet-stream which is not wrong since we are sending byte[0].

octonato · 2024-01-15T15:23:09Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

+    public <T> T bodyAs(Class<T> clz) throws IOException {
+      return JsonSupport.parseBytes(body, clz);
+    }


I realise now that this method can only be useful if added to the HttpResponse.

When we receive a response from the ComponentClient, we will take the internal http response from the WebClient and map it to HttpResponse, no?

Actually, it seems to me that the only public type should be HttpResponse.

So, in that case, it must communicate in its name that it is only applicable with payload is json. Something like jsonBodyAs(Class<T> clz)

Having HttpResponse as the only public class would introduce another problems, for instance we allow to change the content-type only in case of ByteContentResponse. If we move this method to the interface, then other implementations will throw an exception. But I agree that in fact you will not be able to use this method, because I'm parsing the response always to ByteContentReponse.

Maybe it's time to move one step back and remove all the specific implementations and have only a single class with factory methods? and then we could have jsonBodyAs(Class<T> clz)?

Yeah, I was having the same thought. We can keep the specific methods, but one could always change the content-type and status code later. Not a good idea, but it will make things much simpler.

So, for example, if I really want to mess-up, I can do HttpResponse.ok(Foo). That will return a 200 with json. Then HttpResponse.ok(Foo).withContentType(whatever).withStatusCode(NotFound).

I would be messing up, but then we can argue that the user chose to do so. 🤷

An alternative could be that we only offer a method to change the content-type, but not the status code nor the body. This is determined by the helper method ok(T), ok(byte[]), noContent(), notFound(), etc.

I would have more faith in our users. This is low-level api, they shouldn't use it for normal cases, we just want to support very specific cases.

You can check the implementation now.

octonato

Taking good shape.

I left a few more comments. But I think it will be my last ones. 😄

Before merging we should exercise it a bit to ensure we have got a good API.

octonato · 2024-01-16T16:37:19Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

+  public HttpResponse withStatusCode(StatusCode.Success statusCode) {
+    return new HttpResponse(statusCode, contentType, body);
+  }


I still think this is not ok. If we are providing methods that already define the status code, we should not provide this method.

It's quite strange to have something like HttpResponse.ok(..).withStatusCode(somethingElse).
We will be forcing people to first build a 200 and then switch to another code.

I agree that it will be massive to have specific methods for each of the 10 success status code we support. Also because most of them are not that common to justify helper methods.

So maybe, alongside ok(), noContent(), created() and accepted(), we can provide HttpResponse.for(StatusCode.Success code, String contentType, byte[] payload). That last would be the more low-level power user one.

Then the options would be:

either use one of the most common helper methods, eg: ok(), created(), etc

or build a HttpResponse fully from scratch by passing each piece: code, content-type and bytes.

ok, it was like that at the beginning, I will revert it

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

octonato · 2024-01-16T16:40:14Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

+  }
+
+  public static HttpResponse ok(Object object) {
+    if (object == null) throw new IllegalArgumentException("object must not be null");


isn't null a valid json object?

I would answer the same as above.

I agree for byte[], but not for Object. Json spec accept defines null as a valid value (or should I say non-value?).

Although we might find it a bad idea, it might be something that we should allow.

How would you like to serialize it and then deserialize? empty byte array = null object? Also, we can always remove this check later.

Sure, we can add it later.

But to answer your question, when we build the HttpBody, we simply set the payload. When we read it back, we fallback to null.

Anyway, let's leave it for later. Better to start restrictive and relax the rules then the other way around.

octonato · 2024-01-16T16:41:13Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

+    if (text == null) throw new IllegalArgumentException("text must not be null");
+    return new HttpResponse(StatusCode.Success.OK, "text/plain", text.getBytes(UTF_8));


would it be a good idea to send an empty byte[] instead of throwing exception?

Not sure, null might indicate an implementation problem that an empty array will hide.

octonato · 2024-01-16T16:45:50Z

sdk/java-sdk-protobuf/src/main/scala/kalix/javasdk/impl/workflow/WorkflowRouter.scala

+          HttpResponse
+            .ok(httpBody.getData.toByteArray)
+            .withStatusCode(StatusCode.Success.from(statusCode))
+            .withContentType(contentType)


Related to my other comment. If we have a method with all three param, we can build it in one shot.

octonato · 2024-01-16T16:51:55Z

sdk/java-sdk-protobuf/src/main/scala/kalix/javasdk/impl/workflow/WorkflowRouter.scala

    if (result.typeUrl == JsonSupport.KALIX_JSON + "object") {
      val typeUrl = messageCodec.typeUrlFor(expectedInputClass)
      messageCodec.decodeMessage(result.copy(typeUrl = typeUrl))
+    } else if (result.typeUrl == "type.googleapis.com/google.api.HttpBody") {
+      val httpBody = HttpBody.parseFrom(result.value.newCodedInput())
+      httpBody.getExtensionsList.asScala.find(_.getTypeUrl == "Status-Code") match {


Here we can use HttpBodyStatusCodeExtensionTypeUrl.

Unfortunately not, it's from the java-sdk-spring module. Maybe we should prefix the constant somehow to improve the text-based searching?

I think we have a place in the proto sdk where we have some other constants defined.

Actually, I can flip the dependency, and now I think it's fine.

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

octonato · 2024-01-18T09:56:41Z

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java

I think you removed by accident the jsonBodyAs method, no?

Yes, it's back now

updating tests HttpBody to HttpResponse more null safety missing header fmt fixing workflows tests fmt Apply suggestions from code review Co-authored-by: Renato Cavalcanti <[email protected]> specig HttpResponse types addressing PR comments fmt updates after PR review removing dedicate HttpResponse subclasses Apply suggestions from code review Co-authored-by: Renato Cavalcanti <[email protected]> PR comments constant comment test compilation adressing PR comments no content fixed missing docs docs typo Update sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java Co-authored-by: Renato Cavalcanti <[email protected]> missing method for parsing bytes to json # Conflicts: # sdk/java-sdk-spring/src/it/java/com/example/wiring/SpringSdkIntegrationTest.java

octonato

Let's ship it!

Thanks for the patience with all the change requests.

github-actions bot added java-sdk java-sdk-protobuf kalix-runtime Runtime and SDKs sub-team labels Jan 11, 2024

aludwiko commented Jan 11, 2024

View reviewed changes

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java Show resolved Hide resolved

aludwiko commented Jan 11, 2024

View reviewed changes

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java Outdated Show resolved Hide resolved

aludwiko commented Jan 11, 2024

View reviewed changes

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java Outdated Show resolved Hide resolved

aludwiko commented Jan 11, 2024

View reviewed changes

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java Show resolved Hide resolved

aludwiko commented Jan 11, 2024

View reviewed changes

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java Outdated Show resolved Hide resolved

octonato reviewed Jan 11, 2024

View reviewed changes

aludwiko commented Jan 11, 2024

View reviewed changes

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java Outdated Show resolved Hide resolved

aludwiko commented Jan 12, 2024

View reviewed changes

sdk/java-sdk-protobuf/src/main/scala/kalix/javasdk/impl/HttpResponseImpl.scala Outdated Show resolved Hide resolved

johanandren reviewed Jan 12, 2024

View reviewed changes

sdk/java-sdk-spring/src/it/java/com/example/wiring/actions/echo/ActionWithHttpResponse.java Outdated Show resolved Hide resolved

aludwiko force-pushed the http-response-highlevel-api branch from 505a9d0 to 6e07ef9 Compare January 12, 2024 10:00

octonato reviewed Jan 12, 2024

View reviewed changes

sdk/java-sdk-protobuf/src/main/java/kalix/javasdk/HttpResponse.java Outdated Show resolved Hide resolved

octonato reviewed Jan 15, 2024

View reviewed changes

github-actions bot added the Documentation Improvements or additions to the documentation label Jan 15, 2024

aludwiko force-pushed the http-response-highlevel-api branch from 68f2b1d to 6247e7a Compare January 15, 2024 16:33

github-actions bot removed the Documentation Improvements or additions to the documentation label Jan 15, 2024

octonato reviewed Jan 16, 2024

View reviewed changes

aludwiko requested a review from octonato January 17, 2024 08:50

octonato reviewed Jan 18, 2024

View reviewed changes

aludwiko force-pushed the http-response-highlevel-api branch from e910a36 to a4058e3 Compare January 18, 2024 13:39

aludwiko requested a review from octonato January 18, 2024 13:50

octonato approved these changes Jan 18, 2024

View reviewed changes

octonato merged commit aac3509 into main Jan 18, 2024
68 checks passed

octonato deleted the http-response-highlevel-api branch January 18, 2024 14:03

aludwiko mentioned this pull request Jan 23, 2024

feat: support HttpBody as a response type (code first sdk) #1939

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: http response as endpoint return type #1965

feat: http response as endpoint return type #1965

aludwiko commented Jan 11, 2024

octonato left a comment

octonato Jan 12, 2024

aludwiko Jan 15, 2024 •

edited

Loading

octonato Jan 15, 2024

aludwiko Jan 15, 2024

octonato Jan 17, 2024

aludwiko Jan 17, 2024

octonato Jan 15, 2024

octonato Jan 15, 2024 •

edited

Loading

aludwiko Jan 15, 2024

aludwiko Jan 15, 2024

octonato Jan 16, 2024

octonato Jan 16, 2024

aludwiko Jan 16, 2024

aludwiko Jan 16, 2024

octonato left a comment

octonato Jan 16, 2024

aludwiko Jan 17, 2024

octonato Jan 16, 2024

aludwiko Jan 17, 2024

octonato Jan 17, 2024

aludwiko Jan 17, 2024

octonato Jan 17, 2024

octonato Jan 16, 2024

aludwiko Jan 17, 2024

octonato Jan 16, 2024

octonato Jan 16, 2024

aludwiko Jan 17, 2024

octonato Jan 17, 2024

aludwiko Jan 17, 2024

octonato Jan 18, 2024

aludwiko Jan 18, 2024

octonato left a comment

		if (text == null) throw new IllegalArgumentException("text must not be null");
		return new HttpResponse(StatusCode.Success.OK, "text/plain", text.getBytes(UTF_8));

feat: http response as endpoint return type #1965

feat: http response as endpoint return type #1965

Conversation

aludwiko commented Jan 11, 2024

References

octonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aludwiko Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato left a comment

Choose a reason for hiding this comment

aludwiko Jan 15, 2024 •

edited

Loading

octonato Jan 15, 2024 •

edited

Loading