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.

Sign up for GitHub

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

Jump to bottom

allow reserved words as table and column names #139

Merged
merged 1 commit into from Feb 15, 2018
Merged

allow reserved words as table and column names #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions spec/granite_orm/querying/find_by_spec.cr
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@ require "../../spec_helper"

{% for adapter in GraniteExample::ADAPTERS %}
{% model_constant = "Parent#{adapter.camelcase.id}".id %}
{% reserved_word_constant = "ReservedWord#{adapter.camelcase.id}".id %}

describe "{{ adapter.id }} #find_by" do
it "finds an object with a string field" do
Expand All @@ -25,6 +26,20 @@ require "../../spec_helper"
found = {{ model_constant }}.find_by(:name, name)
found.not_nil!.id.should eq model.id
end

it "also works with reserved words" do
value = "robinson"

model = {{ reserved_word_constant }}.new
model.all = value
model.save

found = {{ reserved_word_constant }}.find_by("all", value)
found.not_nil!.id.should eq model.id

found = {{ reserved_word_constant }}.find_by(:all, value)
found.not_nil!.id.should eq model.id
end
end

{% end %}
16 changes: 16 additions & 0 deletions spec/granite_orm/transactions/save_spec.cr
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ require "../../spec_helper"
parent_constant = "Parent#{adapter.camelcase.id}".id
school_constant = "School#{adapter.camelcase.id}".id
nation_county_constant = "Nation::County#{adapter.camelcase.id}".id
reserved_word_constant = "ReservedWord#{adapter.camelcase.id}".id
%}

describe "{{ adapter.id }} #save" do
Expand Down Expand Up @@ -98,6 +99,21 @@ require "../../spec_helper"
found_county.not_nil!.name.should eq new_name
end
end

describe "using a reserved word as a column name" do
# `all` is a reserved word in almost RDB like MySQL, PostgreSQL
it "creates and updates" do
reserved_word = {{ reserved_word_constant }}.new
reserved_word.all = "foo"
reserved_word.save
reserved_word.errors.empty?.should be_true

reserved_word.all = "bar"
reserved_word.save
reserved_word.errors.empty?.should be_true
reserved_word.all.should eq("bar")
end
end
end

{% end %}
19 changes: 19 additions & 0 deletions spec/spec_models.cr
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@ end
nation_county_table = "nation_county_#{adapter_literal}s".id
review_table = "review_#{adapter_literal}s".id
empty_table = "empty_#{adapter_literal}s".id
reserved_word_table = "select".id

if adapter == "pg"
primary_key_sql = "BIGSERIAL PRIMARY KEY".id
Expand Down Expand Up @@ -230,6 +231,22 @@ end
end
end

class ReservedWord{{ adapter_const_suffix }} < Granite::ORM::Base
adapter {{ adapter_literal }}
table_name "{{ reserved_word_table }}"
field all : String

def self.drop_and_create
exec "DROP TABLE IF EXISTS #{quoted_table_name}"
exec <<-SQL
CREATE TABLE #{quoted_table_name} (
id {{ primary_key_sql }},
#{quote("all")} VARCHAR(255)
)
SQL
end
end

module GraniteExample
@@model_classes << Parent{{ adapter_const_suffix }}
@@model_classes << Teacher{{ adapter_const_suffix }}
Expand All @@ -240,6 +257,7 @@ end
@@model_classes << Nation::County{{ adapter_const_suffix }}
@@model_classes << Review{{ adapter_const_suffix }}
@@model_classes << Empty{{ adapter_const_suffix }}
@@model_classes << ReservedWord{{ adapter_const_suffix }}
end

Spec.before_each do
Expand All @@ -252,6 +270,7 @@ end
Nation::County{{ adapter_const_suffix }}.clear
Review{{ adapter_const_suffix }}.clear
Empty{{ adapter_const_suffix }}.clear
ReservedWord{{ adapter_const_suffix }}.clear
end

{% end %}
9 changes: 9 additions & 0 deletions src/adapter/base.cr
View file
Open in desktop
Expand Up @@ -60,6 +60,15 @@ abstract class Granite::Adapter::Base
Granite::Adapter::Base.env(url)
end

# Use macro in order to read a constant defined in each subclasses.
macro inherited
# quotes table and column names
def quote(name : String) : String
char = QUOTING_CHAR
char + name.gsub(char, "#{char}#{char}") + char
Copy link
Member

@drujensen drujensen Feb 12, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned with performance. Do we need to perform a gsub for every table and column name or can we improve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, it will be slightly slow, but gsub is fast and the target string is short so we can ignore the influence on the whole. From the performance point of view, I think there are many other places that can speed up. Furthermore, the implementation by Crystal is already too fast enough.

Therefore, unless it is a clear problem at the present time, I would like to prioritize "features" over "performance". Yep, I'd like to implement many features like following! 😺

  • support natural keys
  • bang method like save!, find!
  • avoid ugly and redundant code like record.name.not_nil!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was called one time, I would agree but its getting called multiple times, once per each column. I agree that the feature outweighs the cost. I was wondering if there was a better way to do this. I'm ok with this for now. We can look at optimizing it later.

Thanks for your contribution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drujensen hopefully granite can defer to lower level database specific adapters at some point, I'd rather not own the code that does escaping 😰

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robacarp right, right. I am not completely comfortable with this change. I know we need it but something feels odd here.

end
end

# class level method so we can test it
def self.env(url)
regex = /\$\{(.*?)\}/
Expand Down
26 changes: 14 additions & 12 deletions src/adapter/mysql.cr
View file
Open in desktop
Expand Up @@ -3,9 +3,11 @@ require "mysql"

# Mysql implementation of the Adapter
class Granite::Adapter::Mysql < Granite::Adapter::Base
QUOTING_CHAR = '`'

# Using TRUNCATE instead of DELETE so the id column resets to 0
def clear(table_name)
statement = "TRUNCATE #{table_name}"
statement = "TRUNCATE #{quote(table_name)}"

log statement

Expand All @@ -20,8 +22,8 @@ class Granite::Adapter::Mysql < Granite::Adapter::Base
def select(table_name, fields, clause = "", params = [] of DB::Any, &block)
statement = String.build do |stmt|
stmt << "SELECT "
stmt << fields.map { |name| "#{table_name}.#{name}" }.join(", ")
stmt << " FROM #{table_name} #{clause}"
stmt << fields.map { |name| "#{quote(table_name)}.#{quote(name)}" }.join(", ")
stmt << " FROM #{quote(table_name)} #{clause}"
end

log statement, params
Expand All @@ -39,9 +41,9 @@ class Granite::Adapter::Mysql < Granite::Adapter::Base
def select_one(table_name, fields, field, id, &block)
statement = String.build do |stmt|
stmt << "SELECT "
stmt << fields.map { |name| "#{table_name}.#{name}" }.join(", ")
stmt << " FROM #{table_name}"
stmt << " WHERE #{field}=? LIMIT 1"
stmt << fields.map { |name| "#{quote(table_name)}.#{quote(name)}" }.join(", ")
stmt << " FROM #{quote(table_name)}"
stmt << " WHERE #{quote(field)}=? LIMIT 1"
end

log statement, id
Expand All @@ -55,8 +57,8 @@ class Granite::Adapter::Mysql < Granite::Adapter::Base

def insert(table_name, fields, params)
statement = String.build do |stmt|
stmt << "INSERT INTO #{table_name} ("
stmt << fields.map { |name| "#{name}" }.join(", ")
stmt << "INSERT INTO #{quote(table_name)} ("
stmt << fields.map { |name| "#{quote(name)}" }.join(", ")
stmt << ") VALUES ("
stmt << fields.map { |name| "?" }.join(", ")
stmt << ")"
Expand All @@ -77,9 +79,9 @@ class Granite::Adapter::Mysql < Granite::Adapter::Base
# This will update a row in the database.
def update(table_name, primary_name, fields, params)
statement = String.build do |stmt|
stmt << "UPDATE #{table_name} SET "
stmt << fields.map { |name| "#{name}=?" }.join(", ")
stmt << " WHERE #{primary_name}=?"
stmt << "UPDATE #{quote(table_name)} SET "
stmt << fields.map { |name| "#{quote(name)}=?" }.join(", ")
stmt << " WHERE #{quote(primary_name)}=?"
end

log statement, params
Expand All @@ -91,7 +93,7 @@ class Granite::Adapter::Mysql < Granite::Adapter::Base

# This will delete a row from the database.
def delete(table_name, primary_name, value)
statement = "DELETE FROM #{table_name} WHERE #{primary_name}=?"
statement = "DELETE FROM #{quote(table_name)} WHERE #{quote(primary_name)}=?"

log statement, value

Expand Down
26 changes: 14 additions & 12 deletions src/adapter/pg.cr
View file
Open in desktop
Expand Up @@ -3,9 +3,11 @@ require "pg"

# PostgreSQL implementation of the Adapter
class Granite::Adapter::Pg < Granite::Adapter::Base
QUOTING_CHAR = '"'

# remove all rows from a table and reset the counter on the id.
def clear(table_name)
statement = "DELETE FROM #{table_name}"
statement = "DELETE FROM #{quote(table_name)}"

log statement

Expand All @@ -22,8 +24,8 @@ class Granite::Adapter::Pg < Granite::Adapter::Base

statement = String.build do |stmt|
stmt << "SELECT "
stmt << fields.map { |name| "#{table_name}.#{name}" }.join(", ")
stmt << " FROM #{table_name} #{clause}"
stmt << fields.map { |name| "#{quote(table_name)}.#{quote(name)}" }.join(", ")
stmt << " FROM #{quote(table_name)} #{clause}"
end

log statement, params
Expand All @@ -39,9 +41,9 @@ class Granite::Adapter::Pg < Granite::Adapter::Base
def select_one(table_name, fields, field, id, &block)
statement = String.build do |stmt|
stmt << "SELECT "
stmt << fields.map { |name| "#{table_name}.#{name}" }.join(", ")
stmt << " FROM #{table_name}"
stmt << " WHERE #{field}=$1 LIMIT 1"
stmt << fields.map { |name| "#{quote(table_name)}.#{quote(name)}" }.join(", ")
stmt << " FROM #{quote(table_name)}"
stmt << " WHERE #{quote(field)}=$1 LIMIT 1"
end

log statement, id
Expand All @@ -55,8 +57,8 @@ class Granite::Adapter::Pg < Granite::Adapter::Base

def insert(table_name, fields, params)
statement = String.build do |stmt|
stmt << "INSERT INTO #{table_name} ("
stmt << fields.map { |name| "#{name}" }.join(", ")
stmt << "INSERT INTO #{quote(table_name)} ("
stmt << fields.map { |name| "#{quote(name)}" }.join(", ")
stmt << ") VALUES ("
stmt << fields.map { |name| "$#{fields.index(name).not_nil! + 1}" }.join(", ")
stmt << ")"
Expand All @@ -77,9 +79,9 @@ class Granite::Adapter::Pg < Granite::Adapter::Base
# This will update a row in the database.
def update(table_name, primary_name, fields, params)
statement = String.build do |stmt|
stmt << "UPDATE #{table_name} SET "
stmt << fields.map { |name| "#{name}=$#{fields.index(name).not_nil! + 1}" }.join(", ")
stmt << " WHERE #{primary_name}=$#{fields.size + 1}"
stmt << "UPDATE #{quote(table_name)} SET "
stmt << fields.map { |name| "#{quote(name)}=$#{fields.index(name).not_nil! + 1}" }.join(", ")
stmt << " WHERE #{quote(primary_name)}=$#{fields.size + 1}"
end

log statement, params
Expand All @@ -91,7 +93,7 @@ class Granite::Adapter::Pg < Granite::Adapter::Base

# This will delete a row from the database.
def delete(table_name, primary_name, value)
statement = "DELETE FROM #{table_name} WHERE #{primary_name}=$1"
statement = "DELETE FROM #{quote(table_name)} WHERE #{quote(primary_name)}=$1"

log statement, value

Expand Down
26 changes: 14 additions & 12 deletions src/adapter/sqlite.cr
View file
Open in desktop
Expand Up @@ -3,9 +3,11 @@ require "sqlite3"

# Sqlite implementation of the Adapter
class Granite::Adapter::Sqlite < Granite::Adapter::Base
QUOTING_CHAR = '"'

# remove all rows from a table and reset the counter on the id.
def clear(table_name)
statement = "DELETE FROM #{table_name}"
statement = "DELETE FROM #{quote(table_name)}"

log statement

Expand All @@ -20,8 +22,8 @@ class Granite::Adapter::Sqlite < Granite::Adapter::Base
def select(table_name, fields, clause = "", params = [] of DB::Any, &block)
statement = String.build do |stmt|
stmt << "SELECT "
stmt << fields.map { |name| "#{table_name}.#{name}" }.join(", ")
stmt << " FROM #{table_name} #{clause}"
stmt << fields.map { |name| "#{quote(table_name)}.#{quote(name)}" }.join(", ")
stmt << " FROM #{quote(table_name)} #{clause}"
end

log statement, params
Expand All @@ -37,9 +39,9 @@ class Granite::Adapter::Sqlite < Granite::Adapter::Base
def select_one(table_name, fields, field, id, &block)
statement = String.build do |stmt|
stmt << "SELECT "
stmt << fields.map { |name| "#{table_name}.#{name}" }.join(", ")
stmt << " FROM #{table_name}"
stmt << " WHERE #{field}=:id LIMIT 1"
stmt << fields.map { |name| "#{quote(table_name)}.#{quote(name)}" }.join(", ")
stmt << " FROM #{quote(table_name)}"
stmt << " WHERE #{quote(field)}=:id LIMIT 1"
end

log statement, id
Expand All @@ -53,8 +55,8 @@ class Granite::Adapter::Sqlite < Granite::Adapter::Base

def insert(table_name, fields, params)
statement = String.build do |stmt|
stmt << "INSERT INTO #{table_name} ("
stmt << fields.map { |name| "#{name}" }.join(", ")
stmt << "INSERT INTO #{quote(table_name)} ("
stmt << fields.map { |name| "#{quote(name)}" }.join(", ")
stmt << ") VALUES ("
stmt << fields.map { |name| "?" }.join(", ")
stmt << ")"
Expand All @@ -75,9 +77,9 @@ class Granite::Adapter::Sqlite < Granite::Adapter::Base
# This will update a row in the database.
def update(table_name, primary_name, fields, params)
statement = String.build do |stmt|
stmt << "UPDATE #{table_name} SET "
stmt << fields.map { |name| "#{name}=?" }.join(", ")
stmt << " WHERE #{primary_name}=?"
stmt << "UPDATE #{quote(table_name)} SET "
stmt << fields.map { |name| "#{quote(name)}=?" }.join(", ")
stmt << " WHERE #{quote(primary_name)}=?"
end

log statement, params
Expand All @@ -89,7 +91,7 @@ class Granite::Adapter::Sqlite < Granite::Adapter::Base

# This will delete a row from the database.
def delete(table_name, primary_name, value)
statement = "DELETE FROM #{table_name} WHERE #{primary_name}=?"
statement = "DELETE FROM #{quote(table_name)} WHERE #{quote(primary_name)}=?"

log statement, value

Expand Down
2 changes: 1 addition & 1 deletion src/granite_orm/querying.cr
View file
Open in desktop
Expand Up @@ -94,7 +94,7 @@ module Granite::ORM::Querying

# count returns a count of all the records
def count : Int32
scalar "select count(*) from #{@@table_name}", &.to_s.to_i
scalar "SELECT COUNT(*) FROM #{quoted_table_name}", &.to_s.to_i
end

def exec(clause = "")
Expand Down
8 changes: 8 additions & 0 deletions src/granite_orm/table.cr
View file
Open in desktop
Expand Up @@ -45,5 +45,13 @@ module Granite::ORM::Table
def self.primary_name
@@primary_name
end

def self.quoted_table_name
@@adapter.quote(table_name)
end

def self.quote(column_name)
@@adapter.quote(column_name)
end
end
end