Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Optional ReqBody' (wrap value into Maybe) #1347

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

unclechu
Copy link
Contributor

@unclechu unclechu commented Oct 4, 2020

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
Copy link
Contributor Author

unclechu commented Oct 4, 2020

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.

@dalpd
Copy link

dalpd commented Nov 29, 2022

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.

@tchoutri
Copy link
Contributor

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

@unclechu
Copy link
Contributor Author

@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.

@dalpd
Copy link

dalpd commented Dec 10, 2022

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

@tchoutri
Copy link
Contributor

tchoutri commented Dec 11, 2022

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
Copy link
Contributor

@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

@isomorpheme
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional ReqBody does not wrap it’s type into Maybe
4 participants