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

Support for Specification Extensions #11 for v3.2.2 (with aeson 2 support) #43

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

Conversation

alexbiehl
Copy link

@alexbiehl alexbiehl commented Mar 24, 2022

This PR is a follow up from #42. Only change compared to #42 is the added support for Aeson 2.

@alexbiehl alexbiehl changed the title alex/aeson 2 support for extensions Support for Specification Extensions #11 for v3.2.2 (with aeson 2) Mar 24, 2022
@alexbiehl alexbiehl changed the title Support for Specification Extensions #11 for v3.2.2 (with aeson 2) Support for Specification Extensions #11 for v3.2.2 (with aeson 2 support) Mar 24, 2022
@maksbotan
Copy link
Collaborator

Wonderful! Now tests pass. I will take a look at the code shortly, thanks!

@@ -510,6 +541,8 @@ data Param = Param
-- the examples value SHALL override the example provided by the schema.
, _paramExamples :: InsOrdHashMap Text (Referenced Example)

, _paramExtensions :: SpecificationExtensions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing haddock here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping

@@ -57,6 +57,7 @@ makeFields ''Encoding
makeFields ''Example
makeFields ''Discriminator
makeFields ''Link
makeLenses ''SpecificationExtensions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe corresponding changes should be done to Optics.hs as well?

@@ -0,0 +1,47 @@
# This file was autogenerated by Stack.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be in the repo? I'm not even sure if current stack.yaml works at all, I use only cabal...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually just use cabal too nowadays, but if there's a stack.yaml I'll usually use stack.

https://stackoverflow.com/questions/64682066/should-stack-yaml-lock-be-checked-into-source-control

Supposedly yes, they should be committed.

@@ -1316,7 +1355,7 @@ instance ToJSON SecurityScheme where

instance ToJSON Schema where
toJSON = sopSwaggerGenericToJSONWithOpts $
mkSwaggerAesonOptions "schema" & saoSubObject ?~ "items"
mkSwaggerAesonOptions "schema" & saoSubObject .~ ["items", "extensions"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's possible to make extensions injection more implicit, like hardcode it into sopSwaggerGenericToJSON'' — check whether we have extensions in the type and then encode/decode it accordingly. What do you think?

With the current approach I fear we may forget to add this to saoSubObject for some of our types. On the other hand, it becomes more implicit, dunno if it's a good thing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this one can be left for future improvement

@alexbiehl
Copy link
Author

@PPKFS Do you want to carry out the requested changes or should I?

@maksbotan
Copy link
Collaborator

I've checked locally with our project that generated json does not change 👍
However servant-openapi3 and our internal library had to be updated a bit before they worked.

@alexbiehl
Copy link
Author

I can also confirm that the mechanism works for our tool for generating Haskell servers from OpenAPI specs: scarf-sh/tie@d08d9a0#diff-b435607ddb81678ffc30b4fefe883599027eb7e487b8c735ded197ef07947efdR17

@PPKFS
Copy link

PPKFS commented Mar 25, 2022

@PPKFS Do you want to carry out the requested changes or should I?

I'm happy to carry them out after work, unless you're already one step ahead of me :)

Also interesting to see we're not the only place with openapi->haskell server tools!

@PPKFS
Copy link

PPKFS commented Apr 13, 2022

Oops, very sorry that I offered to make the changes and then promptly forgot.
I've got the changes done here, though I'm realising I'm not well versed in github PRs...

https://github.com/PPKFS/openapi3/tree/alex/aeson-2-support-for-extensions

With regards to the point about more implicit injection of extensions, I agree, but I realised after a while of trying that I don't know enough about sop-generics to do it properly.

@alexbiehl
Copy link
Author

alexbiehl commented Apr 13, 2022

@PPKFS I think you can just open a new PR from your branch! We can then close this one in favour of the new one.

As to your second point, I agree it would be nice to have a more implicit injection but I hope we can postpone this kind of change to another contribution and get your work in ASAP.

@ersran9
Copy link

ersran9 commented Nov 18, 2022

Anything that can be done to merge this in? @alexbiehl @PPKFS

@alexbiehl
Copy link
Author

@ersran9 @PPKFS @maksbotan I think we should probably merge as is. I can report I am using this in Production for some time and am not missing anything. We can improve incrementally for sure.

Copy link
Collaborator

@maksbotan maksbotan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I've reviewed the wrong one first :)

There's a couple of comments, once they are addressed I'd be happy to merge this.

@@ -119,6 +119,7 @@ module Data.OpenApi (
-- ** Miscellaneous
MimeList(..),
URL(..),
SpecificationExtensions (..),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import Data.OpenApi.Internal.Utils
import Generics.SOP.TH (deriveGeneric)
import Data.Maybe (catMaybes)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one belongs in the first group

@@ -510,6 +541,8 @@ data Param = Param
-- the examples value SHALL override the example provided by the schema.
, _paramExamples :: InsOrdHashMap Text (Referenced Example)

, _paramExtensions :: SpecificationExtensions
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping

@@ -1316,7 +1355,7 @@ instance ToJSON SecurityScheme where

instance ToJSON Schema where
toJSON = sopSwaggerGenericToJSONWithOpts $
mkSwaggerAesonOptions "schema" & saoSubObject ?~ "items"
mkSwaggerAesonOptions "schema" & saoSubObject .~ ["items", "extensions"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this one can be left for future improvement

where
proxy = Proxy :: Proxy a

parseAdditionalField :: Object -> Pair -> Parser ()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a copy of this function on line 223 which is unused now, please remove

in pairs (pairsToSeries (opts ^. saoAdditionalPairs) <> ps)
where
proxy = Proxy :: Proxy a
defs = hcpure (Proxy :: Proxy AesonDefaultValue) defaultValue
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying my comment from #11:

Why is this different from (aesonDefaults proxy) in sopSwaggerGenericToEncoding?

@potocpav
Copy link

It would be amazing to get these improvements in. Is it possible to push it towards merging? 🙏 @alexbiehl @maksbotan

@maksbotan
Copy link
Collaborator

maksbotan commented Jan 14, 2023

Hi @potocpav!
As far I see, my latest review comments have not been addressed yet? Once they are, I'm happy to merge this. Also, there is a merge conflict in src/Data/OpenApi/Internal.hs.

@stepcut
Copy link

stepcut commented Jun 2, 2023

I would love to be able to use this patch. Is there anything I can do to help get it into the mainline?

@alexbiehl alexbiehl force-pushed the alex/aeson-2-support-for-extensions branch from 4793b3c to c2575df Compare May 28, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants