BigDecimal and BigInteger can be serialized by ghajba · Pull Request #85 · firebase/firebase-admin-java

ghajba · 2017-10-08T07:00:42Z

This is my solution to #75.

Because I didn't get any response and I need this change, I've solved it like this. But as mentioned in the related issue, I am open to suggestions.

And a question: when will be changes released that we can use the latest version instead of a snapshot?

googlebot · 2017-10-08T07:00:47Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

ghajba · 2017-10-08T07:04:44Z

I signed it! actually with my primary email address: hajbag@gmail.com 2017-10-08 9:00 GMT+02:00 googlebot <notifications@github.com>:

…

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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/ <https://cla.developers.google.com/> to sign.* Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks. ------------------------------ - If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. - If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. - In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#85 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACoigEzRoOWfPTt5DEZmiZw65W251ibeks5sqHMhgaJpZM4PxnZJ> .

googlebot · 2017-10-08T07:04:47Z

CLAs look good, thanks!

hiranya911

Thank you for the patch. Can you address a couple of questions, and also provide some test cases? See MapperTest, MutableDataTest and DataSnapshotTest.

hiranya911 · 2017-10-08T19:49:03Z

src/main/java/com/google/firebase/database/snapshot/NodeUtilities.java

      } else if (value instanceof Boolean) {
        return new BooleanNode((Boolean) value, priority);
+      } else if (value instanceof BigDecimal) {
+        return new DoubleNode(((BigDecimal) value).doubleValue(), priority);


This is a lossy conversion. What happens when the BigDecimal is too large to fit in a double?

I think BigDecimals should be serialized to String. The entire point of using BigDecimals is to avoid the rounding errors inherent in floating point representation.

hiranya911 · 2017-10-08T19:49:34Z

src/main/java/com/google/firebase/database/snapshot/NodeUtilities.java

+      } else if (value instanceof BigDecimal) {
+        return new DoubleNode(((BigDecimal) value).doubleValue(), priority);
+      } else if (value instanceof BigInteger) {
+        return new LongNode(((BigInteger)value).longValue(), priority);


Same question applies here. What happens when the BigInteger is too large to fit in a long?

Ditto for BigInteger -- we should serialize these to String to maintain the full representation.

ghajba · 2017-10-09T03:39:07Z

I know that these are lossy conversions. If the values don't fit, they'll be converted to infinity (positive or negative) for both types.

But I've already asked in #75 how the conversion should happen. After we clear this, I will adjust the code and write the tests too.

hiranya911 · 2017-10-09T04:37:30Z

I think you will have to implement a whole new Node type to support these types. In there you can just hold on to the string representation of the numbers.

ghajba · 2017-10-09T19:28:22Z

Just to let you know: I am working on the changes but at a slow pace. I hope I can push my changes this week.

added new node types I have still no idea how the tests should work.

hiranya911

Thanks @ghajba

Implementation looks pretty close to what I had in mind. Just some minor comments. Also please add the test cases.

hiranya911 · 2017-10-11T17:46:48Z

src/main/java/com/google/firebase/database/snapshot/BigDecimalNode.java

+
+  @Override
+  protected LeafType getLeafType() {
+    return LeafType.String;


I believe this should be LeafType.Number

And as a heads up, you will have to revise how leaf nodes are compared in the parent LeafNode.compareTo() method. You will see that longs and doubles have been special cased there. Perhaps you can implement a new comparison method for all number types, which converts values to BigDecimal and then does the comparison.

hiranya911 · 2017-10-11T17:50:36Z

src/main/java/com/google/firebase/database/snapshot/BigDecimalNode.java

+
+  @Override
+  public String getHashRepresentation(HashVersion version) {
+    return null;


I think this should do something similar to what the StringNode does.

hiranya911 · 2017-10-11T17:53:51Z

src/main/java/com/google/firebase/database/core/SyncTree.java

        serverCache = currentSyncPoint.getCompleteServerCache(relativePath);
      }
-    } while (!pathToFollow.isEmpty() && serverCache == null);
+    }


Please revert to keep the change footprint small as possible.

hiranya911 · 2017-10-11T17:54:08Z

src/main/java/com/google/firebase/database/snapshot/BigDecimalNode.java

@@ -0,0 +1,60 @@
+package com.google.firebase.database.snapshot;


Add license header.

hiranya911 · 2017-10-11T17:54:46Z

src/main/java/com/google/firebase/database/snapshot/BigIntegerNode.java

@@ -0,0 +1,60 @@
+package com.google.firebase.database.snapshot;


Lot of the comments made on BigDecimalNode are also applicable here. Please revise as appropriate.

I'd add the tests, but I don't see the idea behind how you test your code. The structure is not clear to find an easy entry-point.

NodeTest might be a good place to start. Also check MapperTest. See if you can serialize/deserialize some made up big integers and big decimals via the new code.

hiranya911 · 2017-10-11T17:55:34Z

src/main/java/com/google/firebase/database/snapshot/BigDecimalNode.java

+
+  @Override
+  public Object getValue() {
+    return this.value;


Should this return a BigDecimal? You can reconstruct the original value from the string.

ghajba · 2017-10-12T06:37:38Z

Thanks for the feedback. A big part of the codebase and what is expected is really unclear because of the missing JavaDoc.
Anyway, I'll try my best to come up with a feasible solution for this issue and get it integrated :)

added some tests and code changes

googlebot · 2017-10-14T07:19:10Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

hiranya911

Thanks @ghajba for the PR. This is starting to look pretty good. I've made some more comments (mostly syntactic changes).

Btw would you be able to provide an integration test too? I think we can add something to DataTestIT to cover the new functionality. The test case should write some big numbers to the database, and try to retrieve them back.

hiranya911 · 2017-10-20T21:56:55Z

src/main/java/com/google/firebase/database/snapshot/BigDecimalNode.java

+
+public class BigDecimalNode extends LeafNode<BigDecimalNode> {
+
+  private final String value;


I'm starting to wonder whether we get any benefit by keeping this as a string. I think we can just store it as a BigDecimal here.

hiranya911 · 2017-10-20T21:57:51Z

src/main/java/com/google/firebase/database/snapshot/BigDecimalNode.java

+
+  @Override
+  public int hashCode() {
+    return this.value.hashCode() + this.priority.hashCode();


This is not a very good hash function. Please use MoreObjects.hashCode() here. See https://github.com/google/guava/wiki/CommonObjectUtilitiesExplained#hashcode

hiranya911 · 2017-10-20T21:58:25Z

src/main/java/com/google/firebase/database/snapshot/BigIntegerNode.java

+
+public class BigIntegerNode extends LeafNode<BigIntegerNode> {
+
+  private final String value;


Use BigInteger directly.

hiranya911 · 2017-10-20T21:59:42Z

src/test/java/com/google/firebase/database/MapperTest.java

+  public void bigIntegerDeserialize() {
+    BigIntegerBean beanBigInteger = deserialize("{'value':'1'}", BigIntegerBean.class);
+    assertEquals(0,BigInteger.ONE.compareTo(beanBigInteger.getValue()));
+


Add space after comma.

hiranya911 · 2017-10-20T22:02:58Z

src/test/java/com/google/firebase/database/MapperTest.java

+    BigIntegerBean beanBigInteger = deserialize("{'value':'1'}", BigIntegerBean.class);
+    assertEquals(0,BigInteger.ONE.compareTo(beanBigInteger.getValue()));
+
+    BigIntegerBean beanBigIntegerBig =


Can you use a value that is guaranteed to be larger then what can fit in a long? Something like:

BigInteger value = new BigInteger(String.valueOf(Long.MAX_VALUE)).add(BigInteger.ONE);

hiranya911 · 2017-10-20T22:03:55Z

src/test/java/com/google/firebase/database/MapperTest.java

+  @Test
+  public void bigIntegerDeserialize() {
+    BigIntegerBean beanBigInteger = deserialize("{'value':'1'}", BigIntegerBean.class);
+    assertEquals(0,BigInteger.ONE.compareTo(beanBigInteger.getValue()));


Can we just do:

assertEquals(BigInteger.ONE, beanBigInteger.getValue());

hiranya911 · 2017-10-20T22:04:21Z

src/test/java/com/google/firebase/database/MapperTest.java

+            BigDecimalBean.class);
+    assertEquals(0,BigDecimal.ONE.compareTo(beanBigDecimal.getValue()));
+
+    BigDecimalBean beanBigDecimalBig = 


Same comments made on the test case above applies here.

hiranya911 · 2017-10-20T22:15:52Z

Instructions for integration tests can be found at: https://github.com/firebase/firebase-admin-java/blob/master/CONTRIBUTING.md#integration-testing

ghajba · 2017-10-25T17:43:47Z

Want to give a note that I have not abandoned my PR, but had some busy days. I am back now and will add the required changes/tests and will take a look at the comparison method too @hiranya911 mentioned previously.

googlebot · 2017-10-25T17:43:53Z

CLAs look good, thanks!

ghajba · 2017-10-26T11:06:58Z

Well, the integration testing description seems not working. I updated my sources to the latest version of firebase and the FirebaseCredentialsTest, using a new authentication which is not described in the mentioned documentation, fail.
This makes the contribution experience not the best.

hiranya911 · 2017-10-26T17:22:34Z

The instructions look correct to me. I use them fairly regularly in multiple dev environments, and haven't had any issues. What is the code/test you're trying to run, and what is the error you get?

FWIW, FirebaseCredentialsTest is not an integration test. It's only a unit test. And unit tests are invoked regularly by our CI: https://travis-ci.org/firebase/firebase-admin-java/builds

ghajba · 2017-10-26T18:34:55Z

These are the errors I get when running mvn verify:

Running com.google.firebase.auth.FirebaseAuthTest
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.298 sec <<< FAILURE! - in com.google.firebase.auth.FirebaseAuthTest
initializationError(com.google.firebase.auth.FirebaseAuthTest)  Time elapsed: 0.006 sec  <<< ERROR!
java.io.IOException: The Application Default Credentials are not available. They are available if running in Google Compute Engine. Otherwise, the environment variable GOOGLE_APPLICATION_CREDENTIALS must be defined pointing to a file defining the credentials. See https://developers.google.com/accounts/docs/application-default-credentials for more information.
        at com.google.firebase.auth.FirebaseAuthTest.createApplicationDefaultCredential(FirebaseAuthTest.java:143)
        at com.google.firebase.auth.FirebaseAuthTest.data(FirebaseAuthTest.java:108)

Running com.google.firebase.auth.FirebaseCredentialsTest
Tests run: 15, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.118 sec <<< FAILURE! - in com.google.firebase.auth.FirebaseCredentialsTest
defaultCredentialDoesntRefetch(com.google.firebase.auth.FirebaseCredentialsTest)  Time elapsed: 0.012 sec  <<< ERROR!
java.lang.RuntimeException: java.io.IOException: The Application Default Credentials are not available. They are available if running in Google Compute Engine. Otherwise, the environment variable GOOGLE_APPLICATION_CREDENTIALS must be defined pointing to a file defining the credentials. See https://developers.google.com/accounts/docs/application-default-credentials for more information.
        at com.google.firebase.auth.FirebaseCredentialsTest.defaultCredentialDoesntRefetch(FirebaseCredentialsTest.java:86)
Caused by: java.io.IOException: The Application Default Credentials are not available. They are available if running in Google Compute Engine. Otherwise, the environment variable GOOGLE_APPLICATION_CREDENTIALS must be defined pointing to a file defining the credentials. See https://developers.google.com/accounts/docs/application-default-credentials for more information.
        at com.google.firebase.auth.FirebaseCredentialsTest.defaultCredentialDoesntRefetch(FirebaseCredentialsTest.java:86)

And unit tests are executed with mvn test too.

hiranya911 · 2017-10-26T18:55:40Z

The test case does set the GOOGLE_APPLICATION_CREDENTIALS variable using some reflection magic:

    Map<String, String> environmentVariables =
        ImmutableMap.<String, String>builder()
            .put("GOOGLE_APPLICATION_CREDENTIALS", credentialsFile.getAbsolutePath())
            .build();
    TestUtils.setEnvironmentVariables(environmentVariables);

Looks like this doesn't work in your environment. What is your OS and JVM build? We test this on Linux and Mac, where it works fine.

ghajba · 2017-10-26T19:03:29Z

:)
it's currently Windows. I'll try it on a Mac too

hiranya911 · 2017-10-26T20:43:25Z

@ghajba Feel free to ignore those 2 failures. It's not related to your changes anyway. I'll try to come up with a way to skip those tests on Windows.

dimipaun · 2018-02-07T15:43:40Z

Hi @ghajba and @hiranya911,

Sorry for jumping late into this, but I think BigDecimal should serialize to String. The number 1 reason for using BigDecimals (such as in code that deals with money) is for it's exact representation of the fraction, rather than for the large values that it holds.

Serializing it to double throws this out the window.

hiranya911 · 2019-03-14T22:33:58Z

Over to @schmidt-sebastian who's been looking at #75. Based on the comment there, I assume we are not going to implement these changes?

schmidt-sebastian · 2019-03-15T03:30:18Z

Sorry for being slow to respond on this PR and thank you for your awesome contribution!
As you have probably seen, for now we have updated the error messages for both the RTDB and Firestore. The RTDB client uses JSON as its interop format, and FIrestore uses signed Int64 types. Both of these representations do not allow us to express BigIntegers correctly. While serializing to String is an accurate solution for this particular platform, the type doesn't round-trip very well and our other clients would not have the ability to treat these types as numbers. For that reason, I am leaning towards making this more explicit and asking you (and all other affected customers) to directly use String types instead.

schmidt-sebastian · 2019-03-15T03:31:13Z

Closing this PR for now. If we get significant feedback from a number of customers, we can revisit this, in which case this PR can probably just be merged as is.

ghajba added 2 commits October 8, 2017 08:21

BigDecimal and BigInteger can be serialized

86662e5

Merge remote-tracking branch 'upstream/master'

a943f47

hiranya911 suggested changes Oct 8, 2017

View reviewed changes

added new node types

930c781

Merge pull request #1 from ghajba/bigdecimal

adf76c3

added new node types I have still no idea how the tests should work.

hiranya911 suggested changes Oct 11, 2017

View reviewed changes

Gabor Hajba and others added 2 commits October 12, 2017 12:46

added some tests and code changes

b621a22

Merge pull request #2 from ghajba/bigdecimal

ba0b3c8

added some tests and code changes

hiranya911 suggested changes Oct 20, 2017

View reviewed changes

hiranya911 added the type: feature request label Oct 20, 2017

hiranya911 self-assigned this Oct 20, 2017

Merge remote-tracking branch 'upstream/master'

5c23ca3

hiranya911 assigned schmidt-sebastian and unassigned hiranya911 Mar 14, 2019

schmidt-sebastian closed this Mar 15, 2019

		@@ -0,0 +1,60 @@
		package com.google.firebase.database.snapshot;


		public class BigDecimalNode extends LeafNode<BigDecimalNode> {

		private final String value;


		public class BigIntegerNode extends LeafNode<BigIntegerNode> {

		private final String value;

Conversation

ghajba commented Oct 8, 2017

Uh oh!

googlebot commented Oct 8, 2017

Uh oh!

ghajba commented Oct 8, 2017 via email

Uh oh!

googlebot commented Oct 8, 2017

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghajba commented Oct 9, 2017

Uh oh!

hiranya911 commented Oct 9, 2017

Uh oh!

ghajba commented Oct 9, 2017

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghajba commented Oct 12, 2017

Uh oh!

googlebot commented Oct 14, 2017

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 commented Oct 20, 2017

Uh oh!

ghajba commented Oct 25, 2017

Uh oh!

googlebot commented Oct 25, 2017

Uh oh!

ghajba commented Oct 26, 2017

Uh oh!

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

Uh oh!

Uh oh!

ghajba commented Oct 26, 2017

hiranya911 commented Oct 26, 2017 •

edited

Loading