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

Error with simple test using httpPutJson #257

Closed
frossi85 opened this Issue Sep 26, 2015 · 9 comments

Comments

3 participants
@frossi85

frossi85 commented Sep 26, 2015

I am trying to write a microservice, for that I need a route for update an entity. I write a simple test and allways is failing.

The test result is:

===========================================================================
HTTP PUT /events/50
[Header]    Content-Length -> 2
[Header]    Host -> 127.0.0.1:44442
50
===========================================================================
2015-09-26 00:34:26,245 INF 563936d43b0ac953          AccessLoggingFilter       127.0.0.1 - - [26/Sep/2015:03:34:26 +0000] "PUT /events/50 HTTP/1.1" 400 79 526 "-"
---------------------------------------------------------------------------
[Status]    Status(400)
[Header]    Content-Type -> application/json;charset=utf-8
[Header]    Server -> Finatra
[Header]    Date -> Sat, 26 Sep 2015 03:34:26 +00:00
[Header]    Content-Length -> 79
{
  "errors" : [
    "Can't parse request body with content-type: unknown and body: 50"
  ]
}

I could not find any example with put request, this is some of the code:

case class EventHttp2(@RouteParam id: Long)

class EventsController @Inject() (objectMapper: FinatraObjectMapper) extends Controller {
    put("/events/:id") { request: EventHttp2 =>
        response.ok.json(request.id)
    }
 }

class EventFeatureTest extends FeatureTest with Mockito with HttpTest {

    override val server = new EmbeddedHttpServer(new Server)

    "Event update" in {
        var idEvent = 36

        val putData = ""

        val result = server.httpPutJson(
            path = "/events/" + idEvent,
            andExpect = Ok,
            putBody = idEvent.toString
        )
    }
}

I tried to change the test to:

class EventFeatureTest extends FeatureTest with Mockito with HttpTest {

    override val server = new EmbeddedHttpServer(new Server)

    "Event update" in {
        var idEvent = 36

        val putData = ""

        val result = server.httpPutJson(
            path = "/events/" + idEvent,
            andExpect = Ok,
            putBody = idEvent.toString,
            headers = Map("content-type" -> "application/json")
        )
    }
}

And the result was:

===========================================================================
HTTP PUT /events/53
[Header]    Content-Length -> 2
[Header]    Host -> 127.0.0.1:60860
53
===========================================================================
2015-09-26 00:52:51,474 ERR 9d6c77c99236baff          FinatraDefaultExceptionMapper internal server error
com.fasterxml.jackson.databind.JsonMappingException: Can not deserialize instance of com.localizables.domain.http.EventHttp2 out of VALUE_NUMBER_INT token
 at [Source: org.jboss.netty.buffer.ChannelBufferInputStream@19e1bfba; line: 1, column: 1]
    at com.fasterxml.jackson.databind.JsonMappingException.from(JsonMappingException.java:148) ~[jackson-databind-2.4.4.jar:2.4.4]
    at com.fasterxml.jackson.databind.DeserializationContext.mappingException(DeserializationContext.java:762) ~[jackson-databind-2.4.4.jar:2.4.4]
    at com.fasterxml.jackson.databind.DeserializationContext.mappingException(DeserializationContext.java:758) ~[jackson-databind-2.4.4.jar:2.4.4]
    at com.twitter.finatra.json.internal.caseclass.jackson.FinatraCaseClassDeserializer.incrementParserToFirstField(FinatraCaseClassDeserializer.scala:96) ~[finatra-jackson_2.11-2.0.1.jar:2.0.1]
    at com.twitter.finatra.json.internal.caseclass.jackson.FinatraCaseClassDeserializer.deserializeNonWrapperClass(FinatraCaseClassDeserializer.scala:81) ~[finatra-jackson_2.11-2.0.1.jar:2.0.1]
    at com.twitter.finatra.json.internal.caseclass.jackson.FinatraCaseClassDeserializer.deserialize(FinatraCaseClassDeserializer.scala:65) ~[finatra-jackson_2.11-2.0.1.jar:2.0.1]
    at com.fasterxml.jackson.databind.ObjectReader._bindAndClose(ObjectReader.java:1269) ~[jackson-databind-2.4.4.jar:2.4.4]
    at com.fasterxml.jackson.databind.ObjectReader.readValue(ObjectReader.java:864) ~[jackson-databind-2.4.4.jar:2.4.4]

The stacktrace is bigger but this portion could give an idea of what happend.

This is an issue or I am doing something wrong??

@scosenza

This comment has been minimized.

Contributor

scosenza commented Sep 26, 2015

httpPutJson has a bug where it doesn't set the content-type to application/json. The bug fix is in the following PR which we'll be merging next week: #228

The correct workaround is to manually set the content-type header as you've done above. However, you'll also need the putBody to contain a valid JSON object or array. In your last test, putBody="36" which is not a valid JSON object or array. This is why the error indicates it can't deserialize EventHttp2 out of a VALUE_NUMBER_INT.

Sorry for the inconvenience, and I'll close this issue once the bug fix is merged. Thanks for reporting the issue!

@scosenza

This comment has been minimized.

Contributor

scosenza commented Sep 26, 2015

It looks like we also have a bug when custom case-class requests are used with PUTs of non-JSON content.
The workaround is to use a finagle Request and manually parse the route-param and request content, e.g.

put("/events/:id") { request: Request =>
  val id = request.getLongParam("id")
  val content = request.contentString
  ...  
}

Thanks for the report, and I'll also promptly fix this issue.

@frossi85

This comment has been minimized.

frossi85 commented Sep 26, 2015

Thanks to you for the fast and clear response. Only one question: when is will be released in an stable build? How could I use master in the sbt dependency?

@frossi85

This comment has been minimized.

frossi85 commented Sep 26, 2015

Another simple test that is failing:

@Singleton
class EventsController @Inject() (objectMapper: FinatraObjectMapper) extends Controller {

    put("/events/:id") { request: Request =>
        response.ok.json("""
                  {
                    "id": "35"
                  }
                         """)
    }
 }


class EventFeatureTest extends FeatureTest with Mockito with HttpTest {

    override val server = new EmbeddedHttpServer(new Server)

    "Event update" in {
        val putData = """
                  {
                    "id": "35"
                  }
                     """

        val result = server.httpPutJson(
            path = "/events/" + idEvent,
            putBody = putData,
            andExpect = Ok,
            headers = Map("content-type" -> "application/json")
        )
    }
}

One doubt, would not be more consistent use withJsonBody instead of putBody?

@scosenza

This comment has been minimized.

Contributor

scosenza commented Sep 28, 2015

httpPutJson is meant to be used when the request and response both contain json. As such, httpPutJson needs a type param specified which matches the JSON response to be parsed, e.g.

val eventsResponse = server.httpPutJson[EventsResponse](
  path = "/events/" + idEvent,
  putBody = putData,
  ...

I'll improve the error message to indicate that a type-param should always be used with the server.httpJson methods.

This week we'll release 2.0.2 with the fix for non-JSON PUT's.

To build and use master run ./sbt publish-local from the finatra root directory

@frossi85

This comment has been minimized.

frossi85 commented Sep 28, 2015

Thanks for the reply. My mistake was that I am using server.httpPost not server.httpPostJson and trying to use the put json methond in the same way lol :P

scosenza added a commit that referenced this issue Sep 28, 2015

finatra-http: Custom request support for non-JSON and non-form-encode…
…d content

Problem
Custom case-class requests could only be used with JSON and form-urlencoded content

Solution
Allow any request content-type to be parsed with a custom case-class. Note: To access the raw request content, @Inject the finagle-httpx Request into your case-class (e.g. case class MyCaseClass(@RouteParam id: String, @Inject request: Request)

Thanks @frossi85 for reporting the issue: #257

RB_ID=748131
@frossi85

This comment has been minimized.

frossi85 commented Sep 29, 2015

@scosenza there is a guide to know how to contribute to the project and some documentation that introduce me to the code or arquitecture??

Injecting the request directly in the custom request case clase is a workaround or the final solution?? Does not seems much elegant. Is only an apretiation but I do not know the base code to say that there are better solutions.

I hope that I can help more, and thanks for this great framework :)

@cacoco

This comment has been minimized.

Member

cacoco commented Oct 1, 2015

Closed with commit: ee95a72

@cacoco cacoco closed this Oct 1, 2015

@scosenza

This comment has been minimized.

Contributor

scosenza commented Oct 1, 2015

Hi @frossi85,

A more elegant solution for dealing with non-json/non-form-urlencoded requests would involve the creation of a @RequestBody annotation. We'd be happy to accept a PR which adds this feature and you can find our contributing guide here. See the following method for where this new annotation would be handled: https://github.com/twitter/finatra/blob/master/http/src/main/scala/com/twitter/finatra/http/internal/marshalling/RequestInjectableValues.scala#L37

Thanks!
Steve

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