Apipie / apipie-rails

Ruby on Rails API documentation tool
Apache License 2.0
2.46k stars 457 forks source link

Apiepie::ParamErrors Inconsistent interface #890

Open wozza35 opened 1 year ago

wozza35 commented 1 year ago

It is suggested in the documentation to rescue both ParamMissing and ParamInvalid exceptions by rescuing from the Apipie::ParamError superclass. However, the interface of these two sub-classes is different.

For example, I'm trying to return a custom error JSON message in the case of a param error:

rescue_from Apipie::ParamError do |e|
  render json: {e.param => 'has an error'}, status: :unprocessable_entity
end

api :GET, '/foo'
param :arg, Array, required: true
def foo
  ...

ParamMissing#param returns an instance of Apipie::ParamDescription, whereas ParamInvalid#param returns a symbol. So, If the parameter is not an Array:

{
"arg": "has an error"
}

If the parameter is missing:

  {
"ParamDescription: site#foo#arg": "has an error"
}

I realize I can rescue the error classes separately or make use of meta programming, but it seems like this might be unintentional behaviour?

mathieujobin commented 1 year ago

I am thinking this might have happened due to a refactor. probably best to update the documentation, unless we're really missing out on something...

davidwessman commented 11 months ago

Might be related to my change with returning multiple errors, so either we need to reconsider that change or update the documentation.

https://github.com/Apipie/apipie-rails/pull/886

mathieujobin commented 11 months ago

@davidwessman Can you update the documentation ?

davidwessman commented 11 months ago

EDIT Now I realized the point about the different interfaces of param for ParamMissing vs ParamInvalid. So my answer was probably not on point - sorry.

Previous answer @mathieujobin I reviewed the README and find that it has been updated to this syntax: ```ruby # ParamError is superclass of ParamMissing, ParamInvalid rescue_from Apipie::ParamError do |e| render text: e.message, status: :unprocessable_entity end ``` using `e.message` instead of `e.param`. ------- @wozza35 Where did you find the `e.param` example? ------ This is my suggestion to handle all the errors, but I am not sure if it should be in the README or not. ```ruby rescue_from Apipie::ParamError do |e| errors = if e.is_a?(Apipie::ParamMultipleMissing) e.params.to_h {|p| [p.name, "has an error"]} else {e.param => "has an error"} end render json: errors, status: :unprocessable_entity end ```
davidwessman commented 11 months ago

So it seems like these errors have always used different interfaces for the params.

ParamInvalid is initialized like: https://github.com/Apipie/apipie-rails/blob/7cc859ec4f5727e08c6ebeab47c9a90220e57623/lib/apipie/validator.rb#L75-L77

ParamMissing is initialized like:

https://github.com/Apipie/apipie-rails/blob/7cc859ec4f5727e08c6ebeab47c9a90220e57623/lib/apipie/validator.rb#L41-L49

https://github.com/Apipie/apipie-rails/blob/7cc859ec4f5727e08c6ebeab47c9a90220e57623/lib/apipie/validator.rb#L359-L368

So it seems like it would make sense the change the ParamInvalid error to receive the whole ParamDescription as well. But that seems like breaking change :/

mathieujobin commented 11 months ago

Inconsistent interface isn't ideal, I convey, but considering the little audience of this gem. I'm not sure its worth fixing.

I guess if enough people vote that its worth it. otherwise, I think this is really minor. or at least, I don't see what exact problem is causes.