Standardize api v1 query param handeling

[mickenorlen]

What? Why?

Disclaimers: This is just a draft, the code doesn't work

I felt like we could use some more standardized query param handeling Api::V1::QueryParam. The work quickly outgrew the scope of the issue i was working on. And I wanted to check what you think before continuing.

Please let me know if you like this idea or have other preferred ways of working.

Benefits:

• Standardized validations, error handeling, and type conversion to appropriate rails format
• Get nice overview of what query_params are expected in each action.
• Reduce need of repeated testing for controller actions. Just need to check that QueryParam.new has been called with correct params, then rest of tests target QueryParam functionality.
• Bonus. rswag like syntax so we can put a similar statement in the controller action and its corresponding spec.

Syntax

class MyController
  def index
    query_params = Api::V1::QueryParam.new(params, {
      some_boolean: :boolean,  # shorthand when using default opts
      "some_datetime_arr[]": { type: :array, items: { type: datetime } }
    })
    
    # Might be able to avoid this line if response can be rendered from within QueryParam somehow...
    query_params.valid? || render query_params.json_response
    
    parsed_params = query_params.get  # get all
    some_boolean = query_params.get(:some_boolean)  # get single
  end
end
 

Structure

app/models/
├── api
│   └── v1
│       ├── query_param
│       │   ├── error.rb
│       │   ├── option.rb
│       │   ├── type.rb
│       │   └── types
│       │       ├── array.rb
│       │       ├── boolean.rb
│       │       ├── date_time.rb
│       │       └── hash.rb
│       └── query_param.rb

[mkllnk]

This is a cool concept. I like the idea of standardising the param handling in the API. There's some functionality in ActionController::Parameters already. Maybe we want to build on that with another gem? https://github.com/zendesk/stronger_parameters

You got a lot of code there and we don't have much APIv1 code yet. We probably should wait until we can reduce code by implementing something similar to your draft here. Building it now could mean that we waste effort and our requirements may change.

I'm also wondering if the QueryParam object is trying to do too much. Input parsing and generating a JSON result. That sounds like the whole API could live in there. That would be too much. The result should probably be generated somewhere else.

So I like your thinking but I would go for a much simpler solution for now. We should probably have at least two endpoints before we can see patterns emerging.

[mickenorlen]

Thank you for your feedback.

I definitely respect your point to keep things simple for now as this probably isn't on top of anyone's todo list. Although it hurts just a little bit to make temporary solutions haha.

As i started from small beginnings, I didn't really think about using a gem! Maybe that one could be a good solution. I guess we would have to investigate it a bit more if we decide to move forward with this more seriously.

I personally don't think handling the validation/errors is doing too much, but rather the main point of what we want to do here, the conversion is more of a bonus. I also felt our requirements to handle query param types are clear.

Also, that the api is in its infancy can also be seen as a good reason to get this done right from the beginning instead of building a refactoring dept and writing unnecessary tests.

[mkllnk]

I get your point. It's good to get things right to start with. In this case thought, we have hardly any code to refactor yet. Someone just shared this one in the last week:

[mickenorlen]

haha I definitely relate to over-engineering at times, at least i did this one on my own free time 😄 I think you are a very wise man and you can close this one for now if you want.

If we get back to this later however.. I've learned that, if you're inside a controller action, rendering an error inside a nested method call does not return from the action itself which causes: AbstractController::DoubleRenderError (Render and/or redirect were called multiple times in this action.

Solution is placing validation in a before_action. And indeed in the stronger params gem, they also put the param statement outside of the action. Also they are wise haha

class MyController
  permitted_parameters :show, id: Parameters.integer
  def show
  end
end

[mkllnk]

Good learning. Thanks for bringing this up. Always good to explore different ways.