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

Switch from virtus to dry-types #1347

Closed
guizmaii opened this issue Apr 5, 2016 · 9 comments · Fixed by #1920
Closed

Switch from virtus to dry-types #1347

guizmaii opened this issue Apr 5, 2016 · 9 comments · Fixed by #1920

Comments

@guizmaii
Copy link

guizmaii commented Apr 5, 2016

I don't really know what you do with virtus (I think it's coercion) but I think it could be a good idea to replace virtus by dry-types (http://dry-rb.org/gems/dry-types/) for many reasons:

  1. This reddit post from Solnic, the virtus creator: https://www.reddit.com/r/ruby/comments/3sjb24/virtus_to_be_abandoned_by_its_creator/
  2. Performance (from the dry-types doc): "roughly 10-12x faster than Virtus"
@dblock
Copy link
Member

dblock commented Apr 5, 2016

Thanks for sharing this @guizmaii! I read the post with great interest. I would be open to a PR that replaces Virtus with something else, we can look at the implications.

@solnic
Copy link

solnic commented Apr 11, 2016

Happy to see this proposal. I was also wondering if maybe you could leverage dry-validation dry-shema too.

@totothink
Copy link

Very good, if possible, I want to get involved @dblock

@dblock
Copy link
Member

dblock commented Apr 19, 2016

Looking forward to pull requests @totothink.

@nbulaj
Copy link
Contributor

nbulaj commented Feb 15, 2019

Looks like this enhancement requires a lot of refactoring. Grape tied on Axiom::Types and Virtus coercion logic. It instantiates Virtus::Attribute objects, which I don't sure have Dry::Types equivalent. So to achieve success for this issue we need to completely refactor Grape::Validations module and it's ancestors.

@solnic
Copy link

solnic commented Feb 15, 2019

require 'dry/schema'

module Validation
  class DSL
    attr_reader :schema

    OPTS_MAPPING = {
      regexp: -> v { { format?: v } },
      type: -> v { v.name.downcase.to_sym }
    }

    def initialize
      @schema = Dry::Schema::DSL.new(processor_type: Dry::Schema::Params)
    end

    def call
      schema.call
    end

    def requires(name, opts)
      schema.required(name).value(*convert_opts(opts))
      self
    end

    def optional(name, opts)
      schema.optional(name).value(*convert_opts(opts))
      self
    end

    private

    def convert_opts(opts)
      opts.each_with_object([]) do |(k, v), arr|
        arg = OPTS_MAPPING.fetch(k).(v)
        arr << arg
      end
    end
  end

  def params(&block)
    @schema =
      begin
        dsl = DSL.new
        dsl.instance_eval(&block)
        dsl.call
      end
  end

  def schema
    @schema
  end
end

class Grape
  extend Validation

  params do
    requires :id, type: Integer
    optional :text, type: String, regexp: /\A[a-z]+\z/
  end
end

puts Grape.schema.(id: '1', text: '123').inspect
#<Dry::Schema::Result{:id=>1, :text=>"123"} errors={:text=>["is in invalid format"]}>

puts Grape.schema.(id: '1', text: 'foo').inspect
#<Dry::Schema::Result{:id=>1, :text=>"foo"} errors={}>

👋 so I just wrote this, as a quick PoC. This uses dry-types under the hood for coercion.

@dm1try
Copy link
Member

dm1try commented Feb 15, 2019

as already mentioned above, dry-schema(that extracted from dry-validation) looks like something that is more suitable for grape needs(coercion/validation of input params). though, it should be checked if the interface of the library can be easily used without its DSL.

if something will be refactored, it would be nice to see a solution with an ability to disable/replace validation component at all. I see params block as "meta description" of a user input for a specific endpoint. this description can be used by different consumers(validation component/documentation component/tests generator component/etc). as the result, we should have single "cross-cutting" DSL(as we have now) which provides an endpoint schema to consumers(we do not have now). so I don't really like the snippet above, even if it is a POC :)

@flash-gordon
Copy link

flash-gordon commented Feb 15, 2019

You may be interested then in the fact dry-schema exposes the definition via AST which can be reused for any purpose you mentioned

schema = Dry::Schema.Params do
  required(:user).type(:hash).schema do
    required(:email).type(:string)
    required(:age).type(:integer)

    required(:address).type(:hash).schema do
      required(:street).type(:string)
      required(:city).type(:string)
      required(:zipcode).type(:string)

      required(:location).type(:hash).schema do
        required(:lat).type(:float)
        required(:lng).type(:float)
      end
    end
  end
end
schema.to_ast
=> [:set,
 [[:and,
   [[:predicate, [:key?, [[:name, :user], [:input, Undefined]]]],
    [:key,
     [:user,
      [:and,
       [[:predicate, [:hash?, [[:input, Undefined]]]],
        [:set,
         [[:predicate, [:key?, [[:name, :email], [:input, Undefined]]]],
          [:predicate, [:key?, [[:name, :age], [:input, Undefined]]]],
          [:and,
           [[:predicate, [:key?, [[:name, :address], [:input, Undefined]]]],
            [:key,
             [:address,
              [:and,
               [[:predicate, [:hash?, [[:input, Undefined]]]],
                [:set,
                 [[:predicate, [:key?, [[:name, :street], [:input, Undefined]]]],
                  [:predicate, [:key?, [[:name, :city], [:input, Undefined]]]],
                  [:predicate, [:key?, [[:name, :zipcode], [:input, Undefined]]]],
                  [:and,
                   [[:predicate, [:key?, [[:name, :location], [:input, Undefined]]]],
                    [:key,
                     [:location,
                      [:and,
                       [[:predicate, [:hash?, [[:input, Undefined]]]],
                        [:set, [[:predicate, [:key?, [[:name, :lat], [:input, Undefined]]]], [:predicate, [:key?, [[:name, :lng], [:input, Undefined]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]

There's also another definition, with actual types

@dnesteryuk
Copy link
Member

Hello community, you are welcome to review my PR replacing virtus with dry-types. 🙂 Later we might bring dry-schema as well, but it will require more work and it will bring lots of breaking changes. So, let's start with dry-types 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants