feat(fcm): Added Notification Count parameter to AndroidNotification class

[knocknarea]

Discussion

As discussed in issue #308

Testing

• Added Test for setNotificationCount. I noticed that your JsonParser implementation prefers BigDecimal and it would appear that Integer, Long, etc. are deserialized into BigDecimal.

API Changes

• None

[hiranya911]

This should take an int

[knocknarea]

Not really, the spec allows for null/empty in which case the badge count is unaffected.

[knocknarea]

But I see what you mean, however you will have to break the fluidity of the builder to determine if you should set this if it was an int. Whereas if it's an Integer you can use a variable set to null or badge count before calling the builder.

Integer count = determineCount();

AndroidNotification.builder().setNotificationCount(count).build();

Vs

Integer count = ...

Builder builder = AndroidNotification.builder();

if(count != null) {
   builder.setNotificationCount(count);
}

[hiranya911]

The public API should accept an int. The internal representation could be Integer with a default value of null.

[knocknarea]

But null is part of the public spec, int breaks the builder pattern.

[knocknarea]

Ok, is it not the case that if a property annotated with @@key is null, it is not included in the resultant json? Thus fulfilling the requirement of unspecified?

By using a primitive type in the setter, you have no way of indicating this other than not calling the setter.

And so, as a user of the builder, you would then have no way to use the builder fluently if you had to check if the value of notification count was null.

Mindful of the fact that deriving that value might be complex and that the point of calling the builder may have been supplied this value from a higher level of code. Instead of just handing the value to the builder you would have to create a whole branch in code just switching on that value.

[hiranya911]

Ok, is it not the case that if a property annotated with @@key is null, it is not included in the resultant json? Thus fulfilling the requirement of unspecified?

Right. Set the default value to be null, and you get the right semantics. See how Aps.badge is implemented for an example.

Mindful of the fact that deriving that value might be complex and that the point of calling the builder may have been supplied this value from a higher level of code

The higher-level code should never return null either. It should return 0. If it must return null, it's fairly easy to handle it as:

Integer count = determineCount();
builder.setNotificationCount(count != null ? count : 0); 

[knocknarea]

It may be that the higher level code determines that the badge count on the client device is left unchanged by this push notification. The only way to Express this is to return a count == null.

If the fire base builder can handle null properly and convey this by setting notification count to null (such that it is not included in the json). Then what is the problem here?

Null is a valid value for the attribute that says 'leave the count as it is on the client device'

It seems to me that Optional Integer is a good candidate for differentiation, but primitive int is not sufficient to express all values possible without breaking the fluency of the builder.

I can set it to primitive int, no problem, it just does not feel right in this case

[hiranya911]

Null is a valid value for the attribute that says 'leave the count as it is on the client device'

Null is not a valid value for a field declared as number in the backend. Also in this case the backend allows both 0 and undefined to mean "keep as is". So it's easy to deal with any null-returning user code by mapping null to 0.

Furthermore JSON null and JSON undefined usually have different semantics:

{
  "foo": "test",
  "bar": null // bar is null
}

{
  "foo": "test",
  // bar is undefined
}

When one calls an API as setBar(null), it's typical to expect the value to get serialized as null.

It seems to me that Optional Integer is a good candidate for differentiation

Optional is a Java8 API and we are still on Java7. Plus, that's a somewhat overkill solution for such a simple problem. We already have an established pattern in the SDK to deal with type of fields, and we should be using it here to be consistent.

[knocknarea]

@hiranya911

Ok, I think this comes down to a misunderstanding on my part.

My assumption (which is obviously wrong) is that if an attribute annotated with @key is not initialized or set to a specific value. Then its default value in java is null. The assumption here is that your json serializer does not actually add the attribute to the resultant json,

...

@Key("my_var")
private String myVar;

...

i.e. If the value is null, then instead of this:

{ 
  ...
   "my_var": null,
  ...
}

You'd get this:
{
...
...
}

[hiranya911]

Thanks @knocknarea. Mostly looks good. Just a few nits on API docs and the test case.

[knocknarea]

@hiranya911 Is this ready now?

[hiranya911]

Thanks @knocknarea. LGTM 👍

[chong-shao]

LGTM!
I did some commits from the website trying to fix the merge issues.