Skip to content
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

Allow Tesla middleware to be configured #42

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GrantJamesPowell
Copy link

To provide support for custom middleware, allow the
ExForce.Client.Tesla.build_client/2 method to take an additional param
in it's opts argument defining middleware to run.

The choice was made to allow users to provide their own instrumentation

The middleware runs before the compression/json encoding and after the
API version headers

To provide support for custom middleware, allow the
`ExForce.Client.Tesla.build_client/2` method to take an additional param
in it's opts argument defining middleware to run.

The choice was made to allow users to provide their own instrumentation

The middleware runs _before_ the compression/json encoding and _after_ the
API version headers
@sourcelevel-bot
Copy link

Hello, @GrantJamesPowell! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

end)

client =
ExForce.Client.Tesla.build_client(%{instance_url: bypass_url(bypass), access_token: "foo"},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested modules could be aliased at the top of the invoking module.

@sourcelevel-bot
Copy link

SourceLevel has finished reviewing this Pull Request and has found:

  • 1 possible new issue (including those that may have been commented here).

See more details about this review.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7ffa9c4 on SalesLoft:sl__configurable-tesla-middleware into db7792f on chulkilee:master.

@chulkilee
Copy link
Owner

@GrantJamesPowell thanks for the PR!

I love instrumentation! However, I'm not convinced to choose specific position for any custom middleware hm.

Another approach would be: we add direct integration for Telemetry since Telemetry is becoming a standard instrumentation mechanism in Elixir world, and Telsa has built-in middleware for this - Tesla.Middleware.Telemetry. For example, the function may take telemetry_metadata, and add the Telemetry middleware when it's present.

@GrantJamesPowell
Copy link
Author

@chulkilee, I agree a dedicated telemetry integration could provide value. That might be something our team would be willing contribute if you're interested.

As far as custom middle ware, as we iterated further we found several use cases within our SF sync for custom middleware, our auth flow/rate limiting/reporting is pretty nuanced and we're using custom middle ware to handle it. We're currently using this fork in production.

If you're interested in support for custom middleware in the upstream, we're more than happy to rework this patch to bring it more inline with the vision.

Thanks again for open sourcing this, we appreciate it!

@chulkilee
Copy link
Owner

@GrantJamesPowell thanks a lot for giving more details of your use case. I'm not using this library anymore 😉 but I'm happy to make reasonable changes as needed!

Since ExForce client is a just tesla client, you may use the new Tesla features to get the middleware - elixir-tesla/tesla#373

See this fully working example:

%Tesla.Client{
  adapter: {Tesla.Adapter.Hackney, :call, [[]]},
  fun: nil,
  post: [],
  pre: [
    {ExForce.Client.Tesla.Middleware, :call, [{"http://example.com", "42.0"}]},
    {Tesla.Middleware.Compression, :call, [[format: "gzip"]]},
    {Tesla.Middleware.JSON, :call, [[engine: Jason]]},
    {Tesla.Middleware.Headers, :call, [[{"authorization", "Bearer mytoken"}]]}
  ]
} =
  client =
  ExForce.build_client(%{instance_url: "http://example.com", access_token: "mytoken"},
    adapter: Tesla.Adapter.Hackney
  )

[{ExForce.Client.Tesla.Middleware, _} = h | t] = Tesla.Client.middleware(client)
adapter = Tesla.Client.adapter(client)

%Tesla.Client{
  adapter: {Tesla.Adapter.Hackney, :call, [[]]},
  fun: nil,
  post: [],
  pre: [
    {ExForce.Client.Tesla.Middleware, :call, [{"http://example.com", "42.0"}]},
    {MyMiddleware, :call, [[]]},
    {Tesla.Middleware.Compression, :call, [[format: "gzip"]]},
    {Tesla.Middleware.JSON, :call, [[engine: Jason]]},
    {Tesla.Middleware.Headers, :call, [[{"authorization", "Bearer mytoken"}]]}
  ]
} = Tesla.client([h | [MyMiddleware | t]], adapter)

Could you test it out? If it works, then probably we can add it to documentation to ExForce.Client.Tesla as a tip, instead of changes in this PR.

@GrantJamesPowell
Copy link
Author

@chulkilee, great points/tip. Made a card for our team to try this out.

I agree that the suggestion above, to edit the built client, is a much better idea.

Will close this PR when we move off of the fork

@ggiill
Copy link

ggiill commented Feb 22, 2023

I'm getting a timeout somewhere in Enforce.OAuth.get_token or ExForce.versions, and, since there is no ability to configure the middleware in ExForce.Client.Tesla.build_oauth_client, I can't trace where that timeout is coming from.

+1 to allowing global configuration of Tesla middleware in all ExForce clients. (Or, at least adding Tesla.Middleware.Telemetry by default.)

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.

4 participants