New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle no body as a valid input #1475

Merged
merged 9 commits into from Sep 5, 2018

Conversation

@idoshamun
Contributor

idoshamun commented Jul 22, 2018

Purpose

Allow sending requests with no body without throwing an exception.
Some services might define their input as Option but currently you must send body
with your request if the service has an input even if the body is {}.

My fix checks if the input byte stream is empty and if so it returns an empty JsObject instead of throwing
DeserializationException.

Notes

Please let me know if tests are necessary because I could not find tests for MessageSerializer.

@idoshamun idoshamun changed the title from handle empty body as a valid input to handle no body as a valid input Jul 22, 2018

@erip

This comment has been minimized.

Contributor

erip commented Jul 22, 2018

Take it with a grain of salt, but from a style perspective, I think it would make more sense to represent an empty JSON object in play-json with JsObject(Nil) or JsObject(Seq.empty).

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Jul 23, 2018

@erip thanks! fixed :)

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Jul 23, 2018

Hi @idoshamun, I'm a bit hesitant to allow request payloads to be Option[T]. Can you explain a use case where that's desired?

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Jul 23, 2018

Hi @ignasi35 , it's not just Option[T], it's also T(a: Option[A]) for example.
My use case is to have optional body for setting some parameters with default value.
For example, I have a request which depends on a third party service so I would like the client specify the timeout duration for the third party request.
This is just one example but I believe that optional parameters are common and there are many use cases.

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Jul 23, 2018

... it's not just Option[T], it's also T(a: Option[A]) ...

I thought case class MyPayload(maybeField: Option[String]) was already supported. I think we actually have some sample code where payload classes (used in both request and response) have Option[_] fields. I don't remember how is None serialized though. Is it {"maybeField":null} or {}?

I guess there are several small features to consider:

  1. supporting a payload of type Option[T]
  2. supporting fields of type Option[T]
  3. supporting (or customizing) the serialization of a field Option[T] in a payload ( no field added VS field is added with null value).

(EDIT: wrong formatting on my original writeup)

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Jul 23, 2018

Actually case class MyPayload(maybeField: Option[String]) is supported by it forces the request to have a body even if it's empty {}. My PR enables to send the request without body at all

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Jul 24, 2018

@ignasi35 How do you think we should proceed?

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Jul 24, 2018

hey @idoshamun I'm a bit divided. I like that it makes the services more robust but I don't like that it is semantically weird: is None while {} is MyThing(None).

I'd like more opinions.

PS: If we push forward with the change, it'd be great to have tests for it too but I wouldn't want you to put too much effort and then we decide the PR is discarded.

@ignasi35 ignasi35 self-assigned this Jul 24, 2018

@TimMoore

This comment has been minimized.

Member

TimMoore commented Jul 25, 2018

Hmm, so I think that supporting Option[T] does make some sense, but automatically handling T(Option[A]) does not, because it doesn't generalize.

What if your top-level class is case class MyPayload(maybeField: Option[String], maybeOtherField: Option[String]) or case class MyPayload(maybeField: Option[String], definitelyField: String)

I think if you declare a non-optional request type, the default behavior should be to require a body that can deserialize to that type... but of course you are also free to implement a custom serializer if you prefer different behavior.

@TimMoore

This comment has been minimized.

Member

TimMoore commented Jul 25, 2018

Another thought: why does an empty JSON body translate to {} and not null, or "" or []?

I don't think this is a really obvious thing, so it's better to be more explicit.

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Jul 25, 2018

@TimMoore IMHO, I am not sure it really matters as they are all mean "no data", as long as they can be serialized to None later on.
Coming from other frameworks usually when you set parameters to be optional or have a default value, you don't want to force body because as a client it doesn't make sense to send {} as body.

EDIT: currently I am using custom serializer, this is what made me open this PR

@TimMoore

This comment has been minimized.

Member

TimMoore commented Jul 25, 2018

@idoshamun your implementation doesn't deserialize to None, it deserializes to an empty object. It works because your case class happens to define its member as Option[T], so an empty object deserializes successfully.

My point is, why then can't you define a request of type String with an empty body (which would require substituting "" instead of {}) or a request of type List[T]? This solution seems very specific to your particular usage scenario.

@TimMoore

This comment has been minimized.

Member

TimMoore commented Jul 25, 2018

Personally, however, I would be OK with defining a new StrictMessageSerializer[Option[T]] that would allow you to declare a ServiceCall[Option[T], Response] that handles an empty body by deserializing the request to None. This would compose well with other (non-JSON) message serializers.

Would that suit you @idoshamun?

@ignasi35 you said you don't like that idea… can you elaborate?

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Jul 25, 2018

I think that {} will be serialized not just to None but also to default values if set in the case class. So for example if I define case class A(str: String = "hello") it should be fine if I don't send body in my request. On the contrary If I set ServiceCall[String, Response] than I agree that it is not obvious to send empty body and that body must be presented same with any other date type as well.

I think that empty body should cover only the following use cases:

  • ServiceCall[Option[T], Response] where empty body will be translated to None.
  • ServiceCall[T, Response] where empty body will be translated to T().

In any other case an exception will be thrown.

@TimMoore regarding your question, a new StrictMessageSerializer[Option[T]] is fine by me as I already use it, if we can't agree on how it should be implemented inside JsValueDeserializer.

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Jul 25, 2018

I think should not be T() in any case. I don't find that idiomatic in either scala or javascript.

> var x = new Object();
undefined
> JSON.stringify(x);
"{}"

I don't like that we're discussing wire-level details considering Lagom is an RPC where wire-level details should just work ™️ . I get the impression @idoshamun is using the Lagom endpoint from non-Lagom clients (which is great) but I find it strange that those clients would serialize T() as .


We've discussed (in the past) whether ServiceCall[Option[T], Response] should be supported or was a design smell. I'm not a big supporter of the idea of Option[T] request payloads. The reason why I'm not a supporter of Option[T] request payloads is because I'm not sure I understand what they mean. To me client.doStuff.invoke(None) should be replaced with client.reset.invoke() or client.delete.invoke() but I don't have a clear use case in mind and that prevents me from seeing the real value of this change.

Having said that, I think a possible solution considering all the opinions above is:

  • allow ServiceCall[Option[T], Response] where empty body should be translated to None. This requires having a StrictMessageSerializer[Option[T]] (provided by Lagom or custom built on the service).
  • ServiceCall[T, Response] should fail when receiving an empty body. The signature states it needs a T and it's not getting it. <-- this is what was bugging me.

Note that Option[T] response payloads are a problem I'd like to discuss separately. SPOILER: 404 Not Found, 200 OK + empty payload?

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Jul 25, 2018

@ignasi35 I indeed use Lagom endpoints with non-Lagom clients, which I think will become a very common use case as Lagom will be more popular. This is why it's important to also think about the interoperability of Lagom with other systems.

I can understand that using Option[T] as a payload might be a design smell as most of the time it is used to set some defaults, so why don't you use defaults in the first place. But again even with defaults Lagom doesn't have a proper solution for empty body but throwing an exception.

But I think the main difference is that we look differently at Lagom, you see it as a RPC framework and I see it as a microservice framework that should integrate nicely with my other systems.

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Jul 25, 2018

But I think the main difference is that we look differently at Lagom, you see it as an RPC framework and I see it as a microservice framework that should integrate nicely with my other systems.

Interop is important for Lagom as it is for any other stack that participates on a micro-services system. We're adding support for gRPC for that reason.

I think supporting Option[T] request payloads is the way forward. I actually maps quite well with OpenAPI's required field to describe request bodies (defaults to false).

Then the solution would be:

  • work to support ServiceCall[Option[T], Response] where empty body is possible and is translated to None.
  • ServiceCall[T, Response] is equivalent to OpenAPI's required: true and it should fail when receiving an empty body.
@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Jul 25, 2018

@ignasi35 sounds like a plan! OpenAPI is a good reference for this kind of discussions.
How would you like to proceed?
My idea is to first write test cases and then fix my implementation to pass the tests.

@TimMoore

This comment has been minimized.

Member

TimMoore commented Jul 27, 2018

Sounds good to me

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Jul 27, 2018

Could you refer me please to similar test cases as currently there are no tests for the MessageSerializers?

@TimMoore

This comment has been minimized.

Member

TimMoore commented Jul 27, 2018

Oh, that's too bad that there are no existing tests. I would use unit tests for these with hardcoded JSON strings.

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Aug 8, 2018

I am still on this PR but lately I'm too busy, hope to get to it in the coming days

idoshamun added some commits Aug 28, 2018

override def writes(o: Option[T]): JsValue = o match {
case Some(t) implicitly[Writes[T]].writes(t)
case None JsNull

This comment has been minimized.

@ignasi35

ignasi35 Aug 29, 2018

Member

Prefer => over . Lagom doesn't share the code style that Akka uses. :-/

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Aug 29, 2018

(just to be clear since i believe this showed up indirectly)

#### "null" payloads

This PR is intended to handle empty body for requests where a body is expected. It is not in scope to support empty body responses or null payloads (e.g. setting null as an argument when invokeing the remote call method). I think that should be considered separately. I'm not even talking about the wire-level implementation to transport that null from the client to the server (that'd be a serializer-dependant detail).

What I'm saying is whether we should or should not support client code like serviceClient.sayHello().invoke(null) for def sayHello: ServiceCall[String, String]. I am undecided but inclined towards not supporting it. I'd keep this discussion for a separate PR.

Pasting some test code I used to play around until I realised it was out of scope for this PR:

  "JsValueMessageSerializer" should {
     // ...
    "serialize JsNull into empty ByteString " in {
      val serializer = JsValueMessageSerializer.serializerForRequest
      serializer.serialize(JsNull) shouldBe ByteString.empty
    }
  }

That test fails with the code in this PR because serializer.serialize(JsNull) produces ByteString("null") // ByteString(110, 117, 108, 108). To be discussed on a separate PR.

Final thought: I think supporting null payloads should be addressed in round-trip use cases and should consider both requests and responses.

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Aug 29, 2018

Back to my previous comment:

  • work to support ServiceCall[Option[T], Response] where empty body is possible and is translated to None.
  • ServiceCall[T, Response] is equivalent to OpenAPI's required: true and it should fail when receiving an empty body.

As a Scala developer I would expect the following:

  1. when the type is ServiceCall[Option[T], Response]
    1. ByteString.empty becomes JsNull becomes None
    2. ByteString(...) becomes JsObject becomes Some(...)
  2. when the type is ServiceCall[T, Response]
    1. ByteString.empty becomes JsNull becomes (some type of failure, not null)
    2. ByteString(...) becomes JsObject becomes t

So back to @TimMoore's comment above:

This test fails with JsonValidationError:

    "convert JS null to null by default" in {
      implicit val format: Format[Dummy] = Json.format
      val dummy = JsNull.as[Dummy]
      dummy shouldBe null
    }

I think the behavior is correct but the test is wrong and it should be this instead:

    "fail trying to convert JSNull into T." in {
      implicit val format: Format[Dummy] = Json.format
      intercept[JsResultException] {
        JsNull.as[Dummy]
      }
    }

That is, I still think user code should never receive a null. If the payload is optional (marked required: false in OpenAPI) then, well, it's Optional. If it is required (marked required: true in OpenAPI) then it must not be null. I'm aware that this approach will forbid remotely passing null as an argument (here I was getting into a rabbit hole so I decided to create a separate section on supporting payloads that are literally "null", see below).

Note that for a Scala developer the intermediate representation of emptiness (in this case JsNull) is relevant because Scala users default to writing JsObject to T serializers (aka play json formatters).


As a Java developer I would expect the following:

  1. when the type is ServiceCall<Optional<T>, Response>
    1. ByteString.empty becomes None
    2. ByteString(...) becomes Some(...)
  2. when the type is ServiceCall[T, Response]
    1. ByteString.empty becomes (some type of failure, not null)
    2. ByteString(...) becomes t

This is slightly different because the Jackson implementation is a one shot instead of the design used in Scala which composes MessageSerializer's.

For that to work there's this weird code:

            public MessageEntity deserialize(ByteString bytes) {
                try {
                    if (bytes.isEmpty() && this.type == Optional.class) {
                        // this hack exploits Jackson's Jdk8Module which defaults to mapping
                        // "null" to "Optional.empty"
                        bytes = ByteString.fromString("null");
                    }
                    return reader.readValue(bytes.iterator().asInputStream());
                } catch (Exception e) {
                    throw new DeserializationException(e);
                }
            }

which replaces the ByteString.empty with "null" to exploit Jackson's Jdk8Module behavior. Could this be replaced with :

            public MessageEntity deserialize(ByteString bytes) {
                try {
                    if (bytes.isEmpty() && this.type == Optional.class) {
                        return (MessageEntity) Optional.empty();
                    }
                    return reader.readValue(bytes.iterator().asInputStream());
                } catch (Exception e) {
                    throw new DeserializationException(e);
                }
            }

?

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Aug 29, 2018

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Aug 29, 2018

I generally like the approach but I think there's room for a few more tests. While reviewing the PR I played a bit with the code and wrote these.

Feel free to cherry-pick, adapt, discard...

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Aug 29, 2018

@TimMoore

This comment has been minimized.

Member

TimMoore commented Aug 30, 2018

@ignasi35 what would you expect with ServiceCall[T, Response], no body, and a protobuf message serializer?

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Aug 30, 2018

@ignasi35 Regarding your suggestion to the Java implementation, unfortunately it doesn't pass the test.
I think it's because the casting itself happens outside of the function scope and doesn't ableto catch the ClassCastException and throw DeserializationException. In order to do so I have to get Class<T> as a parameter to Jackson factory, as mentioned here https://stackoverflow.com/a/14524815/2650104.
I think changing the constructor is a major change so I will keep the current implementation.

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Aug 30, 2018

what would you expect with ServiceCall[T, Response], no body, and a protobuf message serializer?

I'm not familiar with the meaning of ByteString.empty for a protobuf parser.

skims over https://developers.google.com/protocol-buffers/docs/encoding and comes back to commenting

  • when T is a protobuf-modelled type where all fields are optional then I would expect ByteString.empty to represent a T instance where every field is None but not null.
  • when T is a protobuf-modelled type where at least one field is not optional then ByteString.empty is an invalid input

In any case, it is unsupported to invoke a call passing in null as the payload.

@TimMoore

This comment has been minimized.

Member

TimMoore commented Aug 30, 2018

@ignasi35 sorry, I should have asked about ServiceCall[Option[T], Response], no body, and a protobuf parser?

What about when you support both JSON and protobuf via content negotiation? A request with no body could have no Content-type header.

I guess my point is that the handling of an empty body with Option as None could be made independent of the specific serialization method for a non-empty body. In my mind, the absence of a body is a distinct type of data from a body that represents empty data, in the same way that None is different from () or Nil.

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Aug 30, 2018

@ignasi35 @TimMoore In this case, what would be your decision?
I already implemented @ignasi35 new tests and everything passes.

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Aug 30, 2018

I guess my point is that the handling of an empty body with Option as None could be made independent of the specific serialization method for a non-empty body. In my mind, the absence of a body is a distinct type of data from a body that represents empty data, in the same way that None is different from () or Nil.

Hmmm, no (?)

The semantics of the body depend on the serializer. IIUC, in protobuf ByteString.empty represents a valid instance where all fields are None so ByteString.empty is ambiguous when the call type is ServiceCall[Option[T], Response]. This is a protobuf specific issue.

I think this PR started out implicitly assuming the serializer was the default JSON (scala or java) in which case the ambiguity doesn't exist.

So my previous comment needs reviewing:

  1. when the type is ServiceCall[Option[T], Response] and payload is JSON
    1. ByteString.empty becomes None (some intermediate serializers may choose to use JsNull, MyFancyEmpty, or whatever they document as the representation of emptiness)
    2. ByteString(...) becomes Some(...)
  2. when the type is ServiceCall[T, Response] and payload is JSON
    1. ByteString.empty becomes (some type of failure, not null)
    2. ByteString(...) becomes t

In my comment above I made the mistake of getting into the play-json implemnetation specifics. Note how I removed the intermediate JsNull from the specs.

Then, we could write:

  1. when the type is ServiceCall[Option[T], Response] and payload is protobuf (and all fields of T are optional)
    1. ByteString.empty becomes 🤷🏼‍♂️
    2. ByteString(...) becomes Some(...)
  2. when the type is ServiceCall[T, Response] and payload is protobuf (and all fields of T are optional)
    1. ByteString.empty becomes t (where all fields are None)
    2. ByteString(...) becomes t

I would focus on JSON for this PR.

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Aug 30, 2018

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Sep 5, 2018

Could someone please merge?

@ignasi35 ignasi35 merged commit aae1a0a into lagom:master Sep 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 5, 2018

hi @idoshamun, thanks for your huge patience on this whole PR. You struck the core team in vacation season (reduced team capacity) and that only helped delay the reviews and attention this PR deserved.

@ignasi35 ignasi35 added this to the Lagom 1.5.0 milestone Sep 5, 2018

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 5, 2018

@idoshamun What do you think about backporting? (cc @TimMoore @renatocaval @marcospereira)

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Sep 5, 2018

@ignasi35 no problem, I understand that it's a big project and we have to follow the guidelines and of course everyone needs a vacation now and then. I am glad to contribute back :)

You mean backporting to 1.4.x?

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 5, 2018

You mean backporting to 1.4.x?

Yes. After merging the PR on master it will only be available in Lagom 1.5.0+. We could also backport to 1.4.x so it's available since 1.4.9. It's not a huge change but it a new behavior and considering the release of 1.5.0 is near I think we shouldn't backport but I could be convinced otherwise.

@idoshamun

This comment has been minimized.

Contributor

idoshamun commented Sep 5, 2018

erip added a commit to erip/lagom that referenced this pull request Sep 30, 2018

handle no body as a valid input (lagom#1475)
Allow sending requests with no body without throwing an exception.
Some services might define their input as `Option` and general serializers may treat empty body as `None`.

@ignasi35 ignasi35 modified the milestones: Lagom 1.5.0, Lagom 1.5.0-M4 Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment