Handling "required", but "nullable: true"

saurabhnanda commented 3 years ago

I'm trying to generate an API client for Hetzner's API (spec at https://docs.hetzner.cloud/spec.json ) and it has this strangeness where a property is required, but allowed to be nullable. This is causing almost all parsing of responses to fail, because the generated data-type is not a Maybe a, but simply a

At first I thought that the spec itself was broken, but apparently this is something that is to be expected in OpenAPI specs

How can this be handled in the code generator?

sorki commented 3 years ago

I've managed to hit this as well and came up with a patch, feel free to use / open a PR with if it helps.

https://github.com/sorki/Haskell-OpenAPI-Client-Code-Generator/commit/5d1ecf53bc6b9e44eaeab3b9df2ee4a1d7264df0

saurabhnanda commented 3 years ago

@sorki brilliant - thanks! Will cherry-pick your commit on the latest version of this package and try. Any reason for not raising a PR upstream?

sorki commented 3 years ago

Time :-) I've managed to get my generated API working with this patch but then decided I'll go servant route instead. It also felt a little obscure but it seems I'm not the only one having this problem.

saurabhnanda commented 3 years ago

@sorki what's the servant route? What's the alternative to this package to generate a haskell client from openapi spec? I tried https://openapi-generator.tech/docs/generators/haskell-http-client but it generate everything in just two modules, which take forever to compile!

sorki commented 3 years ago

@sorki what's the servant route? What's the alternative to this package to generate a haskell client from openapi spec? I tried https://openapi-generator.tech/docs/generators/haskell-http-client but it generate everything in just two modules, which take forever to compile!

Using servant to describe the API manually and then creating a client using the decription, instead of generating it. More tedious but the result can be better, depending on the target API.

saurabhnanda commented 3 years ago

Okay - I think the issue has another aspect.

when the server sends back a response, the field may be null
however, when making the request, either the field may be absent from the JSON, or if it is present it should not be null

How do I handle this?

joel-bach commented 3 years ago

Hi @saurabhnanda , Thank you for raising this issue! I currently see no downside to @sorki 's implementation for handling the response from the server and will try to integrate it later this week. As to sending requests: I think ideally the generated client code should handle required and nullable in the ToJSON instance as follows:

required and nullable: true: Nothing is serialised to null
required and nullable: false: Nothing should omit the property
not required and nullable: true: Nothing should omit the property

The third case is ambiguous and can lead to problems if a server implementation relies on the distinction between null and omitting the property (btw the same is true for the response handling as the absence of a property and a null value cannot be distinguished by the proposed implementation). But I think it is worth it to handle it like this to simplify the interface. What do you think @saurabhnanda ?

saurabhnanda commented 3 years ago

@joelfisch my thoughts on this topic:

For my current use-case it seems like the proposed solution _could work while encoding/sending the JSON. However, it might break for some other API/server, because of the inherent ambiguity in the way the spec has been written.

Similarly, while receiving, I don't think we can use Maybe a to distinguish between these three scenarios. We will need something like data OptionalJsonValue a = Absent | Null | Present a or a Maybe (Maybe a)

key & value present: Present a OR Just (Just a)
key absent: Absent OR Nothing
key present, but value = null: Null OR Just Nothing

Is it worth it to make both of these behaviours configurable using one of the two approaches:

User can pick between Maybe a, Maybe (Maybe a)), and OptionalJsonValue a
Or, during the code-gen can we accept an optional Haskell file containing a module with two functions: (or type-class instances): (a) one to decode an optional JSON value into a data-type of the user's choice, and (b) one to encode an optional JSON value into a data-type of the user's choice.

Since, I'm stuck with this problem right now, I might be able to take a stab at solving this problem if you could help me with where I should start reading the code from.

joel-bach commented 3 years ago

I think an additional ADT would be pretty neat but what I do not like is that it really only works for properties of objects (as only in this combination nullable and required can be used). Therefore I would propose that for schemas with nullable: true the type has to be wrapped in a Maybe. In the case of an optional property you would get your proposed Maybe (Maybe a). I think the main challenge is that you would have to create a non-nullable type and then a type alias, e. g.:

type X = Maybe XNonNullable
data XNonNullable = XNonNullable { propX :: Text, propY :: Text }

This is necessary to remove the need that referencing placing need to know about the nullability. It would probably make sense to create a newtype and adjust the ToJSON instance to ensure it is serialized to null. What do you think?

As to an entry point: I would suggest looking at Model.hs (e. g. https://github.com/Haskell-OpenAPI-Code-Generator/Haskell-OpenAPI-Client-Code-Generator/blob/master/openapi3-code-generator/src/OpenAPI/Generate/Model.hs#L179) where all the model generation is done.

stepcut commented 2 years ago

I have run into the common situation where fields in a request are optional and not-nullable. That is the default state for a field if it is not explicitly marked required or nullable. However, the generator currently sends Nothing values as null instead of omitting the property entirely. The mailchimp API rejects the requests -- which it should since the fields are not nullable.

I do think it is critical to be able to distinguish between absent and null. If you are making a POST request to update a value on the server then when a field is absent, the server may choose to leave the field unchanged. But if it is set to null then it make choose to clear the field. Those are obviously two very different operations. (As a side note, that mean for strings absent, null and present but the empty string "" mean three different things. The openapi spec notes that null and "" are unique).

It is not clear to me that Maybe (Maybe a) is sufficient to uniquely identify the 4 states,

required, notnullable  :: a
required, nullable     :: Maybe a
optional, nullable     :: Maybe (Maybe a)
optional, not-nullable :: ??? -- Maybe a would conflict with required, nullable

It is also tricky to remember if null is Just Nothing or Nothing. And also annoying that what constructors you even use are dependent on if a value is optional or nullable.

I think there is a nice-consistency and clarity to a type like,

-- | The `Null` constructor can only be used when the property is `Nullable`, and `Absent` only used when the property is `Optional`
data Property (required :: IsRequired) (nullable :: IsNullable) a where
  Present :: a -> Property required nullable a
  Absent  ::      Property Optional nullable a
  Null    ::      Property required Nullable a

This has a clear mapping to the 4 possible states:

required, notnullable  :: Property Required NonNullable a
required, nullable     :: Property Required Nullable a
optional, nullable     :: Property Optional Nullable a
optional, not-nullable :: Property Optional NonNullable a

Here is a full working example:

{-# language GADTs #-}
{-# language DataKinds #-}
{-# language KindSignatures #-}
{-# language StandaloneDeriving #-}
module Main where

import Data.Maybe (catMaybes)

-- | a simple type for indicating if a field is nullable or not. Used only at the type-level.
data IsNullable
  = Nullable
  | NonNullable
 deriving (Eq, Ord, Read, Show)

-- | a simple type for indicating if a field is required or not. Used only at the type-level.
data IsRequired
  = Required
  | Optional
 deriving (Eq, Ord, Read, Show)

-- | The `Null` constructor can only be used when the property is `Nullable`, and `Absent` only used when the property is `Optional`
data Property (required :: IsRequired) (nullable :: IsNullable) a where
  Present :: a -> Property required nullable a
  Absent  ::      Property Optional nullable a
  Null    ::      Property required Nullable a
deriving instance (Eq a)   => Eq   (Property required nullable a)
deriving instance (Ord a)  => Ord  (Property required nullable a)
deriving instance (Show a) => Show (Property required nullable a)

-- A potential warning sign is that this can not be derived automatically.
-- The trouble stems from what happens when `Null` is encountered but the return type is not `NotNullable`.
-- deriving instance (Read a) => Read (Property required nullable a)

-- | a sample record with some fields that are required, some that are optional and non-nullable, and some which are 
data MyRecord = MyRecord
 { requiredProperty1            :: Property Required NonNullable String
 , requiredProperty2            :: Property Required Nullable    String
 , optionalNonNullableProperty1 :: Property Optional NonNullable String
 , optionalNonNullableProperty2 :: Property Optional NonNullable String
 , optionalNullableProperty1    :: Property Optional Nullable    String
 , optionalNullableProperty2    :: Property Optional Nullable    String
 , optionalNullableProperty3    :: Property Optional Nullable    String
 }
 deriving (Eq, Ord, Show)

-- | a simple function to render a Property which handles optional vs nullable fields correctly
renderProperty :: (Show a) => String -> Property required nullable a -> Maybe String
renderProperty n a =
  case a of
    Absent    -> Nothing
    Present a -> Just $ n ++ " = " ++ show a
    Null      -> Just $ n ++ " = null"

-- | a simple function to render 'MyRecord' handling optional and nullable fields correctly
renderRecord :: MyRecord -> String
renderRecord r =
  "{" ++
  (concatMap ("\n, " ++) (catMaybes [ renderProperty "requiredProperty1"            (requiredProperty1 r)
                                    , renderProperty "requiredProperty2"            (requiredProperty2 r)
                                    , renderProperty "optionalNonNullableProperty1" (optionalNonNullableProperty1 r)
                                    , renderProperty "optionalNonNullableProperty2" (optionalNonNullableProperty2 r)
                                    , renderProperty "optionalNullableProperty1"    (optionalNullableProperty1 r)
                                    , renderProperty "optionalNullableProperty2"    (optionalNullableProperty2 r)
                                    , renderProperty "optionalNullableProperty3"    (optionalNullableProperty3 r)
                                    ])) ++
  "\n}"

rec1 :: MyRecord
rec1 = MyRecord
  { requiredProperty1             = Present "required"
  , requiredProperty2             = Null
  , optionalNonNullableProperty1  = Absent
  , optionalNonNullableProperty2  = Present "optionalNonNullableProperty2"
  , optionalNullableProperty1     = Null
  , optionalNullableProperty2     = Absent
  , optionalNullableProperty3     = Present "optionalNullableProperty3"
  }

{-
If we attempt to set the non-nullable fields to Null or required fields to Absent we get errors like this:
[1 of 1] Compiling Main             ( Nullable.hs, interpreted )

Nullable.hs:89:33: error:
    • Couldn't match type ‘'Optional’ with ‘'Required’
      Expected type: Property 'Required 'NonNullable String
        Actual type: Property 'Optional 'NonNullable String
    • In the ‘requiredProperty1’ field of a record
      In the expression:
        MyRecord
          {requiredProperty1 = Absent, requiredProperty2 = Null,
           optionalNonNullableProperty1 = Null,
           optionalNonNullableProperty2 = Present "optionalNonNullableProperty2",
           optionalNullableProperty1 = Null, optionalNullableProperty2 = Absent,
           optionalNullableProperty3 = Present "optionalNullableProperty3"}
      In an equation for ‘rec2’:
          rec2
            = MyRecord
                {requiredProperty1 = Absent, requiredProperty2 = Null,
                 optionalNonNullableProperty1 = Null,
                 optionalNonNullableProperty2 = Present "optionalNonNullableProperty2",
                 optionalNullableProperty1 = Null, optionalNullableProperty2 = Absent,
                 optionalNullableProperty3 = Present "optionalNullableProperty3"}
   |
89 |   { requiredProperty1            = Absent
   |                                 ^^^^^^

Nullable.hs:91:33: error:
    • Couldn't match type ‘'Nullable’ with ‘'NonNullable’
      Expected type: Property 'Optional 'NonNullable String
        Actual type: Property 'Optional 'Nullable String
    • In the ‘optionalNonNullableProperty1’ field of a record
      In the expression:
        MyRecord
          {requiredProperty1 = Absent, requiredProperty2 = Null,
           optionalNonNullableProperty1 = Null,
           optionalNonNullableProperty2 = Present "optionalNonNullableProperty2",
           optionalNullableProperty1 = Null, optionalNullableProperty2 = Absent,
           optionalNullableProperty3 = Present "optionalNullableProperty3"}
      In an equation for ‘rec2’:
          rec2
            = MyRecord
                {requiredProperty1 = Absent, requiredProperty2 = Null,
                 optionalNonNullableProperty1 = Null,
                 optionalNonNullableProperty2 = Present "optionalNonNullableProperty2",
                 optionalNullableProperty1 = Null, optionalNullableProperty2 = Absent,
                 optionalNullableProperty3 = Present "optionalNullableProperty3"}
   |
91 |   , optionalNonNullableProperty1 = Null
   |                                 ^^^^
Failed, no modules loaded.

rec2 :: MyRecord
rec2 = MyRecord
  { requiredProperty1            = Absent
  , requiredProperty2            = Null
  , optionalNonNullableProperty1 = Null
  , optionalNonNullableProperty2 = Present "optionalNonNullableProperty2"
  , optionalNullableProperty1    = Null
  , optionalNullableProperty2    = Absent
  , optionalNullableProperty3    = Present "optionalNullableProperty3"
  }
-}

main :: IO ()
main = putStrLn $ renderRecord rec1

joel-bach commented 2 years ago

Hey there, thank you for your input! I really like this approach but there is one flaw in my opinion which is that nullable is possible on every schema whereas optional properties is only part of type: object. This led me to the conclusion that two more or less separate solutions would be the more generic approach to implement it. I created a PR https://github.com/Haskell-OpenAPI-Code-Generator/Haskell-OpenAPI-Client-Code-Generator/pull/75 which aims to add support for nullable and also fix serialization (thanks to @s9gf4ult for the idea in https://github.com/Haskell-OpenAPI-Code-Generator/Haskell-OpenAPI-Client-Code-Generator/pull/73) / deserialization of optional fields (omitting them instead of turning to null values and do not parse null as empty fields).

I will merge this PR soon, please shout if you think there is something missing/wrong about the approach. I will close this issue afterwards as I believe it is solved with it (let me know if you think I am wrong about this).

stepcut commented 2 years ago

I am happy with any solution that works.

For schemas that do not allow optional then with the Property type would always be Property Required nullable. However, if it is fairly common that a value is required and non-nullable, then you have the extra overhead of constructing and matching on the Present constructor when a simple value would due just fine.

It was fun to implement the above code, but I do have questions about how much tedium it would add compared to other approaches.

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

Handling "required", but "nullable: true" #55