Skip to content

Ignore empty message string in FirebaseFunctionsException #501

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 1 commit into from
Jun 6, 2019

Conversation

muZk
Copy link
Contributor

@muZk muZk commented Jun 6, 2019

When throwing new HttpsError('not-found') in a cloud function (ie: without specifying message parameter), error.opt("message") will be just an empty string, so:

  • error.opt("message") instanceof String is true
  • details will be assigned to "".
  • new FirebaseFunctionsException(description, code, details) will be called with this empty string
  • it will throws a Detail message must not be empty exception.

With this change, details will be equal to whatever code.name() returns when the cloud function sends an empty string as a message. In the case of HttpsError('not-found'), it will be NOT_FOUND.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-oss-bot
Copy link
Contributor

Hi @muZk. Thanks for your PR.

I'm waiting for a firebase member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@muZk
Copy link
Contributor Author

muZk commented Jun 6, 2019

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Override cla and removed cla: no labels Jun 6, 2019
@samtstern
Copy link
Contributor

@muZk this LGTM!

@bklimt any concerns?

@samtstern samtstern self-requested a review June 6, 2019 20:42
@samtstern
Copy link
Contributor

/ok-to-test

@bklimt
Copy link
Contributor

bklimt commented Jun 6, 2019

Looks good to me. Thanks for the fix!

I was considering whether it might be better to do error.opt("message", null), but no, I think this PR is better, because an explicit empty string wouldn't work anyway.

Looks like the line might be too long. Make sure to run ./gradlew :firebase-functions:googleJavaFormat to auto format it. I think that's the command, but can't look it up atm.

@muZk
Copy link
Contributor Author

muZk commented Jun 6, 2019

Looks like the line might be too long. Make sure to run ./gradlew :firebase-functions:googleJavaFormat to auto format it. I think that's the command, but can't look it up atm.

Yes, that's the command! I just ran it without modifications. Maybe it is ok? (93 char long)

@samtstern
Copy link
Contributor

@muZk thanks! Gonna merge this now.

@samtstern samtstern merged commit f757065 into firebase:master Jun 6, 2019
@firebase firebase locked and limited conversation to collaborators Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants