fix: the Response from DioError could return null from the data property but String is expected in ParseNetworkResponse #775

Nidal-Bakir · 2022-07-09T09:14:13Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to an issue.

Issue Description

The response from dio error object could have a null data property

Related issue: #774

Approach

Add null aware operator ??:

on dio.DioError catch (error) {
      return ParseNetworkByteResponse(
        data: error.response?.data ?? _fallbackErrorData,
        statusCode: error.response!.statusCode!,
      );
    }

TODOs before merging

All set

Add tests
Add changes to documentation (guides, repository pages, in-code descriptions)
A changelog entry

parse-github-assistant · 2022-07-09T09:14:24Z

Thanks for opening this pull request!

🎉 We are excited about your hands-on contribution!

…rom dio

mtrezza · 2022-07-09T19:26:17Z

Could you please replace the PR title with fix: <summary of what the issue is>; I assume the related issue is #775, I have added this to your PR description.

mtrezza

Could you add a test for this?

parse-github-assistant · 2022-07-10T12:33:28Z

I will reformat the title to use the proper commit message syntax.

Nidal-Bakir · 2022-07-10T13:10:09Z

Sure, but I can not find a way to mock the _ParseDioClient class to fake the DioError.
I was thinking of adding a typedef like this:

typedef ParseDioClientForMockTest = _ParseDioClient;

so I can create a mock for it and inject it in the constructor of ParseDioClient maybe like this:

  ParseDioClient({
      bool sendSessionId = false,
      SecurityContext? securityContext,
      _ParseDioClient? parseDioClientForMockTest,
      }) {
    _client = parseDioClientForMockTest ??
        _ParseDioClient(
          sendSessionId: sendSessionId,
          securityContext: securityContext,
        );
  }

But this requires the 'nonfunction-type-aliases' language feature to be enabled, so we need to set the minimum SDK constraint to 2.16.0 or higher.

Do you have any suitable way to do this?

…-Flutter into bug_fix#774

…K-Flutter into bug_fix#774

codecov · 2022-07-19T13:01:48Z

Codecov Report

Base: 10.26% // Head: 10.08% // Decreases project coverage by -0.18% ⚠️

Coverage data is based on head (2b3b05d) compared to base (14be107).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
- Coverage   10.26%   10.08%   -0.19%     
==========================================
  Files          49       47       -2     
  Lines        2815     2817       +2     
==========================================
- Hits          289      284       -5     
- Misses       2526     2533       +7

Impacted Files	Coverage Δ
...ackages/dart/lib/src/network/parse_dio_client.dart	`0.00% <0.00%> (ø)`
packages/dart/lib/src/network/options.dart	`0.00% <0.00%> (-50.00%)`	⬇️
...ackages/dart/lib/src/network/parse_live_query.dart	`0.00% <0.00%> (-0.92%)`	⬇️
packages/flutter/lib/parse_server_sdk.dart	`8.77% <0.00%> (ø)`
packages/dart/lib/src/objects/parse_file.dart	`0.00% <0.00%> (ø)`
packages/dart/lib/src/objects/parse_user.dart	`0.00% <0.00%> (ø)`
packages/dart/lib/src/objects/parse_file_web.dart	`0.00% <0.00%> (ø)`
packages/dart/lib/src/objects/parse_file_base.dart	`0.00% <0.00%> (ø)`
...ckages/dart/lib/src/network/parse_http_client.dart	`5.12% <0.00%> (ø)`
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

mtrezza · 2022-07-28T20:15:37Z

@parse-community/flutter-sdk-review what do you think, is this ready for merge?

…the values as String when decoding it.

mtrezza · 2022-10-23T09:31:18Z

@Nidal-Bakir any particular reason for closing this?

mtrezza · 2022-10-23T12:14:44Z

@parse-community/flutter-sdk-review should this PR be merged?

Nidal-Bakir · 2022-10-24T01:21:34Z

@mtrezza
I'm sorry, I did not mean to close the PR!
I did not know that if I deleted the forked repository from my account all my PR would be closed.

I was testing some things in my forked repository so I got to a point where I just needed to start from the beginning. so found it easier to just delete the repository and start all over again.

mtrezza · 2022-10-24T11:45:22Z

I've reopened, thanks for clarifying

mbfakourii · 2022-11-29T05:56:43Z

packages/dart/lib/src/network/parse_dio_client.dart

@@ -31,7 +31,9 @@ class ParseDioClient extends ParseClient {
          data: dioResponse.data!, statusCode: dioResponse.statusCode!);
    } on dio.DioError catch (error) {
      return ParseNetworkResponse(
-          data: error.response?.data, statusCode: error.response!.statusCode!);
+        data: error.response?.data ?? _fallbackErrorData,
+        statusCode: error.response!.statusCode!,


I ran this code without internet connection and got null error in statusCode!
I suggest you change the code like this

return ParseNetworkResponse( data: error.response?.data ?? _fallbackErrorData, statusCode: error.response?.statusCode! ?? ParseError.otherCause, );

Nidal-Bakir · 2023-02-24T19:31:30Z

Reopening in a different PR because I lost all the progress and the reference of these changes.
The new PR pull/825

fix bug#774: null is not a subtype of String

e17bb65

Nidal-Bakir changed the title ~~fix bug#774: null is not a subtype of String~~ fix: bug#774: null is not a subtype of String Jul 9, 2022

extract fallbackErrorData geter in case of null data error response f…

cad42c7

…rom dio

mtrezza linked an issue Jul 9, 2022 that may be closed by this pull request

Type 'Null' is not a subtype of type 'String' #774

Closed

4 tasks

mtrezza requested changes Jul 9, 2022

View reviewed changes

Nidal-Bakir changed the title ~~fix: bug#774: null is not a subtype of String~~ fix: The Response from DioError could return null from the data property but String is expected in ParseNetworkResponse Jul 10, 2022

Nidal-Bakir and others added 3 commits July 10, 2022 16:21

Merge branch 'master' of https://github.com/parse-community/Parse-SDK…

44da385

…-Flutter into bug_fix#774

Merge branch 'master' into bug_fix#774

a19cdc3

Merge branch 'bug_fix#774' of https://github.com/Nidal-Bakir/Parse-SD…

46eae42

…K-Flutter into bug_fix#774

Nidal-Bakir requested a review from mtrezza July 19, 2022 10:42

mtrezza requested review from a team and removed request for mtrezza July 19, 2022 12:56

fix json format, Add quotation mark to make sure the dart will parse …

2b3b05d

…the values as String when decoding it.

Nidal-Bakir closed this by deleting the head repository Oct 23, 2022

mtrezza reopened this Oct 24, 2022

mbfakourii reviewed Nov 29, 2022

View reviewed changes

Nidal-Bakir closed this Feb 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: the Response from DioError could return null from the data property but String is expected in ParseNetworkResponse #775

fix: the Response from DioError could return null from the data property but String is expected in ParseNetworkResponse #775

Nidal-Bakir commented Jul 9, 2022 •

edited by mtrezza

Loading

parse-github-assistant bot commented Jul 9, 2022 •

edited

Loading

mtrezza commented Jul 9, 2022 •

edited

Loading

mtrezza left a comment

parse-github-assistant bot commented Jul 10, 2022

Nidal-Bakir commented Jul 10, 2022 •

edited

Loading

codecov bot commented Jul 19, 2022 •

edited

Loading

mtrezza commented Jul 28, 2022

mtrezza commented Oct 23, 2022

mtrezza commented Oct 23, 2022

Nidal-Bakir commented Oct 24, 2022 •

edited

Loading

mtrezza commented Oct 24, 2022

mbfakourii Nov 29, 2022 •

edited

Loading

Nidal-Bakir commented Feb 24, 2023 •

edited

Loading

fix: the Response from DioError could return null from the data property but String is expected in ParseNetworkResponse #775

fix: the Response from DioError could return null from the data property but String is expected in ParseNetworkResponse #775

Conversation

Nidal-Bakir commented Jul 9, 2022 • edited by mtrezza Loading

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

parse-github-assistant bot commented Jul 9, 2022 • edited Loading

Thanks for opening this pull request!

mtrezza commented Jul 9, 2022 • edited Loading

mtrezza left a comment

Choose a reason for hiding this comment

parse-github-assistant bot commented Jul 10, 2022

Nidal-Bakir commented Jul 10, 2022 • edited Loading

codecov bot commented Jul 19, 2022 • edited Loading

Codecov Report

mtrezza commented Jul 28, 2022

mtrezza commented Oct 23, 2022

mtrezza commented Oct 23, 2022

Nidal-Bakir commented Oct 24, 2022 • edited Loading

mtrezza commented Oct 24, 2022

mbfakourii Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Nidal-Bakir commented Feb 24, 2023 • edited Loading

Nidal-Bakir commented Jul 9, 2022 •

edited by mtrezza

Loading

parse-github-assistant bot commented Jul 9, 2022 •

edited

Loading

mtrezza commented Jul 9, 2022 •

edited

Loading

Nidal-Bakir commented Jul 10, 2022 •

edited

Loading

codecov bot commented Jul 19, 2022 •

edited

Loading

Nidal-Bakir commented Oct 24, 2022 •

edited

Loading

mbfakourii Nov 29, 2022 •

edited

Loading

Nidal-Bakir commented Feb 24, 2023 •

edited

Loading