airframe-http: #1100 Support unary and n-ary functions in RPC #1126

xerial · 2020-06-10T21:24:46Z

Addresses #1100

Support an arbitrary number of function arguments for RPC calls.
sbt-airframe: If RPC call requires multiple arguments, wrap them into a single case class and send it as JSON or MsgPack.
Changed to throw IllegalArgumentException and 400 (BadRequest) response when incompatible type data is used in HTTP requests. Note this change still doesn't cover cases described in airframe-http: Validation for unnecessary attributes #978 (unnecessary attributes are given)
Properly handle application/octet-stream requests so as not to bind uploaded file contents to RPC arguments

codecov · 2020-06-10T21:30:00Z

Codecov Report

Merging #1126 into master will decrease coverage by 0.14%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master    #1126      +/-   ##
==========================================
- Coverage   82.58%   82.43%   -0.15%     
==========================================
  Files         272      276       +4     
  Lines       10427    10634     +207     
  Branches      684      686       +2     
==========================================
+ Hits         8611     8766     +155     
- Misses       1816     1868      +52

Impacted Files	Coverage Δ
...airframe/http/codegen/client/ScalaHttpClient.scala	`91.89% <ø> (ø)`
...t/airframe/http/router/HttpRequestDispatcher.scala	`90.90% <ø> (ø)`
...rc/main/scala/wvlet/airframe/http/HttpClient.scala	`94.23% <ø> (ø)`
...c/main/scala/wvlet/airframe/http/HttpContext.scala	`73.33% <ø> (ø)`
...ala/wvlet/airframe/http/codegen/HttpClientIR.scala	`70.11% <36.84%> (-23.54%)`	⬇️
...n/scala/wvlet/airframe/codec/CollectionCodec.scala	`86.31% <64.70%> (-5.15%)`	⬇️
.../main/scala/wvlet/airframe/http/router/Route.scala	`83.33% <75.00%> (-4.17%)`	⬇️
...wvlet/airframe/http/router/HttpRequestMapper.scala	`97.50% <97.43%> (+0.53%)`	⬆️
...cala/wvlet/airframe/codec/ScalaStandardCodec.scala	`96.57% <100.00%> (+0.04%)`	⬆️
.../main/scala/wvlet/airframe/msgpack/spi/Value.scala	`81.42% <100.00%> (+2.44%)`	⬆️
... and 19 more

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 4e7611f...3b057f1. Read the comment docs.

xerial · 2020-06-11T01:29:22Z

@takezoe @shimamoto I revised airframe-http Endpoint/RPC mapping so that we can support any type of functions including unary, n-ary functions even though primitive type values are involved.

I know this PR is fairly complicated, but this change will affect all airframe-http applications, so it's better to have your look.

takezoe · 2020-06-11T17:06:53Z

airframe-http/.jvm/src/main/scala/wvlet/airframe/http/router/HttpRequestMapper.scala

+                    Some(argCodec.fromMsgPack(paramValue.toMsgpack))
+                  case None =>
+                    // When the target parameter is not found in the MapValue, try mapping the content body as a whole
+                    argCodec.unpackMsgPack(msgpack)


Though I know this is an original behavior of the parameter mapping in airframe-http, this behavior is sometimes convenient to take whole request body as a string, however, it might be a little dangerous because this happens unintentionally when we give a wrong parameter name.

def rpc1(name: String) = {} {"name": "hello"} => "hello" {"meme": "hello"} => """{"meme": "hello"}"""

If we need to do this intentionally, it would be better to use HttpRequest injection than relying on this fallback behavior.

Yes. That's an issue and I'm worrying the compatibility of existing applications using airframe-http. For applications using sbt-airframe (e.g., Conductor), we can fix both of the client and server code at the same time, so we don't need to worry about the compatibility.

An idea is totally deprecating the whole content body mapping as you suggested, and ask users to use Request object.

Filed this as #1129. At least for RPC, which uses generated clients, we don't need to support the whole body mapping

takezoe · 2020-06-11T17:10:45Z

airframe-http/.jvm/src/test/scala/wvlet/airframe/http/HttpRequestMapperTest.scala

+
+  @RPC
+  trait MyApi {
+    def rpc1(p1: String): Unit = {}


By the way, how does airframe-http work if we define the same name methods as follows?

def rpc1(p1: String): Unit = {} def rpc1(p1: String, p1: Int): Unit = {}

It will throw a runtime exception when building a Router because these methods will be detected as non-unique mappings.

I think we can generate different HTTP path mappings for the same name methods. I'll file a ticket.

Filed #1128

takezoe · 2020-06-11T17:34:43Z

I saw unintuitive cases in array (and map) conversion.

def test(p1: Int) = {}
{"p1": [1]} => Seq(null)

def test(p1: Option[Int]) = {}
{"p1": [1]} => Seq(Some(null))

I think it's better to throw an exception in these cases, or use the default value (0 in the first case and None in the second case) at least.

takezoe · 2020-06-11T17:51:58Z

In the mapping from the query string, Seq doesn't work for unary parameter while it works for a nested object. Is this a limitation in the unary parameter?

// Query string: ?name=aaaa&name=bbbb

// This doesn't work
def test(name: Seq[String]) = {}

// This works
case class NestedObject(name: Seq[String])
def test(p1: NestedObject) = {}

Even in the second case, if the query string contains a single parameter as ?name=aaaa, it doesn't work. I hope that Seq("aaaa") is generated in this case.

xerial · 2020-06-11T21:35:54Z

@takezoe Query string: ?name=aaaa&name=bbbb should be mapped to Seq[String]. I'll add test cases as well as the cases for

def test(p1: Int) = {}
{"p1": [1]} => Seq(null)  <-- throw an Exception for incompatible types

def test(p1: Option[Int]) = {}
{"p1": [1]} => Seq(Some(null)) <-- throw an Excecption for incompatible types

xerial · 2020-06-12T07:48:56Z

@takezoe Fixed the corner cases mentioned in the above and filed #1128 and #1129 as future work

takezoe · 2020-06-15T02:35:24Z

Thanks for fixing! Please allow me to ask about another corner case.

How should Seq mapping from the query string work if the query string has no corresponding parameter?

def endpoint3(p1: Seq[String]): Unit = {} // => IllegalArgumentException
def endpoint3(p1: Option[Seq[String]]): Unit = {} // => IllegalArgumentException

It would be nice if the first case will be Nil and the second case will be None as same as the mapping from JSON.

xerial · 2020-06-15T04:29:47Z

@takezoe Right. It should bind Seq.empty and None respectively. Added test cases and found how to properly handle these cases (primitive, Seq[primitive], Option[primitive], Option[Seq[primitive]]). Thanks for the comment.

takezoe

Great thanks! I have no other cases which don't work expected for now.

xerial · 2020-06-15T17:58:14Z

Awesome. Your review was really helpful. I appreciate it.

xerial added 8 commits June 9, 2020 22:51

Add doc

5a06a94

Add a request mapping test

f9878d4

Support unary args

e4b6150

Bind multiple arguments

5012ed9

Fix test

b7dffcd

Add Option mapping tests

d382d1f

Fix option mappings

14bb1cb

Add more tests

3f668e2

xerial added 5 commits June 10, 2020 14:49

Cleanup tests

17d7276

Generate a case class for wrapping RPC request parameters

d27ea21

Add a blank line

2174f9d

Add a blank line

94e7642

Fix format of generated client codes

77a4bee

xerial changed the title ~~airframe-http: #1100 Support nested JSON in RPC mapping~~ airframe-http: #1100 Support unary and n-ary functions in RPC Jun 10, 2020

xerial added 3 commits June 10, 2020 17:37

Throw an exception when RPC mapping fails

ceeff03

simplify

f99e744

Ensure recording RPC interface access logs upon exception

a11c8e2

xerial requested review from shimamoto and takezoe June 11, 2020 01:26

takezoe reviewed Jun 11, 2020

View reviewed changes

xerial added 3 commits June 11, 2020 22:39

Support multiple query parameters mapping

37df591

Support atom or seq in SeqCodec

1c80c2f

Fix OptionCodec when parsing elements failed

13af195

xerial added 2 commits June 14, 2020 21:25

Support emtpy binding for Primitive, Seq[Primitive], Option[Primitive]

c4b69c5

Add tests for nomal cases

65351ae

xerial added 2 commits June 14, 2020 21:30

Fix comment

381d07d

Fix comment

3b057f1

takezoe approved these changes Jun 15, 2020

View reviewed changes

xerial merged commit 0d2f280 into wvlet:master Jun 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

airframe-http: #1100 Support unary and n-ary functions in RPC #1126

airframe-http: #1100 Support unary and n-ary functions in RPC #1126

xerial commented Jun 10, 2020 •

edited

codecov bot commented Jun 10, 2020 •

edited

xerial commented Jun 11, 2020

takezoe Jun 11, 2020 •

edited

xerial Jun 11, 2020

xerial Jun 12, 2020

takezoe Jun 11, 2020 •

edited

xerial Jun 11, 2020

xerial Jun 11, 2020

takezoe commented Jun 11, 2020 •

edited

takezoe commented Jun 11, 2020 •

edited

xerial commented Jun 11, 2020 •

edited

xerial commented Jun 12, 2020

takezoe commented Jun 15, 2020 •

edited

xerial commented Jun 15, 2020 •

edited

takezoe left a comment

xerial commented Jun 15, 2020

airframe-http: #1100 Support unary and n-ary functions in RPC #1126

airframe-http: #1100 Support unary and n-ary functions in RPC #1126

Conversation

xerial commented Jun 10, 2020 • edited

codecov bot commented Jun 10, 2020 • edited

Codecov Report

xerial commented Jun 11, 2020

takezoe Jun 11, 2020 • edited

Choose a reason for hiding this comment

xerial Jun 11, 2020

Choose a reason for hiding this comment

xerial Jun 12, 2020

Choose a reason for hiding this comment

takezoe Jun 11, 2020 • edited

Choose a reason for hiding this comment

xerial Jun 11, 2020

Choose a reason for hiding this comment

xerial Jun 11, 2020

Choose a reason for hiding this comment

takezoe commented Jun 11, 2020 • edited

takezoe commented Jun 11, 2020 • edited

xerial commented Jun 11, 2020 • edited

xerial commented Jun 12, 2020

takezoe commented Jun 15, 2020 • edited

xerial commented Jun 15, 2020 • edited

takezoe left a comment

Choose a reason for hiding this comment

xerial commented Jun 15, 2020

xerial commented Jun 10, 2020 •

edited

codecov bot commented Jun 10, 2020 •

edited

takezoe Jun 11, 2020 •

edited

takezoe Jun 11, 2020 •

edited

takezoe commented Jun 11, 2020 •

edited

takezoe commented Jun 11, 2020 •

edited

xerial commented Jun 11, 2020 •

edited

takezoe commented Jun 15, 2020 •

edited

xerial commented Jun 15, 2020 •

edited