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

SQLite: Foreign Key Support #24743

Merged
merged 2 commits into from Jan 17, 2017

Conversation

Projects
None yet
5 participants
@kamipo
Member

kamipo commented Apr 26, 2016

@rails-bot

This comment has been minimized.

rails-bot commented Apr 26, 2016

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

@sgrif

View changes

activerecord/test/schema/schema.rb Outdated
create_table :fk_test_has_fk, force: true do |t|
t.integer :fk_id, null: false
end
if supports_foreign_keys_in_create?

This comment has been minimized.

@sgrif

sgrif Apr 26, 2016

Member

We shouldn't be pushing caring about this onto our users. If create_table is called with foreign keys on SQLite, we should do a separate query automatically

This comment has been minimized.

@kamipo

kamipo Apr 26, 2016

Member

If create_table is called with foreign keys, generates in single query. ref #20009.

This comment has been minimized.

@sgrif

sgrif Apr 26, 2016

Member

I know. You're missing my point. It needs to not do that on SQLite. This code should be capable of running on both backends.

This comment has been minimized.

@kamipo

kamipo Apr 26, 2016

Member

Sorry I don't understand. AFAIK this code block works on mysql2, postgresql, and sqlite3 adapters.

  if supports_foreign_keys_in_create?
    drop_table :fk_test_has_fk, if_exists: true

    create_table :fk_test_has_pk, force: true, primary_key: "pk_id" do |t|
    end

    create_table :fk_test_has_fk do |t|
      t.integer :fk_id, null: false
      t.foreign_key :fk_test_has_pk, column: "fk_id", name: "fk_name", primary_key: "pk_id"
    end
  end

MySQL:

DROP TABLE IF EXISTS `fk_test_has_fk`
DROP TABLE `fk_test_has_pk`
CREATE TABLE `fk_test_has_pk` (`pk_id` int AUTO_INCREMENT PRIMARY KEY) ENGINE=InnoDB
CREATE TABLE `fk_test_has_fk` (`id` int AUTO_INCREMENT PRIMARY KEY, `fk_id` int NOT NULL, CONSTRAINT `fk_name`
FOREIGN KEY (`fk_id`)
  REFERENCES `fk_test_has_pk` (`pk_id`)
) ENGINE=InnoDB

PostgreSQL:

DROP TABLE IF EXISTS "fk_test_has_fk"
DROP TABLE "fk_test_has_pk"
CREATE TABLE "fk_test_has_pk" ("pk_id" serial primary key)
CREATE TABLE "fk_test_has_fk" ("id" serial primary key, "fk_id" integer NOT NULL, CONSTRAINT "fk_name"
FOREIGN KEY ("fk_id")
  REFERENCES "fk_test_has_pk" ("pk_id")
)

SQLite3:

DROP TABLE IF EXISTS "fk_test_has_fk"
DROP TABLE "fk_test_has_pk"
CREATE TABLE "fk_test_has_pk" ("pk_id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL)
CREATE TABLE "fk_test_has_fk" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "fk_id" integer NOT NULL, CONSTRAINT "fk_name"
FOREIGN KEY ("fk_id")
  REFERENCES "fk_test_has_pk" ("pk_id")
)

This comment has been minimized.

@sgrif

sgrif Apr 27, 2016

Member

What I'm saying is this should work without the if statement. On versions of SQLite that do not support foreign keys in create, or any other adapter that doesn't support this as a single query, it should automatically perform 2 queries.

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch 4 times, most recently Apr 26, 2016

@kamipo

View changes

activerecord/test/schema/schema.rb Outdated
create_table :fk_test_has_fk do |t|
t.integer :fk_id, null: false
t.foreign_key :fk_test_has_pk, column: "fk_id", name: "fk_name", primary_key: "pk_id"
end

This comment has been minimized.

@kamipo

kamipo May 2, 2016

Member

@sgrif Thank you for explanation, I understand. Actually this works without if supports_foreign_keys_in_create?. I removed the if statement.

This comment has been minimized.

@sgrif

sgrif May 2, 2016

Member

Tested on an older version of SQLite?

This comment has been minimized.

@kamipo

kamipo May 2, 2016

Member
def (ActiveRecord::Base.connection).sqlite_version
  @sqlite_version ||= ActiveRecord::ConnectionAdapters::SQLite3Adapter::Version.new('3.6.16')
end

ActiveRecord::Schema.define do
  drop_table :fk_test_has_fk, if_exists: true

  create_table :fk_test_has_pk, force: true, primary_key: "pk_id" do |t|
  end

  create_table :fk_test_has_fk do |t|
    t.integer :fk_id, null: false
    t.foreign_key :fk_test_has_pk, column: "fk_id", name: "fk_name", primary_key: "pk_id"
  end
end

Result:

DROP TABLE IF EXISTS "fk_test_has_fk"
DROP TABLE "fk_test_has_pk"
CREATE TABLE "fk_test_has_pk" ("pk_id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL)
CREATE TABLE "fk_test_has_fk" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "fk_id" integer NOT NULL)

https://github.com/kamipo/rails/blob/sqlite_foreing_key_support/activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb#L52-L54
https://github.com/kamipo/rails/blob/sqlite_foreing_key_support/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb#L910

Even Ubuntu 10.04 bundles sqlite3 3.6.22. To test an actual older version (< 3.6.19) is difficult for me...
https://launchpad.net/ubuntu/+source/sqlite3/3.6.22-1

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch 2 times, most recently May 5, 2016

@kamipo

View changes

activerecord/test/schema/schema.rb Outdated
add_foreign_key :lessons_students, :students
create_table :fk_test_has_fk do |t|
t.integer :fk_id, null: false
t.foreign_key :fk_test_has_pk, column: "fk_id", name: "fk_name", primary_key: "pk_id"

This comment has been minimized.

@kamipo

kamipo May 7, 2016

Member

Tested on an older version of SQLite?

def (ActiveRecord::Base.connection).sqlite_version
  @sqlite_version ||= ActiveRecord::ConnectionAdapters::SQLite3Adapter::Version.new('3.6.16')
end

ActiveRecord::Schema.define do
  drop_table :fk_test_has_fk, if_exists: true

  create_table :fk_test_has_pk, force: true, primary_key: "pk_id" do |t|
  end

  create_table :fk_test_has_fk do |t|
    t.integer :fk_id, null: false
    t.foreign_key :fk_test_has_pk, column: "fk_id", name: "fk_name", primary_key: "pk_id"
  end
end

Result:

DROP TABLE IF EXISTS "fk_test_has_fk"
DROP TABLE "fk_test_has_pk"
CREATE TABLE "fk_test_has_pk" ("pk_id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL)
CREATE TABLE "fk_test_has_fk" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "fk_id" integer NOT NULL)

https://github.com/kamipo/rails/blob/sqlite_foreing_key_support/activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb#L52-L54
https://github.com/kamipo/rails/blob/sqlite_foreing_key_support/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb#L910

Even Ubuntu 10.04 bundles sqlite3 3.6.22. To test an actual older version (< 3.6.19) is difficult for me...
https://launchpad.net/ubuntu/+source/sqlite3/3.6.22-1

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch May 11, 2016

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch 3 times, most recently Jun 3, 2016

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch Jun 20, 2016

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch Jul 2, 2016

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch Aug 7, 2016

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch 3 times, most recently Oct 29, 2016

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch Dec 24, 2016

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch Jan 16, 2017

@kamipo kamipo force-pushed the kamipo:sqlite_foreing_key_support branch to 24f264e Jan 16, 2017

@kamipo

This comment has been minimized.

Member

kamipo commented Jan 17, 2017

Since you has merged #27701, and #24986 has assigned to you, can you also review the PR for foreign key creation? r? @pixeltrix

@rails-bot rails-bot assigned pixeltrix and unassigned sgrif Jan 17, 2017

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Jan 17, 2017

@kamipo doesn't this include the changes in #24986

@pixeltrix pixeltrix merged commit d665c61 into rails:master Jan 17, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Jan 17, 2017

@kamipo thanks! 👍

@kamipo kamipo deleted the kamipo:sqlite_foreing_key_support branch Jan 17, 2017

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Jan 18, 2017

kamipo added a commit to kamipo/rails that referenced this pull request Jan 19, 2017

maclover7 added a commit that referenced this pull request Jan 19, 2017

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