virtus-weirdness.rb

require "virtus"

class JsonModel
  include Virtus
end

class Node < Virtus::Attribute::Object
  attr_reader :type

  class << self
    def [](type)
      raise ArgumentError, "Child nodes may only be other JsonModel classes" unless type <= JsonModel

      @generated_class_map       ||= {}
      @generated_class_map[type] ||= Class.new(self) do
        default lambda { |m, a| type.new }

        define_method :type do
          type
        end

        define_method :coerce do |value|
          value.kind_of?(Hash) ? type.new(value) : value
        end
      end
    end
  end
end

class ChildModel < JsonModel
end

class ParentModel < JsonModel
  attribute :child,  Node[ChildModel]
  attribute :string, String
end

# This should be String, but it's a descendant of Node??
puts ParentModel.attributes[:string].class.ancestors.inspect

This is an interesting bug.

It's caused because the Node attribute is registered to handle the same primitive as Virtus::Attribute::Object, namely Object. When Virtus does the lookup on String it tries to find the first Virtus attribute that can handle it by searching a lookup table. When Virtus attributes are defined they're added to the "top" of the stack, meaning they're searched first. Since they have a primitive of Object they match first and are what's used.

I'm not entirely sure what the right fix is for this, but I have a couple of ideas:

1. the lookup table could be an OrderedSet so that if a primitive is registered more than once, the first registration "wins" and nothing else can be registered for a primitive that is already reserved.

2. the search algorithm should start at the bottom of the stack and find the most specific attribute class that handles the primitive. So for String it would start off at Veritas::Attribute::Object, then proceed until it hits Veritas::Attribute::String and nothing else in the list would be more specific.

Given both of these I would probably opt for #2 since it would be the simplest thing that could work. A proper OrderedSet implementation is going to be at least 30-50 loc, including docs, and at least a couple hundred lines of specs. I think #2 could be done in less than 10 loc total, and maybe a dozen or two extra lines for the specs.

Here's an example of a reproduction of the problem I'm describing:

require 'rubygems'                                                                  
require 'backports'                                                                 
require 'virtus'                                                                    

Class.new(Virtus::Attribute::Object)                                                

class JsonModel                                                                     
  include Virtus                                                                    

  attribute :string, String                                                         
end                                                                                 

p Virtus::Attribute.determine_type(String).equal?(Virtus::Attribute::String)

If you comment out the anonymous class creation line this will return true as expected.

Hah, good work stumbling upon this!

Yup, my colleague Paul figured out it boiled down to simply having any anonymous class created from Node. I'd found my way into the TypeLookup code, but no further. DataMapper does a similar thing with Flags, but that code works correctly.

So I guess as an immediate workaround, we could use the boxed type as the primitive, thus forcing them to be unique, though this is clearly not a long term solution. I'll see what I can fathom out with a bit of hacking soon.

I'll file a bug in the interim

The bug for this is in solnic/virtus#26 .. and it is now resolved.

(Added for posterity for anyone that comes across this in google)