fix(fdc): issue where if path was empty on web, the app crashed

[maneesht]

Fixes firebase/firebase-tools#9073

[wer-mathurin]

@cynthiajoan , @yuchenshi @Lyokone
I saw this PR link to another ticket. My case is the app will crash if the errors key is missing in the JSON :-)

Also, I see other potential bugs in the code that need your attention and this can be solved by using the pattern matching otherwise we can have all other bunch of potential runtime exception.

instead of this:

List<DataConnectOperationFailureResponseErrorInfo> suberrors =
            handleErrors(bodyJson['errors']) <----- If the errors key is missing a runtime exception will be thrown (this is our case in another ticket)
                .map<DataConnectOperationFailureResponseErrorInfo>((e) =>
                    DataConnectOperationFailureResponseErrorInfo(
                        e.path != null
                            ? e.path!
                                .map((val) => val is String
                                    ? DataConnectFieldPathSegment(val)
                                    : DataConnectListIndexPathSegment(val))
                                .toList()
                            : [].cast<DataConnectPathSegment>(),
                        e.message))
                .toList();

In the handleError you have the same issue...you are assuming the keys in the json will be present

  List<JsonGraphqlResponseError> handleErrors(List<dynamic> errors) {
    return errors
        .map((eMap) => JsonGraphqlResponseError(eMap['message'], eMap['path']))
        .toList();
  }

Maybe in the JsonGraphqlResponseError you can change the variables to be nullable or throw an explicit error, because the error can be either a null runtime exception or a type cast exception!

Here is an example of how you can achieve the same without creating new methods and new class...so this will be more efficient because we do not need to instantiate a class to only pass data...Im not sure about the DataConnectRestContractException, but this in my opinion will help your team making sure the contract is respected with the backend and can be catch in your integration tests :-)

  <DataConnectOperationFailureResponseErrorInfo>[
    for (final {'path': List paths, 'message': String? message}
        in bodyJson['errors'] ?? [])
      DataConnectOperationFailureResponseErrorInfo(
        [
          for (final val in paths)
            switch (val) {
              String() => DataConnectFieldPathSegment(val),
              int() => DataConnectListIndexPathSegment(val),
              _ => throw DataConnectRestContractException(
                'The val must be either a String or an Int. The current val is  ${val.runtimeType}',
              ),
            },
        ],
        message ??
            (throw DataConnectRestContractException(
              'Message should contain the error',
            )),
      ),
  ];

[maneesht]

@wer-mathurin The errors field should always be sent. Can you give me an example where the errors field is not present in the JSON?

[maneesht]

@wer-mathurin we also do a check above that function to check whether the errors field is added:https://github.com/firebase/flutterfire/pull/17704/files#diff-3cec4d6d4680bc84d9864bd72e0fa5907325afe63b62f08cd9041d5a7656a56aR131

[wer-mathurin]

@wer-mathurin we also do a check above that function to check whether the errors field is added:https://github.com/firebase/flutterfire/pull/17704/files#diff-3cec4d6d4680bc84d9864bd72e0fa5907325afe63b62f08cd9041d5a7656a56aR131

In this case my suggestion is even more appealing, because you can remove the if....less indentations equal a more readable code :-). And I think using pattern matching over imbricated if statements is a cleaner approach.

@wer-mathurin The errors field should always be sent. Can you give me an example where the errors field is not present in the JSON?

This is why I'm suggesting to add a DataConnectRestContractException...this will shield the Dart SDK from contract change from the DataConnect backend...if they decided to change the response format...the SDK will fail to read those "always sent" keys in the JSON. But you need to have an Integration test for that.

[maneesht]

@wer-mathurin Thanks for your suggestion. Though I will say, the if block is more legible for those who aren't necessarily familiar with Dart. I'll take your comments under consideration

[wer-mathurin]

@wer-mathurin Thanks for your suggestion. Though I will say, the if block is more legible for those who aren't necessarily familiar with Dart. I'll take your comments under consideration

I was thinking that the the code in the Dart SDK is maintained or contributed was done by people familiar with the Dart language :-)

For the JSON validation and data extraction this is one of the use case documented by the Dart team : https://dart.dev/language/patterns#validating-incoming-json

Cheers,
Looking to see this PR merged whatever the way it's done, at least we are going forward.

[maneesht]

@wer-mathurin Updated the PR. The new changes should be similar to what you suggested