Skip to content

fix: SDK crashes due to missing error code property in ParseNetworkResponse.data #802

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

Merged
merged 15 commits into from
Dec 14, 2022
Merged

fix: SDK crashes due to missing error code property in ParseNetworkResponse.data #802

merged 15 commits into from
Dec 14, 2022

Conversation

mbfakourii
Copy link
Member

@mbfakourii mbfakourii commented Nov 27, 2022

New Pull Request Checklist

Issue Description

sometimes in buildErrorResponse code not exist in apiResponse.data and json.decode can't parse it!
but statusCode exist in ParseNetworkResponse and can be taken from this variable.

Related issue: #799
Closes: #799

Approach

In some times when the code is not available in ParseNetworkResponse.data, the statusCode is used in the ParseNetworkResponse.statusCode variable.

TODOs before merging

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

@parse-github-assistant
Copy link

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

@parse-github-assistant parse-github-assistant bot changed the title fix: bug in buildErrorResponse fix: Bug in buildErrorResponse Nov 27, 2022
@parse-github-assistant
Copy link

parse-github-assistant bot commented Nov 27, 2022

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@codecov
Copy link

codecov bot commented Nov 27, 2022

Codecov Report

Base: 3.35% // Head: 10.67% // Increases project coverage by +7.31% 🎉

Coverage data is based on head (89612ff) compared to base (a0af822).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
+ Coverage    3.35%   10.67%   +7.31%     
==========================================
  Files           5       47      +42     
  Lines         268     2810    +2542     
==========================================
+ Hits            9      300     +291     
- Misses        259     2510    +2251     
Impacted Files Coverage Δ
packages/dart/lib/src/objects/parse_error.dart 0.00% <ø> (ø)
...lib/src/objects/response/parse_error_response.dart 0.00% <0.00%> (ø)
...src/objects/response/parse_exception_response.dart 0.00% <ø> (ø)
packages/dart/lib/src/objects/parse_acl.dart 68.18% <0.00%> (ø)
packages/dart/lib/src/network/parse_client.dart 20.00% <0.00%> (ø)
packages/dart/lib/src/objects/parse_geo_point.dart 0.00% <0.00%> (ø)
packages/dart/lib/src/utils/parse_date_format.dart 77.77% <0.00%> (ø)
...src/objects/response/parse_success_no_results.dart 0.00% <0.00%> (ø)
...ckages/dart/lib/src/utils/parse_login_helpers.dart 0.00% <0.00%> (ø)
... and 35 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.

@mbfakourii mbfakourii changed the title fix: Bug in buildErrorResponse fix: Bug in buildErrorResponse from statusCode Nov 27, 2022
@mbfakourii mbfakourii changed the title fix: Bug in buildErrorResponse from statusCode fix: Bug in buildErrorResponse from statusCode Nov 27, 2022
@mbfakourii mbfakourii changed the title fix: Bug in buildErrorResponse from statusCode fix: statusCode not exist in ParseNetworkResponse.data Nov 27, 2022
@mbfakourii mbfakourii changed the title fix: statusCode not exist in ParseNetworkResponse.data fix: code not exist in ParseNetworkResponse.data Nov 27, 2022
@mtrezza
Copy link
Member

mtrezza commented Nov 27, 2022

What side effects could that change have? The way of building the error response with responseData[keyCode] has worked so far it seems?

@mbfakourii
Copy link
Member Author

What side effects could that change have? The way of building the error response with responseData[keyCode] has worked so far it seems?

I did various tests and found out that sometimes the code is not sent. That's why I put the null checker ?? for when it is not available, and when the code is not sent, it uses ParseNetworkResponse.statusCode.

@mtrezza
Copy link
Member

mtrezza commented Nov 28, 2022

So the underlying issue is that Parse Server sometimes doesn't send that code?

@mbfakourii
Copy link
Member Author

So the underlying issue is that Parse Server sometimes doesn't send that code?

yes for example
if appID is wrong and try for get health
https://parseapi.back4app.com/health
Json received without code is like this

{"error":"unauthorized"}

@mtrezza
Copy link
Member

mtrezza commented Nov 29, 2022

Got it, I'm a bit careful about introducing this ambiguity. It basically means that a developer cannot be sure whether the code is a parse error code or a http response code, right?

For example, if the error code is 400 and Parse Server also has a Parse Error code 400, how should a developer interpret this, and how should error handling logic in an app deal with this?

Maybe an alternative would be to use a distinct generic Parse Error code if no specific error code is present.

For example the docs already offer OtherCause (-1): "An unknown error or an error unrelated to Parse occurred."

@mbfakourii
Copy link
Member Author

mbfakourii commented Nov 29, 2022

Got it, I'm a bit careful about introducing this ambiguity. It basically means that a developer cannot be sure whether the code is a parse error code or a http response code, right?

For example, if the error code is 400 and Parse Server also has a Parse Error code 400, how should a developer interpret this, and how should error handling logic in an app deal with this?

Maybe an alternative would be to use a distinct generic Parse Error code if no specific error code is present.

For example the docs already offer OtherCause (-1): "An unknown error or an error unrelated to Parse occurred."

It is true that the developer is not able to understand the meaning of the code.
I use -1 instead of ParseNetworkResponse.statusCode
and when the code does not come from the server, -1 is returned.

and set ParseError.otherCause in parse_error_response.dart and parse_exception_response.dart
@mbfakourii
Copy link
Member Author

Changed and added const error code based on Parse-SDK-JS

mtrezza
mtrezza previously approved these changes Dec 9, 2022
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good to me; unless we get contrary feedback regrading the 3 removed codes.

  • Is the PR title correct?
  • Could you add the changelog entry by copy/pasting the PR title + bump the version for the release?

@mtrezza mtrezza changed the title fix: code not exist in ParseNetworkResponse.data fix: Incorrect error code if code property doesn't exist in ParseNetworkResponse.data Dec 9, 2022
@mtrezza
Copy link
Member

mtrezza commented Dec 9, 2022

@mbfakourii You have been quite active recently and make some great contributions. Can I add you to the review team of the Parse Flutter SDK? You would get notified if there are PRs to review.

@mtrezza mtrezza changed the title fix: Incorrect error code if code property doesn't exist in ParseNetworkResponse.data fix: SDK crash due to missing error code property in ParseNetworkResponse.data Dec 9, 2022
@mtrezza mtrezza changed the title fix: SDK crash due to missing error code property in ParseNetworkResponse.data fix: SDK crashes due to missing error code property in ParseNetworkResponse.data Dec 9, 2022
@mbfakourii
Copy link
Member Author

@mbfakourii You have been quite active recently and make some great contributions. Can I add you to the review team of the Parse Flutter SDK? You would get notified if there are PRs to review.

Yes, I will be happy to add

and title added in changelog

@mtrezza

This comment was marked as off-topic.

@mbfakourii

This comment was marked as off-topic.

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good!

@mtrezza mtrezza merged commit 4fa035c into parse-community:master Dec 14, 2022
@mbfakourii mbfakourii deleted the health_check_wrong_appid branch May 23, 2023 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

healthCheck throws error
2 participants