kalix-jvm-sdk icon indicating copy to clipboard operation
kalix-jvm-sdk copied to clipboard

Improve error for Action's method without proper return type

Open efgpinto opened this issue 2 years ago • 6 comments

I've wrongly declared an Action like this:

public class TestingAction extends Action {

  @GetMapping("/test")
  public String test() {
    System.out.println("Metadata: " + actionContext().metadata());
    return "OK";
  }
}

When booting up, discovery was fine, then it blew up on the first request with:

13:43:30.494 ERROR k.javasdk.impl.action.ActionsImpl - Failure during handling of command com.example.shoppingcart.TestingAction.Test
java.lang.ClassCastException: class java.lang.String cannot be cast to class kalix.javasdk.action.Action$Effect  ...

It would be better to check the return type of the action's method and at least break on discovery.

efgpinto avatar Jan 18 '24 13:01 efgpinto

An alternative would be to consider String to be the same as Effect<String>.

This will reduce quite a lot the boilerplate for cases where we want to return a simple response.

octonato avatar Jan 18 '24 19:01 octonato

An alternative would be to consider String to be the same as Effect<String>.

This will reduce quite a lot the boilerplate for cases where we want to return a simple response.

That's a very interesting idea. I think we should consider and try it out.

I assume it would work for any type, be it String or not, if not an Effect then we would do a simple .reply with the returned value, right? This would also only apply for Actions, I guess.. since for entities there are other intermediate steps using the effects().

efgpinto avatar Jan 18 '24 22:01 efgpinto

We could even take this a step further possibly:

  • return (X != Effect) <=> effects().reply
  • return CompletionStage<> <=> effects().asyncReply
  • return DeferredCall<> <=> effects().forward

Now, if you want side effects, emitting some events, etc, you would still use Effect directly. But this would cover a big percentage of the most common cases.

To me, it's not so much about the boilerplate reduction (although it's also interesting from that perspective) but the fact that a newcomer does not need to understand effects from the get-go. I see this mainly as syntax sugar. When later you need to go out of the sweetened option, it can feel weird/confusing, but at that point you'll probably be more comfortable with the remaining concepts. It's not all at once, which can be overwhelming.

efgpinto avatar Jan 19 '24 11:01 efgpinto

We raised the question the other day if it's a good idea to offer many options to do the same.

My take on that is that for the most straightforward cases, we should not need to require extra APIs. So, I personally find it a simplification and positive change.

I think that's quite easier for people to learn that they can go with a function A => B and when they need to pass more instructions to Kalix, they need to change to A => Effect[B].

octonato avatar Jan 22 '24 10:01 octonato

I don't have any strong opinions, but personally, I would introduce Effect for users event for basic use cases, just to familiarize them with the concept, which they must understand anyway.

aludwiko avatar Jan 22 '24 12:01 aludwiko

Effect will be practically unavoidable for Entities as without them you can't even start. So the most simple use case will already require the Effect API.

For read-only commands we can simply have return state instead of return effects().reply(state). But before even starting with read-only commands, users will already have to deal with Effect.

octonato avatar Jan 22 '24 14:01 octonato