If you use Body.auto() with Gson, and send the JSO...

# http4k

[Daniel Skogquist Åborg]

If you use Body.auto() with Gson, and send the JSON literal "null" as body, you get a null out of the lens even though it promises not to return nulls in its contract. Should this be validated by the lens or is it application logic?

[s4nchez]

@Daniel Skogquist Åborg I’m not sure I follow. Are you able to provide an example of this behaviour?

[Daniel Skogquist Åborg]

@Test
    fun nullLensReturn() {
        data class Input(val data: String? = null)

        val app: HttpHandler = { request ->
            val input = Body.auto<Input>().toLens()

            val received = input(request) // not nullable, but Gson overrides this

            println(received as Input? == null) // true

            Response(OK, "Hello world")
        }

        app(Request(GET, "/").body("null"))
    }

[Daniel Skogquist Åborg]

I'm testing corner cases in my API written in http4k, this is the kind of thing that could easily be sent by a broken javascript backend to the API if they serialize a null value instead of the object they intended to send.

[Daniel Skogquist Åborg]

(In fact, it has already happened.)

[Daniel Skogquist Åborg]

Other syntactically correct but unmappable request bodies correctly generate a LensFailure:

listOf(
    "[]",
    "true",
    "false",
    "1",
) // all generate a LensFailure with "Missing/invalid parameters"

[s4nchez]

Are you using Gson? I'm seeing the behaviour you're explaining if I use Jackson (server accepts the literal null and builds a null Input even though the type is not nullable), but with Gson I get:

Exception in thread "main" org.http4k.lens.LensFailure: body 'body' must be object
	at org.http4k.lens.BodyLens.invoke(body.kt:23)
	at PlaygroundKt$main$app$1.invoke(playground.kt:15)
	at PlaygroundKt$main$app$1.invoke(playground.kt)
	at PlaygroundKt.main(playground.kt:20)
	at PlaygroundKt.main(playground.kt)
Caused by: java.lang.NullPointerException: null cannot be cast to non-null type Input
	at PlaygroundKt$main$app$1$$special$$inlined$auto$1.invoke(ConfigurableGson.kt:109)
	at PlaygroundKt$main$app$1$$special$$inlined$auto$1.invoke(ConfigurableGson.kt)

[Daniel Skogquist Åborg]

You're right, I'm using Jackson. Doh. I thought I changed. I switched back to Jackson because Gson litters my objects with nulls.

[s4nchez]

Ah ok. We could try and add that to AutoMarshallingJson contract to make it consistent across all implementations. Would you like to open an issue for that or rather leave it with us?

[Daniel Skogquist Åborg]

I'll have a look at the code for AutoMarshallingJson, if I can fix it I'll send a PR, otherwise I'll raise an issue.

[Daniel Skogquist Åborg]

Do you have any kind of license waiver I need to sign to contribute?

[s4nchez]

Thank you, really appreciated. Take a look at AutoMarshallingJsonContract as it's probably a good place to start.

[s4nchez]

No need to sign anything to contribute, no.

[dave]

I would advise to use Moshi over Gson btw. And Jackson if you need access to the Json Dom)

[Daniel Skogquist Åborg]

I'm looking at the code and there are some differences in the behaviour of the parsers. Argo and Jackson both support reading HTTP body lenses from JSON literals, whereas Gson and KotlinXSerialization refuses to do so, considering literals to not be objects. (Including null literals.)

My preference is to make all parsers support reading body lenses from any kind of valid JSON literal, and require non-null semantics higher in the chain, as parsers should not be where requirements on the structure of valid JSON is enforced(?).

Some tests which show differences in behaviors: https://github.com/dsaborg-waya/http4k/compare/master...dsaborg-waya:json-literals-in-body-lens

How would you like me to handle this?

I'm thinking either make all parsers to parse bodies as only non-null objects, or allow parsers to parse anything and enforce these semantics elsewhere (lens spec?).

Is there a better place to discuss this?

[s4nchez]

Happy to discuss here but a PR would be better to save it for future reference.

For the change, I’m leaning towards making all json implementations responsibility to parse to the correct type and fail one way or another. The alternative feels like pushing the responsibility to a place (lens) that shouldn’t care about that

[Daniel Skogquist Åborg]

OK, I'll see if I can get a PR ready for that.

[Daniel Skogquist Åborg]

I'm reluctant to change existing behaviour of the parsers, as it's a breaking change, but I'll sort out so they don't deserialize nulls when an object is expected.

[dave]

That seems sensible. We generally deal with these differences by adding the "standard case" to the contract and then overriding the specific test in the implementations as required. Then at least it's documented what the behaviour actually is. 🙃

[Daniel Skogquist Åborg]

Here is the initial suggestion, pending a few questions: https://github.com/dsaborg-waya/http4k/compare/master...dsaborg-waya:json-literals-in-body-lens

Questions:

1. There's a lot of duplicated null checks, I'd like to make this more generic but it requires adding NODE.isNull() to JsonContract so we can move null node checks into JsonContract. Is that ok?
2. I'd like to apply the direct null checks I've added in ConfigurableJackson to the other parsers, to protect against reading nulls from Java.
3. I'd like to update ContractRoutingHttpHandlerTest to run the tests with all JSON parsers, not just Jackson, to be sure this works.
4. I've got a few test failures in the server tests due to port binding clashes on my machine, I'd like to update this to use some kind of random port allocation to make sure tests can be run anywhere.
If you can have a look at the code above and let me know whether I'm on the right track and what you think of the above ideas I'll continue with your feedback.

[Daniel Skogquist Åborg]

An alternative to 3. above would be to tighten up the tests for JsonContract to check nullability behaviour across all parsers. This is probably a good idea regardless.

[dave]

1. yep - let's remove the deduplication
2. Yep - can we also remove duplication here as well?
3. Absolutely!
4. There are a couple of server implementations which don't actually support random ports (🙄 ) and sometimes don't close down cleanly occasionally. I thought we'd got all of them for the tests - but if you find some others then absolutely please make the tests better!

[Daniel Skogquist Åborg]

Working on this. Another question: As I understand it, without a contribution waiver you're giving contributors the right to e.g. demand that you remove all their commits from your codebase, as they then own the copyright(?). Maybe worth looking into what rights you're implicitly agreeing to when people contribute. I understand you want it to be easy to contribute, but it's also worth considering that people can do stupid and silly things for arbitrary reasons sometimes. 🙂

[Daniel Skogquist Åborg]

Consider for example someone who contributes for a year, then gets upset by something you do (people can get very possessive about their code, I see it all the time), forks your codebase and demands you remove their code from your original repository. Chaos for you, they get to continue working on your project. Regardless of the outcome, it's not a fun situation to be in.

[dave]

The Apache license:

Grant of Copyright License. Subject to the terms and conditions of
      this License, each Contributor hereby grants to You a perpetual,
      worldwide, non-exclusive, no-charge, royalty-free, irrevocable
      copyright license to reproduce, prepare Derivative Works of,
      publicly display, publicly perform, sublicense, and distribute the
      Work and such Derivative Works in Source or Object form.

   3. Grant of Patent License. Subject to the terms and conditions of
      this License, each Contributor hereby grants to You a perpetual,
      worldwide, non-exclusive, no-charge, royalty-free, irrevocable
      (except as stated in this section) patent license to make, have made,
      use, offer to sell, sell, import, and otherwise transfer the Work,
      where such license applies only to those patent claims licensable
      by such Contributor that are necessarily infringed by their
      Contribution(s) alone or by combination of their Contribution(s)
      with the Work to which such Contribution(s) was submitted. If You
      institute patent litigation against any entity (including a
      cross-claim or counterclaim in a lawsuit) alleging that the Work
      or a Contribution incorporated within the Work constitutes direct
      or contributory patent infringement, then any patent licenses
      granted to You under this License for that Work shall terminate
      as of the date such litigation is filed.

[Daniel Skogquist Åborg]

Awesome. Love it when people have already thought of these things. Thanks.