Initial code migration #1

hiranya911 · 2017-04-07T20:36:03Z

It's quite a few commits, but this is the gist of it:

All source files have been formatted to the satisfaction of the maven-checkstyle-plugin. This involved renaming many variables, rearranging many imports, re-indenting most source files and updating some javadocs.
Check style plugin configured to run with the build. Any lint errors will break the build.
All crypto artifacts (private keys, certificates etc.) have been scrubbed from the repo. They shouldn't be present in the Git history either. Updated tests to use a new set of mock crypto artifacts where necessary.
Went through all test cases, and re-enabled all unit test cases. They are all passing.
Legacy integration tests have been removed temporarily. I've added 2 of these back with some modifications to ensure we have support in Maven to run integration tests.

To run the linter by itself: mvn validate
To run unit tests only: mvn test
To run unit and integration tests: mvn verify -Dfirebase.it.certificate={PathToCertFile} -Dfirebase.it.url={FirebaseDbUrl}

* Reformatted all source files * Separated unit and integration tests * Added maven checkstyle plugin * Got the basic build and unit tests working * Scrubbed the codebase for private keys, certificates etc.

hiranya911 · 2017-04-09T19:16:11Z

New files added:

pom.xml
checkstyle.xml
Everything under src/test/resources/service_accounts
IntegrationTestUtils.java
FirebaseDatabaseTestIT.java (based on FirebaseDatabaseTest legacy integration test)
FirebaseDatabaseAuthTestIT.java (based on DatabaseServerAuthTest legacy integration test)

Files removed:

All legacy integration tests

jwngr

Okay, so I took a crack at reviewing this. It's huge! I honestly don't think it's going to be reasonable to review the source and test files since it's impossible to know what changed. From what I can tell, it was only linting changes and changes required to use new mocked keys. Given that, I think I'm cool with what's in here. But I do have a slight worry about possible minor bugs introduced from the linting changes. Those are going to be pretty difficult to find in this CL. What do you guys think?

jwngr · 2017-04-10T21:20:12Z

src/test/resources/service_accounts/owner.json

@@ -0,0 +1,12 @@
+{
+  "type": "service_account",
+  "project_id": "mock-project-id",


Should this be mock-project-id-owner? I see you use this in client_x509_cert_url, but nowhere else in this file. Should we also update client_email?

jwngr · 2017-04-10T21:20:46Z

src/test/resources/service_accounts/none.json

@@ -0,0 +1,12 @@
+{
+  "type": "service_account",
+  "project_id": "mock-project-id",


Should you add -none to project_id and client_email?

I think you're right about the client_email. project_id can be the same for all users.

I think the existing way is correct. These credentials are supposed to be for a set of users (with different privileges) on the same project. I've checked that client_email and cert_url fields are different for each user.

jwngr · 2017-04-10T21:21:16Z

src/test/resources/service_accounts/editor.json

@@ -0,0 +1,12 @@
+{
+  "type": "service_account",
+  "project_id": "mock-project-id",


Should you add -editor to project_id and client_email?

mikelehen · 2017-04-10T21:35:37Z

@schmidt-sebastian once fixed hundreds of lint errors in our Android codebase and to verify the correctness, he and BenWu decompiled the jars and compared the code. Can we do the same thing here?

…ethod

hiranya911 · 2017-04-10T23:00:55Z

Michael, that was an excellent suggestion. I ran a Java binary compatibility checker on 4.1.6 (last release) and the latest build from Github, and uncovered a couple of changes that shouldn't have been made. I'm making the appropriate fixes now.

hiranya911 · 2017-04-10T23:07:39Z

compat_reports.zip

I'm attaching the report generated by the binary compatibility checker. After the latest fixes the compatibility level is 98.3%. You can see the report by extracting the attached zip, and opening the file compat_reports/firebase-admin/4.1.6_to_4.1.7-SNAPSHOT/compat_report.html in a web browser.

The only reported changes are due to me renaming some type variables. For example types like Task<TResult> were renamed to Task<T> to meet style requirements. The binary compatibility checker report them as incompatibilities for some reason. The only other change is removal of ThreadBackgroundExecutor.java. But this was an empty class to begin with, and didn't have any code.

hiranya911 · 2017-04-11T02:21:43Z

Jacob, take a look at the binary diff report I've attached. I think it shows that there is no noticeable change at the binary level. So I think we should be good to go. We also have reasonable amount of unit tests, all of which passes. Plus, I've been working on porting the legacy integration tests, and most of them work when pointed to Firebase prod.

schmidt-sebastian · 2017-04-11T04:57:16Z

Jacob, take a look at the binary diff report I've attached. I think it shows that there is no noticeable change at the binary level. So I think we should be good to go. We also have reasonable amount of unit tests, all of which passes. Plus, I've been working on porting the legacy integration tests, and most of them work when pointed to Firebase prod.

This checker (if I understand correctly) checks for changes in the API surface. That is not enough unfortunately. As @mikelehen pointed out, the way we checked this when I cleaned up the code for G4 compatibility was to create jar files and decompile them. Your diff will likely still be considerable since you renamed variables, but I don't think there is any other way.

This commit will also break our ability to merge changes from Android directly, but I think that's fine for now.

hiranya911 · 2017-04-11T07:13:44Z

I ran another binary diff tool (pkgdiff) which compares compiled class files all the way down to the instructions. According to this tool the difference between v4.1.6 and latest snapshot is 4.3%. I need to run this again on a Linux environment, to be able to see the detailed diff output. Will do that tomorrow.

hiranya911 · 2017-04-11T16:51:38Z

Attaching the pkgdiff reports. I'm still sifting through the data. Will report on my findings. File to look for is pkgdiff_reports/firebase-admin/4.1.6_to_4.1.7-SNAPSHOT/changes_report.html

pkgdiff_reports.zip

hiranya911 · 2017-04-11T17:17:04Z

Here's a diff report between decompiled sources:

X_to_Y.zip

hiranya911 · 2017-04-11T18:04:48Z

I've gone through the diff report for the decompiled sources. I'm not seeing any changes that can result in API or behavior changes. In general, the reported diff's are due to following reasons:

Order of methods changing in source files.
Variable renaming.
Differences in how the 2 versions of Java compiler handles string concatenation and anonymous classes (the snapshot was built using JDK 1.8).

The only functional diff I saw was in the GaeThreadFactory class, but this is due to a change we merged to the codebase after 4.1.6 release.

jwngr

LGTM. I took a look at the latest diff report you generated and it looked like there was feature parity between the old and new and it's comforting that all our test suites pass. I do think someone like @schmidt-sebastian with some more Java background should take a look to triple check this though before we merge it in.

schmidt-sebastian · 2017-04-13T03:50:41Z

I spot checked a fair number of files in X_to_Y and the diff looks good to me. Thanks for being so thorough!

Hiranya Jayathilaka and others added 14 commits April 5, 2017 19:12

Initial code migration:

428eedb

* Reformatted all source files * Separated unit and integration tests * Added maven checkstyle plugin * Got the basic build and unit tests working * Scrubbed the codebase for private keys, certificates etc.

Reformatting code according to check-style plugin conventions

cfbbd6a

Code reformatting: Fixing javadoc indentation

59842b8

More reformatting: Added google check style config

fef4ecf

Code formatting continued

a606833

Code formatting continued

1d2ed82

Reformatting

8972b1b

Fixed all checkstyle violations; Build configured to fail on lint errors

e5509c3

Updated checkstyle config

0587d52

Working on re-enabling the integration tests

7f2f470

Temporarily removing IT test cases

8ee5cfc

Extending checkstyle coverage to test sourcesw

dbc6ee6

Cleaned up verbose test output

c66f35b

Adding 2 integration tests to verify the maven integration test support

f7cbdc2

hiranya911 requested review from jwngr and schmidt-sebastian April 7, 2017 20:36

hiranya911 assigned jwngr Apr 7, 2017

jwngr suggested changes Apr 10, 2017

View reviewed changes

Moving test only trampolines to the test/ subtree; Adding a removed m…

7715b90

…ethod

jwngr approved these changes Apr 12, 2017

View reviewed changes

jwngr assigned schmidt-sebastian and unassigned jwngr Apr 12, 2017

schmidt-sebastian approved these changes Apr 13, 2017

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Apr 13, 2017

hiranya911 merged commit 30702e7 into master Apr 13, 2017

hiranya911 deleted the hkj-initial-code-migration branch April 13, 2017 04:09

mikelehen mentioned this pull request May 16, 2017

Fixing a Thread Leak in Database Client Code (Related to #29) #30

Merged

grebenshikov mentioned this pull request Jul 3, 2019

Firebase connection problems from GCP Cloud Run app #284

Closed

Initial code migration #1

Initial code migration #1

Uh oh!

Conversation

hiranya911 commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 commented Apr 9, 2017

Uh oh!

jwngr left a comment

Choose a reason for hiding this comment

Uh oh!

jwngr Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

jwngr Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

hiranya911 Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

hiranya911 Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

jwngr Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

mikelehen commented Apr 10, 2017

Uh oh!

hiranya911 commented Apr 10, 2017

Uh oh!

hiranya911 commented Apr 10, 2017

Uh oh!

hiranya911 commented Apr 11, 2017

Uh oh!

schmidt-sebastian commented Apr 11, 2017

Uh oh!

hiranya911 commented Apr 11, 2017

Uh oh!

hiranya911 commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 commented Apr 11, 2017

Uh oh!

hiranya911 commented Apr 11, 2017

Uh oh!

jwngr left a comment

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Apr 13, 2017

Uh oh!

Uh oh!

hiranya911 commented Apr 7, 2017 •

edited

Loading

hiranya911 commented Apr 11, 2017 •

edited

Loading