search

koltyakov / gosip

⚡️ SharePoint SDK for Go
https://go.spflow.com
MIT License
140 stars 32 forks source link

support ctx in GetAuth functions #68

Open OmerNinburg-RecoLabs opened 7 months ago

OmerNinburg-RecoLabs commented 7 months ago

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
OmerNinburg-RecoLabs commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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