Skip to content

Commit

Permalink
Reduce the access token refresh interval rate. Based on https://suppo…
Browse files Browse the repository at this point in the history 
…rt.exactonline.com/community/s/knowledge-base#All-All-DNO-Content-oauth-eol-oauth-devstep3
  • Loading branch information
Freek Vandeursen committed May 25, 2021
1 parent 4669343 commit 166aaa5
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/Picqer/Financials/Exact/Connection.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ private function tokenHasExpired()
return true;
}

return ($this->tokenExpires - 60) < time();
return ($this->tokenExpires - 10) < time();
}

private function formatUrl($endPoint, $includeDivision = true, $formatNextUrl = false)
Expand Down

6 comments on commit 166aaa5

@TWSnijders
Copy link

Choose a reason for hiding this comment

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

Why is it that 10 (seconds) is subtracted from $this->tokenExpires ?

Exact gives a value of 600 (seconds / 10min), why subtract 10 seconds from that? Wouldn't this result in requesting a new token too early?

@casperbakker
Copy link
Member

Choose a reason for hiding this comment

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

@TWSnijders I don't know for this specific case, but we do that in a lot of our clients to prevent calls that will fail. So we will try to refresh an expiring token a bit sooner, and with rate limits we will hold off a bit earlier. We think that is being a good web citizen. If you know that the token will almost expire, it is better to refresh it already then to wait for a 400 response.

It could also be 2 or 5 seconds, but in this case it is 10 seconds. The specific implementation does not matter that much.

@TWSnijders
Copy link

@TWSnijders TWSnijders commented on 166aaa5 May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@TWSnijders I don't know for this specific case, but we do that in a lot of our clients to prevent calls that will fail. So we will try to refresh an expiring token a bit sooner, and with rate limits we will hold off a bit earlier. We think that is being a good web citizen. If you know that the token will almost expire, it is better to refresh it already then to wait for a 400 response.

It could also be 2 or 5 seconds, but in this case it is 10 seconds. The specific implementation does not matter that much.

@casperbakker Problem we have is that we get a message that the token is not expired yet, so we are not getting a new token.

If that 10 seconds was not there, I would expect we would not get that error anymore.

@casperbakker
Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a bad implementation on Exact's side. Time can be out of sync a little bit. So than you really need to wait for it to expire, and only then refresh it.

I don't know much about this library, but I don't think this is the problem as this code is used in a lot of (big) projects, and this code is there for years.

@TWSnijders
Copy link

Choose a reason for hiding this comment

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

That sounds like a bad implementation on Exact's side. Time can be out of sync a little bit. So than you really need to wait for it to expire, and only then refresh it.

I don't know much about this library, but I don't think this is the problem as this code is used in a lot of (big) projects, and this code is there for years.

It being used by many (big) companies and it being there for a long time, does not make it right.

This library makes a connection with the connect() function which calls checkOrAcquireAccessToken() which in turn checks tokenHasExpired().

Because of that -10 seconds, the code tells us the token is not expired yet (but has) and we then try to make a new connection with that (expired) token, which then gives us an error (from Exact) that the token is not expired yet.

So if all these checks are already done, I don't see a reason as to why one would subtract that 10 seconds from the token expire time, when this is checked before setting up the connection.

@casperbakker
Copy link
Member

Choose a reason for hiding this comment

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

It being used by many (big) companies and it being there for a long time, does not make it right.

What I meant is that I doubt this 10 seconds offset is the reason for your error. As we would have seen this error many times before and then it would be fixed.

But as I said, I am not very into this library, so I cannot look into this further. You can try create an issue and see if someone else knows what the problem is.

Please sign in to comment.