Haskell-OpenAPI-Code-Generator / Haskell-OpenAPI-Client-Code-Generator

Generate Haskell client code from an OpenAPI 3 specification
46 stars 19 forks source link

Failed parsing nullable response field #93

Closed pbrisbin closed 6 months ago

pbrisbin commented 7 months ago

Apologies in advance if I'm missing something, but...

I have some OpenAPI docs that include the following response,

      "LoginResponse": {
        "properties": {
          "result": {
            "$ref": "#/components/schemas/MatchResult"
          },
          "global_user_id_matched": {
            "type": "string",
            "title": "Global User Id Matched",
            "description": "Matched global user id"
          },
          "submitted_payload": {
            "$ref": "#/components/schemas/LoginRequest"
          }
        },
        "additionalProperties": false,
        "type": "object",
        "required": [
          "result",
          "submitted_payload"
        ],
        "title": "LoginResponse"
      },

I see global_user_id_matched of type: string that is not listed under required. As I expected, this generates a Maybe field:

  loginResponseGlobal_user_id_matched :: (GHC.Maybe.Maybe Data.Text.Internal.Text)

But when I actually go to use the thing, the JSON de-serialization fails:

2024-02-16 21:24:15 [warn     ] Unsuccessful response posting LoginRequest
                                env=local
                                environment=local
                                route=StudentsSessionsR
                                team=Educator
                                trace_id=5.263277280113848999e18
                                version=8f38ec44603f7c59cf1db132fa0305f43a5f8a59
                                request={is_unique: False, local_user_id: 3, product_name: freckle}
                                responseBody=Error in $['global_user_id_matched']: parsing Text failed, expected String, but encountered Null
                                responseStatus=200

Isolated, that's:

Error in $['global_user_id_matched']: parsing Text failed, expected String, but encountered Null

I believe the issue is in the FromJSON,

obj Data.Aeson.Types.FromJSON..:! "global_user_id_matched"

[.:!] differs from .:? by attempting to parse Null the same as any other JSON value, instead of interpreting it as Nothing.

So I guess my API is returning global_user_id_matched: null instead of omitting the field, and the generated client is strict about that. I'd prefer it not be. Is there anything I can do?

One other thing to note: the specification I'm working with originally had,

"global_user_id_matched": {
  "anyOf": [{"type": "string"}, {"type": "null"}]
}

Which sounds an awful lot like the spec trying to say that it might return an explicit null instead of omitting the field entirely. But the generator rejects this:

Error in $.components.schemas.LoginResponse.properties['global_user_id_matched'].anyOf[1].type: Only types integer, string, number, boolean, array and object are supported but got: null

So I changed it to simply type: string because, as I understand it, it's omission from required means it's nullable... but now is that the source of (.:!) being an issue?

NorfairKing commented 6 months ago

@joel-bach I haven't run into this issue before. What do you think?

joel-bach commented 6 months ago

Hey @pbrisbin ,

The behavior you are facing is by design. The tricky bit is that nullable and optional are not the same thing (if you are interested, we had a discussion about this here a while back). It is by design that the deserialization fails if null is returned but the field is not nullable. Essentially it means that the OpenAPI specification is not matching the actual behavior of the API. What I would propose is to define the field as nullable, so the specification would look something like this:

      "LoginResponse": {
        "properties": {
          "result": {
            "$ref": "#/components/schemas/MatchResult"
          },
          "global_user_id_matched": {
            "type": "string",
            "title": "Global User Id Matched",
            "description": "Matched global user id",
+           "nullable": true
          },
          "submitted_payload": {
            "$ref": "#/components/schemas/LoginRequest"
          }
        },
        "additionalProperties": false,
        "type": "object",
        "required": [
          "result",
          "submitted_payload"
        ],
        "title": "LoginResponse"
      },

Note that it may be reasonable to define the field as required as well so you do not end up with a Nullable (Maybe a) which is probably not what you want if your API actually always returns the field (just returning null in some cases but never omits it entirely).

I hope that helps!

I'll close the ticket because the behavior of the generator is how the design is intended.

pbrisbin commented 6 months ago

Thanks @joel-bach, that works for me.

Just to make sure I understand:

  1. type: string, in required list: field may not be missing, may not be null :: Text
  2. type: string, not in required list: field may be missing, may not be null :: Maybe Text
  3. type: string, nullable: true, not in required list: field may be missing, may be null :: Nullable (Maybe Text)
  4. type: string, nullable: true, in required list: field may not be missing, may be null :: Maybe Text

And what I received originally (anyOf(string, null)) is just not a valid specification?

pbrisbin commented 6 months ago

OK, I've gotten a bit further and can answer some of my own questions:

  1. type: string, nullable: true, in required list: field may not be missing, may be null :: Maybe Text

This ends up as Nullable Text -- same idea though, you avoid the double Nullable/Maybe. It would be nice if the library also generated a function like,

unNullable :: Nullable a -> Maybe a
unNullable = \case
  NonNull a -> Just a
  Null -> Nothing

I can already see myself writing that all over the place.

And what I received originally (anyOf(string, null)) is just not a valid specification?

Base on this Stack Overflow, it seems anyOf(string, null) is valid OpenAPI 3.1, while string + nullable is the equivalent OpenAPI 3.0 construction.

So, I'm satisfied with that explanation and (luckily) my specification is small enough that I can edit it by hand to downgrade it to the 3.0 style. But I'm curious, does this library plan to support 3.1?

joel-bach commented 6 months ago

Hey @pbrisbin I think you got it 👍

Correct, null is not a type in OpenAPI 3.0 whereas it is one in 3.1.

The inclusion of unNullable makes sense I think 👍

But I'm curious, does this library plan to support 3.1?

Personally, I do not have plans to support it atm. I am open for PRs adding support though.