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

Operation validation errors don't get reported to client properly #101

Closed
catmando opened this issue Jan 10, 2019 · 7 comments · May be fixed by #304
Closed

Operation validation errors don't get reported to client properly #101

catmando opened this issue Jan 10, 2019 · 7 comments · May be fixed by #304
Labels
bug Something isn't working ready-to-release Internal Use Only: Has been fixed, specs passing and pushed to edge branch
Milestone

Comments

@catmando
Copy link
Contributor

reported by @psmir

When client side validation fails my promise receives the convenient Hyperstack::Operation::ValidationException. But when server side validation fails it receives plain Exception and I can't get errors hash from it. Moreover the server responds with code 500.
Is it possible to make Hyperstack to raise Hyperstack::Operation::ValidationException on the client side when server validation fails and not to respond with code 500 ?

@catmando catmando added the bug Something isn't working label Jan 10, 2019
@catmando catmando added this to the alpha1.3 milestone Jan 10, 2019
@psmir
Copy link
Contributor

psmir commented Jan 11, 2019

Same thing applies to abort! and failed { }

Hyperstack::Operation
  step { abort!({error: "some description"}) }
end

In the promise I receive an object with class Hyperstack::Operation::Exit
and can fetch the errors hash like this

.fail {|e| e.result } 

Hyperstack::ControllerOp responds with code 500

the promise receives plain Exception with the message {"error":"Hyperstack::Operation::Exit"}

Hyperstack::Operation
  step { fail }
  failed do
    {error: "some description"}
  end
end
# returns !!Hash !! {"error"=>"some description"}
#
.fail {|e| puts "!!#{e.class} !! #{e}" }

Hyperstack::ControllerOp responds with code 500 and the promise receives plain Exception with the message {"error"=>"some description"}

@catmando
Copy link
Contributor Author

catmando commented Jan 12, 2019

@psmir so just to confirm... the big problem here is that you resulting error type does not match the message type you got on the client?

@psmir
Copy link
Contributor

psmir commented Jan 12, 2019

@catmando Yes

  1. Validation, abort! and failed {} should behave identically both client and server side
    1.1 On validation error I would like to receive something similar to Hyperstack::Operation::ValidationException.
    1.2 On abort! invocation I would like to receive something similar to Hyperstack::Operation::Exit
    1.3 When I return some hash from failed {} I would like to receive it in my .fail {}
  2. Validation errors, abort! and failed {} makes server to respond with 500 error. The error 500 is a signal of application crash to me.

@catmando
Copy link
Contributor Author

catmando commented Jan 14, 2019

I think Validation errors should be a 400, authentication errors be a 403, and all other errors should stay as 500 errors:

500 Internal Server Error

A generic error message, given when an unexpected condition was encountered and no more specific message is suitable.

@catmando catmando added the ready-to-release Internal Use Only: Has been fixed, specs passing and pushed to edge branch label Jan 15, 2019
@psmir
Copy link
Contributor

psmir commented Jan 18, 2019

Thank you for the fix. I have tested it and found out that some inconsistencies are still there.

  1. Validation
class SignUpOp < Hyperstack::Operation
  param :email, type: String
  param :password, type: String
  param :password_confirmation, type: String
  
   validate do
     add_error(
       :password_confirmation,
       :doesnt_match,
       "Your new password and confirmation do not match"
     ) unless params.password == params.password_confirmation
   end
end

# In some component

SignUpOp.run(email: nil, password: 'aaa', password_confirmation: 'bbb')
.fail do |e|
  puts e.errors.class    # Mutations::ErrorHash
  puts e.errors.symbolic # {"email"=>"required", "password_confirmation"=>"doesnt_match"}
  puts e.errors.message  # {"email"=>"email is required", "password_confirmation"=>"Your new password and confirmation do not match"}
  
  # if I change the base operation class to Hyperstack::ControllerOp
  
  puts e.errors.class    # Mutations::ErrorArray (we need Mutations::ErrorHash here)
  puts e.errors.symbolic # required,doesnt_match
  puts e.errors.message  # email is required,Your new password and confirmation do not match  
                          
end
  1. abort!
class SignUpOp < Hyperstack::Operation

  step do 
    h = { some_key: 'aaa' }
    abort! h
  end

end

SignUpOp.run()
.fail do |e|
  puts e.class           # Hyperstack::Operation::Exit
  puts e.result          # {"some_key"=>"aaa"}
  puts e.result[:key]    # aaa
  
  # if I change the base operation class to Hyperstack::ControllerOp
  puts e.class           # Hyperstack::Operation::Exit
  puts e.result          # nothing
end
  1. failed {}
class SignUpOp < Hyperstack::Operation
  step { fail! }
  failed do
    { some_key: 'aaa' }
  end
end

SignUpOp.run()
.fail do |e|
  puts e.class       # Hash
  puts e             # {"some_key"=>"aaa"}
  puts e[:some_key]  # aaa
  
  # if I change the base operation class to Hyperstack::ControllerOp
  puts e.class       # Hash
  puts e             # {}
end

@psmir
Copy link
Contributor

psmir commented Jan 19, 2019

@catmando Can you look at my comment above, please?

@catmando
Copy link
Contributor Author

@psmir thanks for making this clear. I'll have a chance to work on this once I get this Rails Conf presentation sent in.

kvechera pushed a commit to kvechera/hyperstack that referenced this issue Mar 23, 2020
fixed hyperstack-org#101 when ServerOp transmits errors to the client
kvechera pushed a commit to kvechera/hyperstack that referenced this issue Mar 23, 2020
fixed hyperstack-org#101 when ServerOp transmits errors to the client
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready-to-release Internal Use Only: Has been fixed, specs passing and pushed to edge branch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants