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.

Sign up for GitHub

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

Jump to bottom

Merge OAuth2 scopes #151

Merged
merged 3 commits into from
Jan 27, 2020
Merged

Merge OAuth2 scopes #151

merged 3 commits into from
Jan 27, 2020

Conversation

taojang
Copy link
Contributor

@taojang taojang commented Mar 22, 2018 •
edited
Loading

The problem was when merging two swagger doc together, the OAuth2 scopes are not merged together properly because the Monoid InsOrdMap has behavior of replacing the value at the same key.

The tactical solution is to make a SwaggerMonoid overlapping instance for it. It only merges scopes when SecurityScheme

  • have same key in security definitions map
  • both are SecuritySchemeOAuth2
  • flows are same

@fizruk
Copy link
Member

fizruk commented Mar 22, 2018

Makes sense! Thank you for your contribution!

@phadej
Copy link
Collaborator

phadej commented Mar 22, 2018 via email

@taojang
Copy link
Contributor Author

taojang commented Mar 22, 2018

@phadej I'm fairly new to this library, I couldn't find the monoid instance of SecurityScheme. Have I overlooked something?

@taojang
Copy link
Contributor Author

taojang commented Apr 23, 2018 •
edited
Loading

@phadej I noticed master does not build with stack, so I guess my PR is not going to build on CI after rebasing.

I checked again that there's no instance for SecurityScheme. My humble opinion is rather not to have a semigroup instance for it, because:

  • it's not directly helping in this situation, because _swaggerSecurityDefinitions has type Definitions SecurityScheme which is alias to InsOrdMap Text SecurityScheme. By default it uses InsOrdMap.union as mappend which overwrites value with same key
  • such instance could give a false impression that it's ok to combine two SecurityScheme together. But in reality it only combines scopes together for oauth2 under certain condition. Otherwise it would throw one value away

What do you think? I can do everything to just get this merged 😁

@phadej
Copy link
Collaborator

phadej commented Apr 24, 2018 •
edited
Loading

I'm not a fan of overlapping instances.

@fizruk could we rather have newtype SecurityDefinitions = SecurityDefinition (Definitions SecurityScheme)

@phadej
Copy link
Collaborator

phadej commented Apr 24, 2018 •
edited
Loading

And thinking again, why it's ok to monoidally combine Definitions SecurityScheme, but not bare SecuritySchemes. Can @taojang open that up?

@taojang
Copy link
Contributor Author

taojang commented Apr 24, 2018

@phadej thx for pointing out the direction. I updated the implementation according to your suggestion

reactormonk
reactormonk reviewed Apr 25, 2018
View reviewed changes
src/Data/Swagger/Internal.hs
@@ -1104,6 +1120,9 @@ instance ToJSON PathItem where
instance ToJSON Example where
toJSON = toJSON . Map.mapKeys show . getExample

instance ToJSON SecurityDefinitions where
Copy link

Choose a reason for hiding this comment

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

You can derive these with GeneralizedNewtypeDeriving.

@fizruk
Copy link
Member

fizruk commented Jun 28, 2018

@phadej are you happy with this change? Should I merge?

@i-am-the-slime
Copy link

Is there any chance this will be merged one day?

@fisx
Copy link
Collaborator

fisx commented Sep 29, 2019

I've just received commit authority, and I will merge this if (1) nobody objects in time; and (2) you resolve the merge conflict.

Thanks!

@i-am-the-slime
Copy link

@taojang

@taojang
Copy link
Contributor Author

taojang commented Jan 8, 2020

Thanks for the heads-up! I will resolve it before next week :)

@taojang
Copy link
Contributor Author

taojang commented Jan 11, 2020

@fisx I have resolved the merge conflict

fisx
fisx approved these changes Jan 27, 2020
View reviewed changes
Copy link
Collaborator

@fisx fisx left a comment

Choose a reason for hiding this comment

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

I have (silly) remarks, but I will merge this now. Thanks!

src/Data/Swagger/Internal.hs

instance Semigroup SecurityDefinitions where
(SecurityDefinitions sd1) <> (SecurityDefinitions sd2) =
SecurityDefinitions $ InsOrdHashMap.unionWith (<>) sd1 sd2
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Shouldn't there be an instance Semigroup InsOrdHashMap? Same for Monoid?)

src/Data/Swagger/Internal.hs
SecurityDefinitions $ InsOrdHashMap.unionWith (<>) sd1 sd2

instance Monoid SecurityDefinitions where
mempty = SecurityDefinitions $ InsOrdHashMap.empty
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mempty = SecurityDefinitions $ InsOrdHashMap.empty
mempty = SecurityDefinitions InsOrdHashMap.empty

@fisx fisx merged commit 5470313 into GetShopTV:master Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reactormonk reactormonk reactormonk left review comments

@fizruk fizruk fizruk approved these changes

@fisx fisx fisx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@taojang @fizruk @phadej @i-am-the-slime @fisx @reactormonk