dsrees/JavaPhoenixClient

Message.payload marked as non-nullable but can be null

lukasz-kalnik-gcx opened this issue · 7 comments

Because Gson doesn't respect default values of parsed classes, if the payload is missing in the received event JSON then its value will still be null, breaking the non-nullability contract for the field (it is defined as val payload: Payload = Hashmap() in the Message class).

A quick fix would be to make the Payload field nullable.

Also, is there a reason why Message isn't a data class?

Is payload an optional value from Phoenix? I was under the impression that it would return an empty payload if none was specified

I don't know how it's specified, but our app is crashing with an NPE inside on(event: String, callback: (Message) -> Unit) when trying in the callback to perform the message.payload.isNotEmpty() check, saying that the Map instance is null.

I would say you have a bug in your phoenix app. There should always be a payload at least in the form of

"payload":{"status":"ok", ...}

A message without a payload doesn't have much value to it. I will try to verify this behavior on phoenix but until then, im hesitant to make any change

A message without payload still has a value. There are sometimes events which don't carry any information, apart from that the event happened (e.g. scan finished or something like this).

Also I don't insist on changing the API to make payload nullable. I know this would break the code of existing library users.
I think the fix should rather be making sure that the non-nullable contract of this field is not broken by Gson.

I might be missing something, but this test case passes while the json is missing a payload

@Test
  internal fun `parses from Gson if missing payload`() {
    val json = """
      {"topic":"phoenix","ref":"82","event":"phx_reply"}
    """
    val message = Defaults.gson.fromJson(json, Message::class.java)
    assertThat(message.payload).isNotNull()
  }

Yes, indeed, the test passes. I don't know why at runtime on an Android device it then still returns null.
Never mind, we have found a workaround in our app. So you can close the issue.

Thanks for your support!

Of course. Thanks for reaching out!