Skip to content

Conversation

egoodhall
Copy link
Contributor

@egoodhall egoodhall commented Mar 26, 2025

Fixes

Adds support for providing a custom ObjectMapper to the rest client builders. If no ObjectMapper is provided to the builder, a default one which matches the existing configuration will be used. The default ObjectMapper for each rest client type will also now be held in a static variable, so we aren't instantiating a new one every time a rest client is created.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

@manisha1997
Copy link
Contributor

Hello!
Can I get the intended use case for this via an example?

@egoodhall
Copy link
Contributor Author

Hi! Sure - I'd like to be able to provide my own object mapper (a singleton instance, with some customization applied) while constructing a RestClient. Right now, it's not possible to provide my own, and additionally, a new object mapper gets created for each client. Ideally, I want to be able to do something like:

private static final ObjectMapper MY_CUSTOM_MAPPER = //...

//...

TwilioRestClient client = new TwilioRestClient
  .Builder(username, token)
  .objectMapper(MY_CUSTOM_MAPPER)
  //...
  .build()

Even without support for custom object mappers, having a single static default ObjectMapper would be a nice improvement. I currently use the SDK to make 10s-100s of requests per second across thousands of subaccounts, and the fact that I'm constructing a new ObjectMapper (an expensive operation) every time I create new a rest client (with auth for a different subaccount) isn't ideal.

@manisha1997
Copy link
Contributor

Thanks for the example. I understood what is your use case.
Yes, that is a lot of heap memory being wasted for something that will be standardised in the application.
The test case you added is failing. Can you fix that?

@egoodhall egoodhall force-pushed the eg/custom-object-mapper-support branch from 49cbac8 to 0ee9dc2 Compare May 8, 2025 18:39
@egoodhall
Copy link
Contributor Author

Hey @manisha1997 - I've updated the test, and it should be passing now!

@manisha1997
Copy link
Contributor

@egoodhall, I tested these changes. This will still create multiple mappers for each TwilioRestClient, so, the heap memory problem will still exist.
Added some docs.

@manisha1997 manisha1997 self-requested a review May 9, 2025 07:50
@egoodhall
Copy link
Contributor Author

Hey @manisha1997 - I'm not sure I follow. By default, it will use the DEFAULT_OBJECT_MAPPER, which is static in the builder, so there will only be one instance per rest client type (I didn't want to stray from the strict separation that exists in those today). A reference to the default object mapper gets passed around in the respective builders/clients, but it shouldn't be creating copies. This test verifies that it's referentially the same object.

It doesn't (and can't) verify that provided custom object mappers are the same instance, but I think if an override is provided for the object mapper, it's outside of the SDK's responsibilities to worry about that.

@manisha1997
Copy link
Contributor

Ohk. I was under the assumption that even the custom mappers will only be 1 throughout the Application.
This makes sense. Tested and merged.
Thanks for the PR! @egoodhall

@manisha1997 manisha1997 merged commit 0db5dd5 into twilio:main May 10, 2025
6 checks passed
@egoodhall egoodhall deleted the eg/custom-object-mapper-support branch May 12, 2025 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants