`LowPriorityKeyReads.readableKeyReads` should not exist

[arturaz]

Play JSON Version (2.5.x / etc)

3.0.1

API (Scala / Java / Neither / Both)

Scala

Problem

Map serialization/deserialization is broken if the key doesn't have a KeyReads instance.

The problem stems from the LowPriorityKeyReads.readableKeyReads which implies that if you have a Reads[A] then you have KeyReads[A], which does not seem like a logical conclusion. After all, the Reads can expect JsArray, JsObject or any other type, none of which are JsString.

import play.api.libs.json.*

val map = Map((1, 2) -> "foo")

val asJson = Json.prettyPrint(Json.toJson(map)) 
println(asJson)
// Gets serialized to: [ [ [ 1, 2 ], "foo" ] ]

val fromJson = Json.parse(asJson).validate[Map[(Int, Int), String]]
println(fromJson)
// Fails to deserialize, as a failing `KeyReads[(Int, Int)]` will be created from `LowPriorityKeyReads.readableKeyReads`

Suggested solution

Remove LowPriorityKeyReads.readableKeyReads. This breaks backwards compatibility, but I argue that this function is useless (as in it almost never produces a useful Reads instance anyway).

Reproducible Test Case

https://scastie.scala-lang.org/8ePgBH6lQ5i4GmCOPksvyw

[coreywoodfield]

Another point which you alluded to but didn't explicitly note as problematic:

implicitly[Format[Map[(Int, Int), String]]

produces a Format that is incompatible with itself. I don't completely agree that the readableKeyReads method is useless (it is in fact quite useful for things that serialize to JsStrings), but I think it should definitely not be implicit. It being implicit is a huge foot gun. In your example, the parsing isn't failing because the KeyReads doesn't work: it's failing because a json array can't be parsed as an object, which is what the Reads[Map[?, ?]] expects. I.e. the default implicit Reads for Map[Key, Value] (where Key is not String) is inherently incompatible with the default implicit Writes for Map[Key, Value] (assuming there is no custom implicit KeyReads or KeyWrites defined for Key).

I think this example makes that a bit more obvious. Note that the key type does in fact serialize to String, and the default implicit KeyReads is therefore sensible: https://scastie.scala-lang.org/ThpZiOBeR1OOFehC9VWpdA

So I think there are two possible "fixes" that could be made:

1. Make readableKeyReads not implicit. This could perhaps be paired with having a writeableKeyWrites that requires a new StringWrites (analogous to OWrites, but it's guaranteed to write JsStrings rather than JsObjects). The key thing here is that implicit definitions shouldn't allow us to get a Format that is incompatible with itself. So we either need to get rid of the general implicit KeyReads, or constrain it to types that serialize to string and add an equivalent implicit KeyWrites
2. Change readableKeyReads and add an implicit def writeableKeyWrites that is compatible with it. A possible implementation follows:

implicit def readableKeyReads[A: Reads]: KeyReads[A] = Json.parse(_).validate[A]
implicit def writeableKeyWrites[A: Writes]: KeyWrites[A] = a => Json.stringify(Json.toJson(a))

this is admittedly not ideal for things that do serialize to strings, but it works.

I would personally go with the first option.

[cchantep]

Hi,

readableKeyReads is used for key types represented as JSON strings, such as URI, Locale, and UUID.

The issue is that it currently applies to any T: Reads, even when T is not string-represented.

I drafted a solution that fixes this while preserving backward compatibility as much as possible: #1413