Skip to content

Added precondition format checks for App-Id and Api-Key #1470

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.

Sign up for GitHub

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

Jump to bottom
Merged
ChaoqunCHEN merged 4 commits into from
Apr 22, 2020
Merged

Added precondition format checks for App-Id and Api-Key #1470

ChaoqunCHEN merged 4 commits into from
Apr 22, 2020

Conversation

@ChaoqunCHEN
Copy link
Contributor

@ChaoqunCHEN ChaoqunCHEN commented Apr 15, 2020

No description provided.

@googlebot googlebot added cla: yes Override cla labels Apr 15, 2020
@ChaoqunCHEN ChaoqunCHEN requested a review from andirayo April 15, 2020 20:13
@ChaoqunCHEN ChaoqunCHEN requested a review from ankitaj224 April 15, 2020 20:13
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 15, 2020 •
edited
Loading

Binary Size Report

Affected SDKs

  • firebase-common

    Type Base (205b523) Head (1b20739) Diff
    aar ? 34.7 kB ? (?)
    apk (aggressive) ? 82.5 kB ? (?)
    apk (debug) ? 770 kB ? (?)
    apk (release) ? 636 kB ? (?)

  • firebase-components

    Type Base (205b523) Head (1b20739) Diff
    aar ? 34.5 kB ? (?)
    apk (aggressive) ? 11.0 kB ? (?)
    apk (debug) ? 35.8 kB ? (?)
    apk (release) ? 25.4 kB ? (?)

  • firebase-installations

    Type Base (205b523) Head (1b20739) Diff
    aar ? 58.1 kB ? (?)
    apk (aggressive) ? 84.3 kB ? (?)
    apk (debug) ? 794 kB ? (?)
    apk (release) ? 657 kB ? (?)

  • firebase-installations-interop

    Type Base (205b523) Head (1b20739) Diff
    aar ? 7.51 kB ? (?)
    apk (aggressive) ? 61.7 kB ? (?)
    apk (debug) ? 744 kB ? (?)
    apk (release) ? 616 kB ? (?)

Test Logs

andirayo
andirayo approved these changes Apr 15, 2020
View reviewed changes
Copy link
Contributor

@andirayo andirayo left a comment

Choose a reason for hiding this comment

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

good stuff

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 21, 2020 •
edited
Loading

Coverage Report

Affected SDKs

  • firebase-installations

    SDK overall coverage changed from 57.45% (205b523) to 57.81% (1b20739) by +0.37%.

    Filename Base (205b523) Head (1b20739) Diff
    FirebaseInstallations.java 97.08% 97.69% +0.61%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

andirayo
andirayo reviewed Apr 21, 2020
View reviewed changes
...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java
return TextUtils.isEmpty(firebaseApp.getOptions().getProjectId())
? firebaseApp.getOptions().getGcmSenderId()
: firebaseApp.getOptions().getProjectId();
return firebaseApp.getOptions().getProjectId();
Copy link
Contributor

@andirayo andirayo Apr 21, 2020

Choose a reason for hiding this comment

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

Is there no test that you have to modify?
(I mean a test that created FirebaseApp without projectId but with gcmSenderId.)

Copy link
Contributor Author

@ChaoqunCHEN ChaoqunCHEN Apr 21, 2020

Choose a reason for hiding this comment

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

No, we don't have

@ChaoqunCHEN ChaoqunCHEN force-pushed the readd_FIS_format_check branch from 843413b to 1b20739 Compare April 21, 2020 22:50
@ChaoqunCHEN ChaoqunCHEN merged commit 1807529 into master Apr 22, 2020
@ChaoqunCHEN ChaoqunCHEN deleted the readd_FIS_format_check branch April 22, 2020 00:27
@firebase firebase locked and limited conversation to collaborators May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ankitaj224 ankitaj224 Awaiting requested review from ankitaj224

1 more reviewer

@andirayo andirayo andirayo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes Override cla size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ChaoqunCHEN @google-oss-bot @andirayo @googlebot