servant icon indicating copy to clipboard operation
servant copied to clipboard
verified publisher icon haskell-servant

Fix Optional ReqBody' (wrap value into Maybe)

Open unclechu opened this issue 5 years ago • 8 comments

Closes #1346

Here is a small reproducible demonstration of the fix (requires Nix to run): https://gist.github.com/unclechu/77ec314b8651c0c17ea4ac486fdb9804

ReqBody' '[Optional, Strict] '[JSON] [Integer]

Is resolving to [Integer] in server serializer. This fix makes it become Maybe [Integer] instead (because it has Optional mode).

This PR brings these changes:

  1. Makes Optional ReqBody' wrap it’s type into Maybe
  2. Request without headers and without payload resolves to Nothing
  3. Request with proper Content-Type but without payload resolves to Nothing
  4. When proper Content-Type header is provided and payload is there it resolves Just in case there were no parsing errors
  5. Required ReqBody' behaves as usual (not affected by this fix)

unclechu avatar Oct 04 '20 02:10 unclechu

I’m not certain about these two patterns:

(SFalse, STrue,  False) -> return . either (const Nothing) (Just . Right)
(SFalse, SFalse, False) -> return . either (const Nothing) Just

But they seem to work as expected. It turned ouit that requests from the tests are having requestBodyLength request as KnownLength 0. I’m not sure why.

unclechu avatar Oct 04 '20 02:10 unclechu

Hi!

Is there something keeping this from going upstream? I was under the impression this was the default behavior (that one would construct a custom type with '[Optional] to have optional request bodies like how Header works), but as pointed out that's not the case.

dalpd avatar Nov 29 '22 12:11 dalpd

Yep', there is a conflict to be resolved in this PR. :)

tchoutri avatar Nov 29 '22 13:11 tchoutri

@tchoutri I resolved the conflicts and rebased the change on top of latest master. Also I ran the tests locally with a successful result. Please have a look.

unclechu avatar Nov 29 '22 14:11 unclechu

Is there anything we can do to help maintainers move this PR forward?

dalpd avatar Dec 10 '22 09:12 dalpd

I somehow managed to miss the mention from 12 days ago. There could be a CI failure due to HsOpenSSL that I'm exploring in #1635, otherwise I don't have anything more to ask. :)

tchoutri avatar Dec 11 '22 13:12 tchoutri

@unclechu okay, I believe that you should bump the version of HsOpenSSL used, reading its changelog: https://flora.pm/packages/%40hackage/HsOpenSSL/0.11.7.5/changelog

tchoutri avatar Feb 24 '23 23:02 tchoutri

I'd like to see this PR merged - is there something I can do to help? I guess if I wanted to rebase it myself I'd have to open a separate PR?

isomorpheme avatar May 30 '23 12:05 isomorpheme