brandonchinn178 / aeson-schemas

Easily consume JSON data on-demand with type-safety
http://hackage.haskell.org/package/aeson-schemas
BSD 3-Clause "New" or "Revised" License
51 stars 1 forks source link

Break up `HasSchemaResult` to not require all constraints when only one is needed #74

Open dewey92 opened 3 years ago

dewey92 commented 3 years ago

Describe the bug

First of all, thanks for the amazing plugin 🙂 I just started picking up Haskell for my pet project so my knowledge is pretty limited. So to my understanding, this constraint https://github.com/LeapYear/aeson-schemas/blob/70fc7e2158c4dc2626cbc4ead5998bab126c3ac0/src/Data/Aeson/Schema/Internal.hs#L178 will make sure that every type used in the schema QuasiQuote should always have both FromJSON and ToJSON instance. I think this can be a bit problematic if, for instance, I have a password type that can be parsed but shouldn't be sent over the wire:

data Hash = Plain | Hashed

newtype Password (hash :: Hash) = Password Text

mkPlainPassword :: Text -> Either Text (Password 'Plain)
mkPlainPassword pw
  | T.length pw >= 8 = pure $ Password pw
  | otherwise = Left "Password should be at least 8 chars"

instance FromJSON (Password 'Plain) where
  parseJSON rawPw = do
    pw <- parseJSON rawPw
    case mkPlainPassword pw of
      Right p -> pure p
      Left e -> fail $ T.unpack e

type PasswordPlain = Password 'Plain
type PostNewUser =
  Object
    [schema|
  {
    username: Text,
    email: Text,
    password: PasswordPlain,
    fullname: Text,
  }
|]

At least as far as Servant is concerned, I got a type error saying that Password 'Plain should have ToJSON instance, while PostNewUser is only consumed as a request body and not as a response.

data User = ...

instance ToJSON User where
  toJSON u =
    object
      [ "username" .= user_username u
      , "email" .= user_email u
      , "fullname" .= user_fullname u
      ]

type SignUpAPI = ReqBody '[JSON] PostNewUser :> Post '[JSON] User

server :: Server SignupAPI
server = ...

-- | Error
-- 
--   No instance for (ToJSON (Password 'Plain)) arising from a use of `serve`
app = serve (Proxy @SignUpAPI) server

I'm not sure if this is Servant's specific problem or not, but perhaps you can help me give a clue

Expected behavior

I should be able to define a custom type without having ToJSON instance.

brandon-leapyear commented 3 years ago

:sparkles: This is an old work account. Please reference @brandonchinn178 for all future communication :sparkles:


Sure, that's valid! It would require a lot of refactoring, though, so it could take a while to implement.

Personally, I would recommend using plain Text anyway. JSON is a language for communicating over the wire, and IMO things like validation logic don't belong in JSON serialization/deserialization code. Even without this bug, I think this would be a better design anyway:

type Payload = Object [schema|
  { password: Text }
|]

myHandler :: ReqBody Payload -> Handler ...
myHandler obj = do
  validatedPassword <- validatePassword [get|obj.password|]
  foo validatedPassword

But if you still really want PasswordPlain in the schema, you can work around for now by implementing a dummy ToJSON instance

On Sun, Jun 20, 2021, 2:26 AM Jihad D. Waspada @.***> wrote:

Describe the bug

First of all, thanks for the amazing plugin 🙂 I just started picking up Haskell for my pet project so my knowledge is pretty limited. So to my understanding, this constraint https://github.com/LeapYear/aeson-schemas/blob/70fc7e2158c4dc2626cbc4ead5998bab126c3ac0/src/Data/Aeson/Schema/Internal.hs#L178 will make sure that every type used in the schema QuasiQuote should always have both FromJSON and ToJSON instance. I think this can be a bit problematic if, for instance, I have a password type that can be parsed but shouldn't be sent over the wire:

data Hash = Plain | Hashed

newtype Password (hash :: Hash) = Password Text

mkPlainPassword :: Text -> Either Text (Password 'Plain)

mkPlainPassword pw

| T.length pw >= 8 = pure $ Password pw

| otherwise = Left "Password should be at least 8 chars"

instance FromJSON (Password 'Plain) where

parseJSON rawPw = do

pw <- parseJSON rawPw

case mkPlainPassword pw of

  Right p -> pure p

  Left e -> fail $ T.unpack e

type PasswordPlain = Password 'Plain type PostNewUser =

Object

[schema|

{

username: Text,

email: Text,

password: PasswordPlain,

fullname: Text,

} |]

At least as far as Servant is concerned, I got a type error saying that Password 'Plain should have ToJSON instance, while PostNewUser is only consumed as a request body and not as a response. Expected behavior

I should be able to define a custom type without having ToJSON instance.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/LeapYear/aeson-schemas/issues/74, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGUC75IX6JQ35GZN3XXQVBLTTWX4XANCNFSM4674YLSQ .

dewey92 commented 3 years ago

Thanks for the answer. From the resources read, it seems to me that many do request validation in FromJSON instance body, so that's why I do it that way. Perhaps because aeson already does a good job at parsing JSON types and allows us to convert them into user defined types. Anyway, good to know that user defined types indeed require at least both FromJSON and ToJSON instance 🙂

So, would you call it a bug? Or shall I close the issue?

brandon-leapyear commented 3 years ago

:sparkles: This is an old work account. Please reference @brandonchinn178 for all future communication :sparkles:


Perhaps because aeson already does a good job at parsing JSON types and allows us to convert them into user defined types.

aeson is good at parsing JSON, but IMO it should only be used to get JSON into haskell-land, with not much logic in it. But that's just my preference.

So, would you call it a bug? Or shall I close the issue?

I would call this a feature request. It's working as intended, but it would be nice to not require all constraints.