-
Notifications
You must be signed in to change notification settings - Fork 82
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
Question: Does this package populate session tokens automatically? #31
Comments
@abdimussa87 Any request will save the session token (in memory). |
The findAutocompletePredictions() takes a newSessionToken boolean. Does it autogenerate the token if set to true? And when should I set that to true? Also I'll be calling fetchPlace(placeId) when a user clicks one of the predictions result returned from findAutocompletePredictions(), but I don't see a session token field for that method. Does it automatically use the one from findAutocompletePredictions()? |
@abdimussa87 right, my mistake - apologizes. if set to true - it will create a new token. Generally speaking there's no reason for you to touch it. |
I read about how one should manage session tokens and if it's reused it'll just be discarded and there won't be session control, basically each request gets billed. Not the best outcome |
@abdimussa87 🤔 I doubt you'll be able to make it so requests won't be billed. |
Here is a link https://developers.google.com/maps/documentation/places/web-service/session-tokens Use session tokens for all autocomplete sessions. |
I have to admit the documentation is a bit confusing to me.
@abdimussa87 Correct me if I'm wrong here. |
I don't know if it does return a token, but basically the token will be used for each autocomplete request till a user selects a place and a subsequent place detail request is done. This will count as one session and billed accordingly(which makes the bill for the autocomplete to be free and the place detail to be charged). Then a new session token is required for any other autocomplete and/or place detail request. And I think this sessions requirement is for autocomplete requests which can lead to a subsequent place detail request |
Hi @abdimussa87, I read your native implementation for iOS and I think the one for Android is the same and I think there is a problem with the token. I read the google place api documentation and this is what I understand:
I think your implementation is correct for the first autocomplete query, but subsequent queries are billed separately because the token is invalid and was never generated. Therefore, I suggest handling the token as follows:
What are your thoughts on this? Would you rather have the user choose whether to force the generation of a new token? |
That is good implementation, but where did you get the info about the threshold of 2-3 minutes. And I think having an option for the user to choose to force generate new token isn't bad. And by the way, which native iOS implementation are you talking about, as I didn't implement that? |
@abdimussa87 Yes, having an option that allows the user to choose to force generate a new token is not bad. But if the user forgets to reset the token, the plugin does nothing to handle this issue. private func getSessionToken(force: Bool) -> GMSAutocompleteSessionToken! {
let localToken = lastSessionToken
if (force || localToken == nil) {
return GMSAutocompleteSessionToken.init()
}
return localToken
} There is no token validation logic, that's what I meant. But if the user is in charge of managing it, there is no problem. |
So does this package currently support the ability to provide tokens or does it do it behind the scenes? |
No, this package supports the ability to force the generation of a new token. If you mishandle the logic of the request to force a new token, you may encounter incorrect billing. |
I opened an issue about session token on web platform #43. |
@abdimussa87 I've submitted a PR for http SDK According to documentation, it seems we have to force to reset That's why I think it can be a correct way to get session token for final sessionToken = (newSessionToken ?? false) || _lastSessionToken == null
? _lastSessionToken = const Uuid().v4()
: _lastSessionToken; And this -- for final sessionToken = (newSessionToken ?? false) ? null : _lastSessionToken;
_lastSessionToken = null; Do you know if I misunderstood something? |
@AndreiMisiukevich I think that seems to be correct. I wonder if we can know whether this works or not based on the result that is returned |
@AndreiMisiukevich Yes, you are right. But the caller is in charge of reset the token. So passing |
@mrcsilverfox the logic I usually try to follow is give user full control IF he wants to, but by default do what we think is best.
What do you guys think? |
@mrcsilverfox I don't think so. In your scenario, the token will expire with the first call of |
To be honest, I don't quite understand why someone might want to make this library work in a cost-inefficient way. :) Can anyone give me a use-case when the developer will pass
|
@AndreiMisiukevich the library is talking against a 3rd party service. Things change overtime, and maintainers don't always stay. Don't look it as "work in a cost-inefficient way". What if the spec changes? what if the user has a special agreement with the 3rd party service? etc etc. hence default to do as they think is best at the moment they are written, while flexible enough to allow the user to override if desired. |
@matanshukry well, then the library should allow passing own Still can't find any example when someone needs to pass |
@AndreiMisiukevich taking my example I'm assuming you mean |
Imagine that you a have your own
If the |
What is the status for this issue? |
@mrcsilverfox I see in your example the call to the fetchPlace() doesn't have the option to pass in a token? How do you suppose it'll use the token used to perform findAutocompletePredictions()? |
@abdimussa87 I think the idea is:
|
Does this package populate session tokens automatically or do we need to provide one each time we make a request say to findAutocompletePredictions and fetchPlace?
The text was updated successfully, but these errors were encountered: