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

ObjectMapper reuse & config #126

Merged
merged 10 commits into from Oct 17, 2014

Conversation

4 participants
@Xorlev
Contributor

Xorlev commented Mar 21, 2014

Enables ObjectMapper reuse. ObjectMapper can be accessed & configured, with a convenience module registration method.

Additionally, added a per-request method which allows overriding the ObjectMapper for a specific request

It's inefficient to make new ObjectMappers with every request and incur annotation processing overhead etc.

I'm by no means a Scala expert, I'd love some feedback on where this could be taken. I'm especially interested in sharing an ObjectMapper to an entity decoding function.

ObjectMapper reuse & config
Enables ObjectMapper reuse through implicit injection. ObjectMapper can
be accessed & configured.

Additionally, added a per-request method which allows overriding the
ObjectMapper for a specific request
class AppService(controllers: ControllerCollection)
extends Service[FinagleRequest, FinagleResponse] with App with Logging {
def render: ResponseBuilder = new ResponseBuilder
def render: ResponseBuilder = new ResponseBuilder(new DefaultJacksonJsonSerializer)

This comment has been minimized.

@davemssavage

davemssavage Apr 2, 2014

I think DefaultJacksonSerializer should be an object in this patch vs a class otherwise we do not get the reuse of ObjectMapper across separate render calls

def serialize[T](item: T) = mapper.writeValueAsString(item).getBytes(UTF_8)
}
class DefaultJacksonJsonSerializer() extends JacksonJsonSerializer(

This comment has been minimized.

@davemssavage

davemssavage Apr 2, 2014

make this an object?

}
class ResponseBuilder {
class ResponseBuilder(serializer:JsonSerializer) {

This comment has been minimized.

@capotej

capotej Apr 6, 2014

Contributor

Why not pass in a default serializer like serializer: JsonSerializer = new DefaultJacksonJsonSerializer? That way you don't have to pass it in everywhere and it doesn't break compatibility.

This comment has been minimized.

@davemssavage

davemssavage Apr 6, 2014

Yep I like that idea too, also if DefaultJacksonJsonSerializer is an object this means we get reuse of the object mapper between responses

DefaultJacksonSerializer is now an object
Also, serializer in the controller is a settable var for setting the
default serializer without making a new render method.

@Xorlev Xorlev changed the title from [WIP] ObjectMapper reuse & config to ObjectMapper reuse & config May 24, 2014

@Xorlev

This comment has been minimized.

Contributor

Xorlev commented May 24, 2014

@davemssavage @capotej Sorry for the terrible delay. Made your recommended changes.

One thing I'm curious for your take on is the var serializer in the Controller, ostensibly so it could be overridden once in the controller and Just Works with render. If you don't like it, I can rip that out (this time without a 6-week delay :))

@mattweyant

This comment has been minimized.

mattweyant commented May 24, 2014

I had a similar requirement and I've now been using this internally for a few days, and it works great!

@capotej

This comment has been minimized.

Contributor

capotej commented May 27, 2014

Hey Xorlev, I still think this is a good idea, can you update this PR so it will merge? Thanks!

Merge remote-tracking branch 'twitter/master' into mapper-reuse
Conflicts:
	src/main/scala/com/twitter/finatra/Controller.scala
	src/main/scala/com/twitter/finatra/ResponseBuilder.scala
@Xorlev

This comment has been minimized.

Contributor

Xorlev commented May 28, 2014

@capotej Should be good to merge.

Update JsonSerializer.scala
Newline removal
@Xorlev

This comment has been minimized.

Contributor

Xorlev commented Jul 23, 2014

@capotej If this looks good to you, would it be possible to merge?

@Xorlev

This comment has been minimized.

Contributor

Xorlev commented Oct 9, 2014

@capotej Any remaining concerns here?

@capotej

This comment has been minimized.

Contributor

capotej commented Oct 15, 2014

Hey @Xorlev sorry again for the delay, can your remove the @author tags? Fix that and I'll happily merge.

@Xorlev

This comment has been minimized.

Contributor

Xorlev commented Oct 17, 2014

@capotej Done

capotej added a commit that referenced this pull request Oct 17, 2014

Merge pull request #126 from Xorlev/mapper-reuse
ObjectMapper reuse & config

@capotej capotej merged commit 57a87a0 into twitter:master Oct 17, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@capotej

This comment has been minimized.

Contributor

capotej commented Oct 17, 2014

Thanks! (re: author tags, you'll get added to THANKS as credit, don't worry)

@Xorlev

This comment has been minimized.

Contributor

Xorlev commented Oct 17, 2014

Thanks for merging! And no problem, I forgot they were there honestly, they're auto-genned by IntelliJ.

@davemssavage

This comment has been minimized.

davemssavage commented Oct 18, 2014

Good news, any idea when this will make it into a release?

Kind regards,

/Dave

On Friday, October 17, 2014, Michael Rose notifications@github.com wrote:

Thanks for merging! And no problem, I forgot they were there honestly,
they're auto-genned by IntelliJ.


Reply to this email directly or view it on GitHub
#126 (comment).

cacoco pushed a commit that referenced this pull request May 13, 2015

Merge pull request #126 from Xorlev/mapper-reuse
ObjectMapper reuse & config

cacoco pushed a commit that referenced this pull request May 14, 2015

Merge pull request #126 from Xorlev/mapper-reuse
ObjectMapper reuse & config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment