Use Virtus for defining attributes and helping with data-type coercions

[bmorton]

This isn't a bug, but I wanted to get some feedback on the viability of this being accepted if I open a PR for it. Basically, I want to replace the attribute definition stuff with Virtus: https://github.com/solnic/virtus

Virtus is an extraction from DataMapper that provides a common API for defining attributes and doing data-type coercion.

The cool thing here is that you don't have to define attributes with Virtus. This change would be completely backwards compatible with the current way of defining models. It would simply add the attribute DSL.

Here's what a model would look like using Virtus attributes:

class Note
  include Curator::Model

  attribute :id, String
  attribute :title, String
  attribute :description, String
  attribute :user_id, Integer
end

Basically, we'd let Virtus handle the constructor/mass assignment stuff and could remove the stuff in Curator for that. We can also define created_at/updated_at as Time types.

require 'curator'

module Curator
  module VirtusModel
    extend ActiveSupport::Concern

    included do
      include Virtus.model

      attribute :created_at, Time
      attribute :updated_at, Time

      attr_writer :version
    end

    def persisted?
      id.present?
    end

    def touch
      now = Time.now.utc
      created_at = now if created_at.nil?
      updated_at = now
    end

    def version
      @version || self.class.version
    end

    def ==(other)
      self.id == other.id
    end

    module ClassMethods
      include ActiveModel::Naming

      def current_version(number)
        @version = number
      end

      def version
        @version || 0
      end
    end
  end
end

Thoughts?

[bmorton]

Sorry, it looks like the changes would not be backwards compatible :(

Thoughts on switching to this method or optionally using the attribute DSL?

EDIT: Wrong again. I'm getting confused by some failing tests. I think this would be backwards compatible, but now I'm working on the test coverage to prove it.

[bmorton]

It looks like this change is still compatible with attr_accessor, but breaks attr_reader support for defining attributes. It looks like the Curator::Model constructor sets attributes through instance variables, while Virtus' constructor sets them through methods, which explains why this breaks.

[pgr0ss]

I'm cool with switching to Virtus. Ideally, we should try to keep backwards compatibility. If it's really not possible, we can bump the major version. It sounds like the benefits are worth it, specifically data type coercion for MongoDB.

[jtdowney]

Since the version is less than 1.0 we don't have to bump a major version for a breaking change.

[pgr0ss]

That's true, although I think it's a poor user experience. If we like how the attributes stuff looks, and given that development has slowed, I think we could release 1.0.0.

[bmorton]

Awesome. I'll get a pull request and some documentation pulled together and we can start looking at how this will come together.

Thanks!

Sent from my iPhone

On Jan 28, 2014, at 5:46 PM, Paul Gross [email protected] wrote:

That's true, although I think it's a poor user experience. If we like how the attributes stuff looks, and given that development has slowed, I think we could release 1.0.0.

—
Reply to this email directly or view it on GitHub.

[jlecour]

Hi,

I've been using Virtus a lot, in many different contexts, and I like it a lot.

But I've had issues with performance, especially when using many coercions.
Before investing too much time in switching to Virtus, you should build the basic and compare the performance when fetching many records from the data store.

In a Rails project, I wanted to implement the Repository pattern with an Elasticsearch backend, and I've found that Virtus (and coercions) are a huge performance penalty, so much that I'm in the process of not using it anymore.

Here is a simple benchmark.

[bmorton]

Is there a Virtus issue open for this?

[jlecour]

There is no issue (yet), since it's not a bug. At least @solnic doesn't think it's a bug since coercions are expensive and delegated to coercible.

See this convo on Twitter : https://twitter.com/jlecour/status/421398836142424064

[solnic]

You can turn off coercions when you don't need them. They are expensive.

I'm looking at ways to simplify and speed up coercible for virtus 2.0 so things will improve. Coercible supports a lot of coercions which require regex matching and I suspect this is the main reason why it's slow.

[bmorton]

Given this, how do people feel moving forward using Virtus by default? Should we wait for something more performant? Should we push attribute management like this outside the scope of Curator?

[jlecour]

Honestly, Virtus remains a really great library. It has great features, it is well coded, …

I've just found myself in a situation where massive assignments with many coercions were too much expensive. But for the general case it's really good.

Ideally, in Curator, we should be able to use either a full fledge Virtus model (by default) but also be able to switch to a lightweight alternative.
I don't know if defining an abstract interface in Curator, with adapter for Virtus, … would be relevant. It might be too complicated.

[bmorton]

Agreed. I like using Virtus as a default, but providing the tools to build your own models from the ground up to optimize or handle whatever is needed.