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

support ctx in GetAuth functions #68

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

Conversation

OmerNinburg-RecoLabs
Copy link

This PR has 2 main goals

  1. add the http request ctx to the GetAuth functions - this is needed as currently the GetAuth functions are running with the default context.Background ctx.
  2. pass the ctx from the initiating function instead of the api struct configurations - this allows having different contexts for different api calls on top of the same api instance

this allows us to have a user defined ctx in the auth flow
this allows the ctx to traval down to functions chain and be passed to
every sub http request
@OmerNinburg-RecoLabs OmerNinburg-RecoLabs changed the title Original pkg name support ctx in GetAuth functions Jan 22, 2024
@OmerNinburg-RecoLabs
Copy link
Author

Hi @koltyakov ,
I would appreciate you considering Merging this PR.
If you would like any changes in the PR - please comment so I will know.

@koltyakov
Copy link
Owner

Hi @OmerNinburg-RecoLabs ,

Thank you for an improvement.

I'm still thinking what to do with it. Please give me more time.

The reason I can't merge it now is that it introduces braking changes.

For the #2, currently, you can pass context to any hierarchy for a group or an individual call. Yet, throug .Conf method - such a mechanism was done for avoiding braking changes.

Context for GetAuth is a bit more tricky, yet maybe little consumers use it directly, so the interface braking change effect is not huge.

Anyways, I'm thinking how to resolve it.

Out of interest, are you facing an actual need in request cancelation or this is a theoretical thing?

@OmerNinburg-RecoLabs
Copy link
Author

OmerNinburg-RecoLabs commented Jan 24, 2024

Hi @koltyakov ,
This is not theoretical - in my use case I have an http middleware that writes all the raw http response to kafka.
This middleware uses some values that are injected to the ctx via the context.WithValue function.

Due to the fact the GetAuth function does not get the initiating api call ctx, my middleware is failing.
After thinking about it, I understood the best option for me is passing the ctx as I do not want my middleware to actively know the originator of the http request.

Regarding the ctx in the API config - I used to use it but I think it's a weird way to pass the ctx and it is not aligned to the common use of ctx in other pkgs (including the go core) - nevertheless, this is more syntactic sugar so I don't mind this feature being discarded

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