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

In SCIM handlers, format (almost) all errors using SCIM error schema #575

Merged
merged 4 commits into from
Apr 9, 2019
Merged

In SCIM handlers, format (almost) all errors using SCIM error schema #575

merged 4 commits into from
Apr 9, 2019

Conversation

neongreen
Copy link
Contributor

No description provided.

@neongreen neongreen requested a review from fisx January 22, 2019 19:08
@neongreen neongreen force-pushed the scim-spar-errors branch 2 times, most recently from 829f41c to 553bc77 Compare January 25, 2019 11:12
fisx
fisx reviewed Feb 5, 2019
View reviewed changes
services/spar/src/Spar/Scim.hs Outdated Show resolved Hide resolved
services/spar/src/Spar/Scim.hs Outdated Show resolved Hide resolved
services/spar/src/Spar/Scim.hs Outdated
-- SCIM.serverError and rethrown.
Right (Left sparError) ->
Left . SAML.CustomServant . Scim.scimToServantErr $
Scim.serverError (cs (errBody (sparToServantErr sparError)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just Left sparError? Then it will be converted later on in sparToServantErr like before.

Come to think of it, I would like to add a constructor ... | SparScimError ScimError to SparCustomError, change toSpar as follows, and leave the conversion to ServantErr to the Error module:

toSpar = Scim.fromScimHandler (throwError . SparScimError)

We will still have this weird construct where the saml library wraps all errors, even those that have nothing to do with saml, but it keeps the errors in the app logic more strongly typed, and moves the burdon of rendering them more cleanly to the Error module.

Copy link
Contributor

Choose a reason for hiding this comment

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

(do you want me to do that?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm okay with the constructor, as long as the /scim/v2 branch uses the SCIM format for all errors (as the spec requires). We also should take care to not wrap SCIM errors twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed. let me know if you want me to do any of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will get around to it at some point but I'm not sure when.

@neongreen neongreen changed the title In SCIM handlers, format (almost) all errors using SCIM error schema [WIP] In SCIM handlers, format (almost) all errors using SCIM error schema Mar 7, 2019
@neongreen neongreen changed the title [WIP] In SCIM handlers, format (almost) all errors using SCIM error schema [WIP] [blocked on hscim] In SCIM handlers, format (almost) all errors using SCIM error schema Mar 18, 2019
@neongreen
Copy link
Contributor Author

I'll return to this PR once #698 is merged.

@neongreen neongreen force-pushed the scim-spar-errors branch 2 times, most recently from dc0d057 to e0bed1e Compare April 8, 2019 15:39
@neongreen neongreen changed the title [WIP] [blocked on hscim] In SCIM handlers, format (almost) all errors using SCIM error schema [WIP] In SCIM handlers, format (almost) all errors using SCIM error schema Apr 8, 2019
@neongreen neongreen changed the title [WIP] In SCIM handlers, format (almost) all errors using SCIM error schema In SCIM handlers, format (almost) all errors using SCIM error schema Apr 8, 2019
@neongreen neongreen requested a review from fisx April 8, 2019 16:49
fisx
fisx approved these changes Apr 9, 2019
View reviewed changes
Copy link
Contributor

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

LGTM! Can go in as is if you don't like my changes.

services/spar/src/Spar/Scim.hs
-- properly. See <https://github.com/haskell-servant/servant/issues/1022>
-- for why it's hard to catch impure exceptions.
wrapScimErrors :: Spar a -> Spar a
wrapScimErrors = over _Spar $ \act -> \env -> do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
wrapScimErrors = over _Spar $ \act -> \env -> do
wrapScimErrors = over _Spar $ \act env -> do

services/spar/src/Spar/Scim.hs Outdated Show resolved Hide resolved
services/spar/src/Spar/Scim.hs Outdated Show resolved Hide resolved
fisx and others added 2 commits April 9, 2019 13:47
@fisx
Update services/spar/src/Spar/Scim.hs
0ed4b47 
Co-Authored-By: neongreen <yom@artyom.me>
@fisx
Update services/spar/src/Spar/Scim.hs
4531d7d 
Co-Authored-By: neongreen <yom@artyom.me>
@fisx
Copy link
Contributor

fisx commented Apr 9, 2019

sorry about the glitch! :/

@neongreen neongreen merged commit da6d80c into develop Apr 9, 2019
@neongreen neongreen deleted the scim-spar-errors branch April 9, 2019 15:04
@fisx fisx mentioned this pull request May 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.
2 participants
@neongreen @fisx