fix(supabase): get jwt on http call

[Vinzent03]

What is the new behavior?

I'm now getting the latest access token when the actual request is done by wrapping the http client with another client, which gets the newest from gotrue and sets the headers correctly.

Additionally I made expiresAt a late variable to avoid reparsing the jwt each time. And I added expired, because I think its a nice addition to easy check the validity of a session. But I'm not sure if some additional padding could be useful. So that a session is maybe 5 seconds before actual expiration, marked as expired.

Additional context

That technique is inspired from supabase-js

[dshukertjr]

This is brilliant!! I attempted to do what the js client library was doing, but I couldn't find a way to do it without breaking change, but here you are to save the day again! I am loving every bit of the code in this PR! Thank you so much for the amazing work!

[Vinzent03]

Thanks for the kind words 😊
Updated to isExpired and added the buffer.
One thing though that came to my mind are race conditions. If the session is expired and one does Future.wait(<listOfFutures>) all, for example postgrest, calls are done "simultaneously". I think this may make the session refresh for each request. Should there be some catching for this or am I missing something?

[dshukertjr]

Thanks for the kind words 😊
Updated to isExpired and added the buffer.
One thing though that came to my mind are race conditions. If the session is expired and one does Future.wait() all, for example postgrest, calls are done "simultaneously". I think this may make the session refresh for each request. Should there be some catching for this or am I missing something?

Yeah this PR is a bit scary to publish without a more robust integration tests, ins't it? I know that gotrue server has a mechanism to allow token refresh using the same access token for a short period of time (I believe it was 10 seconds) to allow simultaneous token refreshes to happen.

Either way, I was thinking about creating some additional tests within the supabase_flutter package that launches a local Supabase instance using the Supabase Github actions and tests basic use cases of all the Supabase components like postgrest or storage from a Flutter app. Then we could include a test case where we try to sending bunch of requests using Future.await and see if it works properly. What do you think about this?

[Vinzent03]

this PR is a bit scary to publish without a more robust integration tests, ins't it?

Absolutely haha
You are right about the refresh token interval, but that's configurable, so I definitely wouldn't depend on that.

I understand the idea behind making more complete flutter tests, but I don't know if we would really gain new use cases. I would rather build more robust and complete tests in the sub libraries. My point of concurrent refreshes could be tested by expanding the existing test in supabase. I will probably do that tomorrow.

The only thing that comes to my mind that changes in flutter environment vs pure dart is the issue of an app going to background or network issues, but these are hard to test.

[dshukertjr]

The only thing that comes to my mind that changes in flutter environment vs pure dart is the issue of an app going to background or network issues, but these are hard to test.

Probably testing realtime would be easier too, I think. But yeah maybe not a huge gain overall as you say.

[Vinzent03]

My guess was indeed the case and I've added a global completer now to combine these token refresh requests. When trying to implement that, I removed all the other completer that were used for the retry logic. All tests are passing and I think everything is fine. The changes of this pr are more significant than expected, though. I think the code of the retry logic is cleaner as well.

[dshukertjr]

Would removing completeError cause any errors to not be caught that used to be able to be caught before this update?

[dshukertjr]

I think developers can listen to the errors with onStateChanged, so I'm not too concerned, but thought I would flag it with you!

[Vinzent03]

I think actually the opposite is the case. Previously a completer got created in _saveSession causing any errors to get ignored. Now these errors dont get catched. Will investigate and might add further tests. These changes are really though...

[Vinzent03]

I've added a new commit to catch any errors from _callRefreshToken in _saveSesison and _setTokenRefreshTimer, because these should already be added to the stream and be handled there. Testing this with a test is very complicated, because all these methods are private and I would have to create a cusomt http client to mock some responses.

[dshukertjr]

Thank you so much for all the hard work!

[dshukertjr]

I wouldn't mind making things public with the internal annotation like you did with another method or property the other day if that makes testing things easier.

[Vinzent03]

I don't think that extra effort is neccessary. I'm very confident the errors are properly catched. _saveSession and _setTokenRefreshTimer are the only ones calling _callRefreshToken, which are not directly called by the user, so I'm pretty sure that's correct now.

[dshukertjr]

Looks great to me. If you are ready to merge it, let's merge it and we can publish new versions. .