Validating operation input

questions
1

I wonder how other people feel about validating the operation input. I recently wrote an operation like this:

module MyApp
  module Operations
    module Pages
      class Update < MyApp::Operation
        include Deps["repos.page_repo"]

        def call(id:, content:)
          transaction do
            page = get_page(id)
            step create_version(page.id, content)
            get_page(page.id)
          end
        end

        # ...

Now, it’s all fine until I accidentally passed nil as id. It silently passed through the repository and only manifested in “undefined method id for nil" in page.id. At this point though I thought that for some reason fetching from the database did not work and it took me a while to notice the actual problem.

Then several minutes later I accidentally passed nil as content (you can probably see a pattern of me not being a great coder lol).

This would be solved by input validation for operation. Do you think it’s a good idea? What would you use to implement it, given that the operation itself does not expose any mechanism for that?

2

Is this what you’re looking for, @katafrakt? Add Validation extension for input validation with Dry Validation by aaronmallen · Pull Request #34 · dry-rb/dry-operation

This will be in the next release, which should arrive in the next couple of weeks.

If you’ve been thinking about this stuff, I’m keen for your thoughts on that extension. If you think it should do anything differently, now’s the perfect time for feedback :slight_smile:

3

Ah, yes, I missed that.

Before seeing it, I was thinking about something much simpler, basically just a type-check.

module MyApp
  module Operations
    module Pages
      class Update < MyApp::Operation
        include Deps["repos.page_repo"]

        class Input < Dry::Struct
          attribute :id, MyApp::Types::Integer
          attribute :content, MyApp::Types::String
        end

        def call(id:, content:)
          input = Input.new(id:, content:)
          transaction do
            page = get_page(input.id)
            step create_version(page.id, input.content)
            get_page(page.id)
          end
        end

My reasoning was that Dry Validation is much more user-facing with its human-readable error messages etc. I was just looking for something that will blow up in my face when I’m doing something stupid as a programmer, for which this simple type check was fine.

I’ll take a closer look at this extension. IMO at the time of calling the operation, the user input should be already sanitized and errors from the operation should not be resurfaced to the user. Using Dry Validation in the operation layer might invite to actually defer the validation to the operation and to show operation errors to the user. Other people mileage might vary, but I personally try to avoid doing things like that.

4

Maybe we could add a “typed arguments” extension in addition to the validation one? It could follow some of the same general structure.

This way people could pick and choose whichever extension meets their needs.

1 Like
5

That’s an option for sure. But I would be slightly worries of introducing a new concept and immediately telling the user to choose between two approaches. Perhaps Dry Validation thing is better and more convenient for majority of users, and the typed one is actually really easy to roll on one’s own.

6

My general approach to this looks like

module MyApp
  module Operations
    module Pages
      class Update < MyApp::Operation
        include Deps["repos.page_repo"]

        def call(id:, content:)
          step Types::Integer.try(id)
          step Types::FilledString.try(content)

          transaction do
            page = get_page(id)
            step create_version(page.id, content)
            get_page(page.id)
          end
        end

        # ...

Except I use a helper method called either instead of step, which allows me to convert Failures into tuples with a name automatically. The result would end up looking like Failure[:type, #<Dry::Types::ConstraintError ...>, id]

I will likely adopt Aaron’s param validation DSL instead

2 Likes