matanshukry / flutter_google_places_sdk

Flutter plugin for google places native sdk
36 stars 74 forks source link

Question: Does this package populate session tokens automatically? #31

Open abdimussa87 opened 1 year ago

abdimussa87 commented 1 year ago

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?

matanshukry commented 1 year ago

@abdimussa87 As per the documentation, you can provide a session token which will be used.

Any request will save the session token (in memory). For any request after that, if you do not provide a token, the previous one that was obtained will be used.

abdimussa87 commented 1 year ago

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()?

matanshukry commented 1 year ago

@abdimussa87 right, my mistake - apologizes.

if set to true - it will create a new token. If set to false or null - it will only create a new token on the 1st time. After that, will use the previous token.

Generally speaking there's no reason for you to touch it. And yes - the session token is shared across all methods.

abdimussa87 commented 1 year ago

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

matanshukry commented 1 year ago

@abdimussa87 🤔 I doubt you'll be able to make it so requests won't be billed. Anyway feel free to give links on managing session tokens you mentioned, on any proposal on how it should be done/exposed.

abdimussa87 commented 1 year ago

Here is a link https://developers.google.com/maps/documentation/places/web-service/session-tokens And the important takeaways: We recommend the following guidelines:

Use session tokens for all autocomplete sessions. Generate a fresh token for each session. Ensure that the API key(s) used for all Place Autocomplete and Place Details requests within a session belong to the same Google Cloud Console project. Be sure to pass a unique session token for each new session. Using the same token for more than one session will result in each request being billed individually. You can optionally omit the autocomplete session token from a request. If the session token is omitted, each request is billed separately, triggering the Autocomplete - Per Request SKU. If you reuse a session token, the session is considered invalid and the requests are charged as if no session token was provided.

matanshukry commented 1 year ago

I have to admit the documentation is a bit confusing to me. But if I'm guessing and assuming some of these parts, I think the bottom line of what one should implement is:

@abdimussa87 Correct me if I'm wrong here.

abdimussa87 commented 1 year ago

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

mrcsilverfox commented 1 year ago

I have to admit the documentation is a bit confusing to me. But if I'm guessing and assuming some of these parts, I think the bottom line of what one should implement is:

  • Note: keep in mind on every request we make, where we don't provide a token, we get one from the response. iirc, if we do provide a token, we get the same one back.
  • [x] On every response, keep the token we got back.
  • [x] On every search request, use the previous token (if we have one)
  • [x] On every place detail request, use the previous token
  • [ ] On every place detail response , discard the token so it will no longer be used.
  • [x] What about other requests? unclear.

@abdimussa87 Correct me if I'm wrong here.

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: Let's take autocomplete as an example. When the session starts, we need to create a token, this token is valid for 2:30 minutes or 3 minutes, or until the user makes a place detail request. After that, the session is counted as one. From the documentation:

Once a session has concluded, the token is no longer valid; your app must generate a fresh token for each session. If the session token parameter is omitted, or if you reuse a session token, the session is charged as if no session token was provided (each request is billed separately).

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?

abdimussa87 commented 1 year ago

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?

mrcsilverfox commented 1 year ago

@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. However, it is a good implementation. I don't remember where I have read this, but I use this logic to force reset the token.

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.

abdimussa87 commented 1 year ago

So does this package currently support the ability to provide tokens or does it do it behind the scenes?

mrcsilverfox commented 1 year ago

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.

mrcsilverfox commented 1 year ago

I opened an issue about session token on web platform #43.

AndreiMisiukevich commented 1 year ago

@abdimussa87 I've submitted a PR for http SDK https://github.com/matanshukry/flutter_google_places_sdk/pull/46

According to documentation, it seems we have to force to reset last session token once getPlace call is made because it means that the session is complete.

image

That's why I think it can be a correct way to get session token for getSuggestions

    final sessionToken = (newSessionToken ?? false) || _lastSessionToken == null
        ? _lastSessionToken = const Uuid().v4()
        : _lastSessionToken;

And this -- for getPlace

    final sessionToken = (newSessionToken ?? false) ? null : _lastSessionToken;
    _lastSessionToken = null;

Do you know if I misunderstood something?

abdimussa87 commented 1 year ago

@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

mrcsilverfox commented 1 year ago

@AndreiMisiukevich Yes, you are right. But the caller is in charge of reset the token. So passing newSessionToken equal to true, the new autocomplete and get place request will use a new token. The source of truth is user-side, not from the plugin itself. I think it is this logic that @matanshukry wanted to use.

matanshukry commented 1 year ago

@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. I'm thinking on having a union class as argument on each method with the following values:

@Freezed()
class SessionTokenMode with _$SessionTokenMode
{
  cons factory SessionTokenMode._();

  /// This will pass the given sessionToken.
  const factory SessionTokenMode.manual({String? sessionToken}) = _SessionTokenModeManual;

  /// This will let the library choose when to create a new session token and when to use an existing one.
  const factory SessionTokenMode.auto() = _SessionTokenModeAuto;

  /// This will use the last sessionToken, if exists.
  ///  If the last token is null and [createIfNull] is true - it will create it and use it.
  ///  If the last token is null and [createIfNull] is false - it will use null.
  const factory SessionTokenMode.useLastToken({ required bool createIfNull }) = _SessionTokenModeLast;

  /// Get the last token bsaed on the curent mode
  String? getSessionToken(bool isPlaceDetailsRequest, String? lastToken) => map(
    manual: (manual) => manual.sessionToken,

    /// if last token is empty - create new one.
    /// if [isPlaceDetailsRequest] is true - 
    auto: (auto) => auto.
    useLastToken: (useLastToken) => (lastToken != null) ? lastToken :
      (useLastToken.createIfNull ? (const Uuid().v4()) : (null)),
  );
}

String? _handleSessionToken(bool isPlaceDetailsRequest) {
  // SessionTokenMode _sessionTokenMode;
  // String? _lastToken;

  final chosenToken = _sessionTokenMode.map(
    manual: (manual) => manual.sessionToken,
    auto: (auto) => (_lastToken != null) ? _lastToken : (const Uuid().v4()),
    useLastToken: (useLastToken) => (_lastToken != null) ? _lastToken :
      (useLastToken.createIfNull ? (const Uuid().v4()) : (null)),
  );

  // Update the value of the last token
  _lastToken = _sessionTokenMode.maybeMap(
    /// In auto mode we clean the token when it's a place request
    auto: (auto) =>isPlaceDetailsRequest ? null : chosenToken,

    // in other cases, use the token we chose
    orElse: () => chosenToken,
  );

  // return the chosen token
  return chosenToken;
}

What do you guys think?

AndreiMisiukevich commented 1 year ago

@mrcsilverfox I don't think so. It will be highly inconvenient to use this library. As a user, I want this library to work in the most cost-efficient way from the box.

In your scenario, the token will expire with the first call of getPlace, and I, as a user, will have to tell the library that I need to reset the token with the next call of autocomplete. It means I, as a user, will be forced to store the variable saying whether it's the first call of autocomplete after getPlace or not what is extremely inconvenient.

AndreiMisiukevich commented 1 year ago

To be honest, I don't quite understand why someone might want to make this library work in a cost-inefficient way. :) I am sure no one ever wants burning dollars just for fun.

Can anyone give me a use-case when the developer will pass newSessionToken true?

newSessionToken parameter is useless from my perspective.

String? sessionToken would certainly make sense (this is the actual control over the library), but newSessionToken doesn't (For cases when a user works with a few address search boxes at the same time.

matanshukry commented 1 year ago

@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". Look at it as giving freedom to the users to do as they will.

What if the spec changes? what if the user has a special agreement with the 3rd party service? etc etc. Libraries should help their users, while making sure not to limit them and give them the freedom to do as they will.

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.

AndreiMisiukevich commented 1 year ago

Libraries should help their users, while making sure not to limit them and give them the freedom to do as they will.

@matanshukry well, then the library should allow passing own String? sessionToken, shouldn't it?

Still can't find any example when someone needs to pass newSessionToken: true

matanshukry commented 1 year ago

@AndreiMisiukevich taking my example I'm assuming you mean useLastToken. it's just more convinent than saving the last token than passing it back as sessionToken. Utility method, that's all.

mrcsilverfox commented 1 year ago

Libraries should help their users, while making sure not to limit them and give them the freedom to do as they will.

@matanshukry well, then the library should allow passing own String? sessionToken, shouldn't it?

Still can't find any example when someone needs to pass newSessionToken: true

Imagine that you a have your own place_provider that has a var bool? _shouldResetSessionToken;

// A method to request a new token
void _refreshSession() {
    _shouldResetSessionToken = true;
    _logger.v('Google Places API provider session refreshed.');
  }

/// Checks and refreshes the current Place Autocomplete session if needed.
  void _checkSession() {
    if (_shouldResetSessionToken == null) {
      _logger.i(
        'This is the first $GooglePlacesApiProvider session. '
        'A new session token will be generated...',
      );
      _refreshSession();
    } 
  }

  // A method to fetch predictions 
Future<List<PlaceAutocompletePrediction>> fetchPredictions(String query) async {
    _checkSession();
    try {
      final response = await _client.findAutocompletePredictions(
        query,
        newSessionToken: _shouldResetSessionToken,
      );
      _shouldResetSessionToken = false;
      return response.predictions
          .map((p) => PlaceAutocompletePrediction.fromSdkModel(p))
          .toList();
    } catch (e, s) {
      ...
    }
  }

  Future<GoogleMapsPlace> getPlaceDetails(String placeId) async {
    _checkSession();
    final placeFields = GoogleMapsPlaceField.values;
    _logger.v('API ——> fetch place details of place with id $placeId');
    try {
      final response = await _client.fetchPlace(placeId, fields: placeFields);
      _refreshSession();
      if (response.place == null) {
        ..
      }
      return response.place!;
    } catch (e, s) {
      ...
    }
  }

If the fetchPlace is call the token must be reset. The fetch is always called with the last session token, if this is no longer valid the request is billed individually. (e.g. is the same as generate a new session token to fetch the place).

abdimussa87 commented 1 year ago

What is the status for this issue?

abdimussa87 commented 1 year ago

@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()?

mrcsilverfox commented 1 year ago

@abdimussa87 I think the idea is: