Ruby Repository Pattern

Question

I am trying to teach myself about repository pattern in ruby. Could you please review my code.

adapter.rb

module Adapter
  class Main
    def prebuild(obj)
      self.set_table_name = obj.table_name.to_s
      variables = obj.instance_variables
      @values = []
      @keys = []

      variables.reject! { |c| c.to_s == '@table_name' }.map do |key, value|
        @keys.push(key.to_s.gsub('@', ''))
        @values.push(obj.send(@keys.last))
      end
    end

    def get_table_name
      @table_name
    end

    def get_keys
      @keys
    end

    def get_values
      @values.map { |value| "'#{value}'" }
    end

    def set_table_name=(table_name)
      @table_name = table_name
    end
  end

  class Sqlite < Main
    require 'sqlite3'

    def initialize
      @db = SQLite3::Database.new 'file.db'
    end

    def persist(obj)
      prebuild(obj)
      sql_string = "INSERT INTO #{get_table_name} (#{get_keys.join(', ')}) VALUES (#{get_values.join(', ')})"
      @db.execute sql_string
      obj.id = @db.execute "SELECT last_insert_rowid()"
      obj
    end

    def retrieve(entity, id)
      self.set_table_name = entity.table_name
      rows = @db.execute "SELECT * FROM #{get_table_name} WHERE id=#{id}"
      fields = get_fields
      new_entity = entity.new

      for i in (0..fields.size-1)
        new_entity.send("#{fields[i]}=", rows[0][i])
      end
      new_entity
    end

    def get_fields
      rows = @db.execute "PRAGMA TABLE_INFO (#{get_table_name});"

      rows.map { |name| name[1] }
    end

    def delete(obj)
      prebuild(obj)
      @db.execute "DELETE FROM #{get_table_name} WHERE id=#{obj.id}"
    end
  end
end

entity.rb

module Entity
  class Member
    attr_reader :table_name

    attr_accessor :username
    attr_accessor :email
    attr_accessor :password
    class << self
      def table_name
        @table_name ||= 'members'
      end
    end
    def table_name
      @table_name ||= 'members'
    end

    def id=(new_id)
      @id = new_id
    end

    def id
      @id
    end

    def username=(new_username)
      @username = new_username
    end

    def username
      @username
    end

    def email=(new_mail)
      @email = new_mail
    end

    def email
      @email
    end

    def password=(new_password)
      @password = new_password
    end

    def password
      @password
    end
  end
end

repository.rb

module Repository
  class Member
    def initialize(adapter)
      @adapter = adapter.new
    end

    def save(member)
      @adapter.persist(member)
    end

    def get(id)
      @adapter.retrieve(Entity::Member, id)
    end

    def delete(member)
      @adapter.delete(member)
    end
  end
end

test.rb

require './lib/entity'
require './lib/adapter'
require './lib/repository'

member = Entity::Member.new
member.username = 'ahmet'
member.email = '[email protected]'
member.password = '12345'
repository = Repository::Member.new(Adapter::Sqlite)
repository.save(member)

member = repository.get(5)

repository.delete(member)

Answer 1

Starting with the end: test.rb barely tests anything. Yes, it'll exercise your code, and make it crash if there's a mistake, but it doesn't actually verify that your code does what it's supposed to do.

So, I'd highly suggest you write proper unit test using one of the many test libraries out there (Ruby's own Test::Unit, minitest, rspec, etc...)

You seem to be confused about accessor methods. The purpose of attr_accessor is to have Ruby automatically generate the methods to get and set an instance variable. In your Member class, you're using attr_accessor, but you're also (redundantly) providing your own getters and setters. Since they literally just get and set, they're exactly the same as what attr_accessor has already made for you.

So Member can be reduce to this:

module Entity
  class Member
    attr_accessor :id, :username, :email, :password

    class << self
      def table_name
        @table_name ||= 'members'
      end
    end

    def table_name
      @table_name ||= 'members'
    end
  end
end

(Of course, id shouldn't be easily settable, but I can't cover everything in this review)

The whole table_name construction is odd too. If anything, this would make more sense:

module Entity
  class Member
    attr_accessor :id, :username, :email, :password

    def self.table_name
      'members'
    end
  end
end

Since table_name should probably be constant, just return a string. And don't duplicate the method for both class and instance. If anyone needs the table name from an instance, they can do some_member_object.class.table_name.

Your Main class also has some funny ideas about data access. Here, you're not using attr_reader :keys and attr_accessor :table_name although you might as well. Then you can skip the methods. The other reason to do this is that naming methods get_* or set_* is not very Ruby-like.

get_values (which should just be named values), is also problematic for a few reasons:

• Main is supposed to be pretty abstract. Why does it assume that values should be returned as quoted strings? What is the derived class isn't meant for SQLite or any kind of SQL? Perhaps you just want the raw values?

• Yes, in fact you do want the raw values! Poor string-quoting is the oldest attack vector in the book. For instance, what if, say, the password value is a string like this: "sup'); DROP TABLE members;"?

With your code, you'll end up running the following the current query when trying to save:

INSERT INTO members (email, username, password)
VALUES ('[email protected]', 'foo', 'sup'); DROP TABLE members; ');

A basic SQL injection attack. As you can see from the syntax highlighting, the SQL suddenly contains TWO commands: INSERT and DROP TABLE. You insert a row... and it happens to delete your entire table. Oops.

Long story short: Use proper parameter substitution (which the sqlite3 gem has built in):

db.execute("INSERT INTO members (email, username, password) VALUES (?, ?, ?)", [email, username, password])

That let's sqlite take care of doing proper string quoting/escaping and all that. It's safe.

And you can replace the get_values method with attr_reader :values.

Of course, the only reason attr_readers make sense is that you have the prebuild method. Which doesn't really make much sense. If must be called before anything else works, so if anything it should be a constructor.

But really, it's simpler to just drop Main entirely, and instead add a superclass for Member:

class Record
  def to_hash
    instance_variables.each_with_object({}) do |hash, name|
      key = name.to_s.sub('@', '') }
      value = send(key)
      hash[key] = value;
    end
  end
end

class Member < Record
  ...
end

And then the sqlite adapter becomes:

require 'sqlite3' # require statement should be at the top of the file

class Sqlite
  def initialize
    @db = SQLite3::Database.new 'file.db' # the filename should really be an argument
  end

  # formerly named "persist", but "persist" can mean either insert *or* update
  def insert(record)
    hash = record.to_hash.delete("id")
    markers = ["?"] * hash.keys.count
    sql = "INSERT INTO #{record.class.table_name} (#{hash.keys.join(',')} VALUES (#{markers.join(','))"
    @db.execute(sql, hash.values);

    record.id = @db.execute("SELECT last_insert_rowid()")
    record
  end

  # presumably you'll only want 1 record returned when searching by id
  def retrieve(klass, id)
    columns = columns_in_table(klass.table_name)
    rows = @db.execute("SELECT * FROM #{klass.table_name} WHERE id=? LIMIT 1", id)
    return nil if rows.empty? # nothing found
    instance = klass.new
    rows.first.zip(columns).each do |key, value|
      instance.send("#{key}=", value)
    end
    instance
  end

  def delete(record)
    @db.execute("DELETE FROM #{record.class.table_name} WHERE id=#{record.id}"
  end

  private

  def columns_in_table(table)
    @db.execute("PRAGMA TABLE_INFO (#{table})").map { |info| info[1] }
  end
end

Of course, there's still a long way from here to a well-known and battle-tested implementation like Rails' ActiveRecord. For instance, you currently have to duplicate the names of columns in your objects. If an object doesn't have the same instance variables as the table's columns - or if they're named differently, or of the wrong type... - things will break.

So, in all, a good exercise, but not something I'd rely on.