RequiredParam results in 500

[gregwebs]

I am using RequiredParam

use Goliath::Rack::Validation::RequiredParam, key: param

When I run my specs or try a url in development mode, I get a 500 error. Internally, the middleware is raising a Goliath 400

raise Goliath::Validation::Error.new(400, "#{@type} identifier missing")

It seems like the exception is getting translated to a 500 instead of a 400. Also, a 400 is supposed to indicate bad syntax. I think a
422 Unprocessable Entity is more appropriate, which indicates:

The request was well-formed but was unable to be followed due to semantic errors

[dj2]

For the 500 error do you have

use Goliath::Rack::Params
use Goliath::Rack::ValidationError

before the validation middleware?

[gregwebs]

thanks, adding Goliath::Rack::ValidationError fixes it. I think this ticket should be left open to note that this dependency should be addressed in an automatic way.

[dj2]

I think changing the response to a 422 makes sense. I don't think auto-injecting Goliath::Rack::ValidationError necessarily makes sense as, you could write your own validation middleware that logs to a database or sends an email, or does any number of things. I don't think we can assume that people always want ValidationError.

[gregwebs]

I think the default should be to inject the dependency. If more flexibility is required a user can add a parameter to the use statement to indicate that they have a custom technique of dealing with errors.

Perhaps exception handling is not the best default either. Why can't the middleware just return a 4xx response be default? That way there is no dependency required. An upstream middleware can still recognize errors and deal with them as desired.

[igrigorik]

I agree, this has burned me before as well.

Since existing validators are Goliath specific, custom handling of those errors seems like overkill (yagni). ValidationError should either be auto-included by default, or, deleted alltogether and merged into the actual validators.

[dj2]

Is this along the lines of what you're thinking: https://github.com/postrank-labs/goliath/compare/validation

[gregwebs]

That looks great! And it is still easy enough to override for custom handling or throwing exceptions back to a custom handler. The only problem left is executing the switch to the new api. If i understand things correctly, everything will still just work, but can a warning be given when the old middleware is used now? Or do we just want to add documentation somewhere?

[igrigorik]

+1, that looks much better. I wish there was a clear rule to distinguish the two.. but I would have made it a mixin. Half a dozen of one, and half a dozen of the other...

I wouldn't be too worried about changing the API at this point. Still a young project, we can afford to make some breaking changes before we get bogged down in legacy support. :-)

[gregwebs]

My rule of thumb is if the choice is even (and if you can choose), then always use a module. There is really no difference between the 2 anyways- a module behaves exactly like a superclass. But you can only inherit from one class, so you might as well guard that until it provides a benefit. Definitely not worth the effort of changing.

[dj2]

Ok, branch is merged into master. I converted to a module. I wasn't sure which I wanted when I started and the module ended up working better.

Let me know if anything seems wonky.