Fix for reading Some(Map.empty)

[martinw-ct]

A small failing test plus my suggested fix for what I think may be a bug in how it handles empty maps in an option. 🤔

Specifically {} in json when read into a Option[Map[String, _]] becomes None instead of Map.empty.

[martinw-ct]

A very simple fix is to remove JObject(Nil) from here

sphere-scala-libs/json/json-core/src/main/scala/io/sphere/json/FromJSON.scala

Line 45 in 51637a9

case JNothing | JNull | JObject(Nil) => validNone

But it looks very intentional that a {} be mapped to None in general (e.g. a case class with only optional fields) and I strongly suspect some callers have come to depend on this, so that probably isn't the way to go.

[yanns]

A very simple fix is to remove JObject(Nil) from here

sphere-scala-libs/json/json-core/src/main/scala/io/sphere/json/FromJSON.scala

Line 45 in 51637a9

case JNothing | JNull | JObject(Nil) => validNone

But it looks very intentional that a {} be mapped to None in general (e.g. a case class with only optional fields) and I strongly suspect some callers have come to depend on this, so that probably isn't the way to go.

When we used json to map objects into mongo, we tried hard not to optimize the payload. For example, for an optional field, we removed all "field": null to save a payload without the "field" field.

I guess that JObject(Nil) is in the same vein? Instead of saving "field": {}, we tried to save a payload without the "field" field? (I'm not sure about this one)

We can also try to remove it, make some test release and check if all tests are passing. WDYT?

[yanns]

I don't understand why we treat Option[Map] differently from Option[List]. This does not make sense.

[martinw-ct]

We can also try to remove it, make some test release and check if all tests are passing. WDYT?

I tried removing the JObject instead of the hacky specialized reader (because your comment made sense) and hit something very strange.

There are tests asserting that Option[Int] when None gets written out and read back as {} and they fell over. Fixing the tests were easy and I'm pretty sure that codepath was never hit on prod, just in some internal benchmarks and test code. Here is it in another branch 5f97030

Shall put that commit in this PR? I am now mildly concerned there is a Mongo record somewhere with {} instead of null though. 🤔

[yanns]

We can also try to remove it, make some test release and check if all tests are passing. WDYT?

I tried removing the JObject instead of the hacky specialized reader (because your comment made sense) and hit something very strange.

There are tests asserting that Option[Int] when None gets written out and read back as {} and they fell over. Fixing the tests were easy and I'm pretty sure that codepath was never hit on prod, just in some internal benchmarks and test code. Here is it in another branch 5f97030

Shall put that commit in this PR? I am now mildly concerned there is a Mongo record somewhere with {} instead of null though. 🤔

Yes, we should not have any null at the end. This PR with a specialized typeclass seems more safe.