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.

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

UUID PrimaryGeneratedColumn is not marked as PRIMARY KEY in SQL schema #1161

Closed
RLovelett opened this issue Nov 9, 2017 · 7 comments
Closed
Labels

Comments

@RLovelett
Copy link

RLovelett commented Nov 9, 2017

If I declare a column as PrimaryGeneratedColumn('uuid') the actual column in the schema is not marked as PRIMARY KEY in the resulting SQL.

executing query:  BEGIN TRANSACTION
executing query:  SELECT * FROM sqlite_master WHERE type = 'table' AND name IN ('foo')
executing query:  CREATE TABLE "foo" ("id" varchar NOT NULL)
executing query:  COMMIT
executing query:  BEGIN TRANSACTION
executing query:  INSERT INTO "foo"("id") VALUES ($1) -- PARAMETERS: ["c7c987df-0045-4e2e-ac90-c25b80fd9a73"]
executing query:  COMMIT

I would have expected the CREATE TABLE statement to look more like CREATE TABLE "foo" ("id" varchar NOT NULL PRIMARY KEY).

Source that exemplifies the possible bug mentioned above

example.ts

import 'reflect-metadata';
import {
  ConnectionOptions,
  createConnection,
  Entity,
  PrimaryGeneratedColumn,
} from 'typeorm';

@Entity()
class Foo {
  @PrimaryGeneratedColumn('uuid')
  public id: string;
}

(async () => {
  const databaseOptions: ConnectionOptions = {
    type: 'sqlite',
    database: ':memory:',
    entities: [
      Foo,
    ],
    logging: true,
    logger: 'advanced-console',
    synchronize: true,
  };
  const databaseConnection = await createConnection(databaseOptions);
  const repository = databaseConnection.getRepository(Foo);
  const foo = repository.create();
  await repository.save(foo);
})();


This in and of itself is a problem because it does not require the column to be unique. Of course, I could work-around this quite simply by applying an Index to the column.

The real-show stopper occurs if you try to apply a relationship to the id column above it fails to create the tables.

executing query:  BEGIN TRANSACTION
executing query:  SELECT * FROM sqlite_master WHERE type = 'table' AND name IN ('bar', 'foo')
executing query:  CREATE TABLE "bar" ("id" varchar NOT NULL, "fooId" varchar)
executing query:  CREATE TABLE "foo" ("id" varchar NOT NULL)
executing query:  CREATE TABLE "temporary_bar" ("id" varchar NOT NULL, "fooId" varchar, FOREIGN KEY("fooId") REFERENCES "foo"("id"))
executing query:  INSERT INTO "temporary_bar"("id", "fooId") SELECT "id", "fooId" FROM "bar"
query failed:  INSERT INTO "temporary_bar"("id", "fooId") SELECT "id", "fooId" FROM "bar"
error:  { Error: SQLITE_ERROR: foreign key mismatch - "temporary_bar" referencing "foo" errno: 1, code: 'SQLITE_ERROR' }
executing query:  ROLLBACK
Source that exemplifies the possible bug mentioned above

Foo.ts

import 'reflect-metadata';
import {
  Entity,
  OneToMany,
  PrimaryGeneratedColumn,
} from 'typeorm';
import Bar from './Bar';

@Entity()
export default class Foo {
  @PrimaryGeneratedColumn('uuid')
  public id: string;

  @OneToMany((type) => Bar, (bar) => bar.foo)
  public bar: Bar;
}

Bar.ts

import 'reflect-metadata';
import {
  Entity,
  ManyToOne,
  PrimaryGeneratedColumn,
} from 'typeorm';
import Foo from './Foo';

@Entity()
export default class Bar {
  @PrimaryGeneratedColumn('uuid')
  public id: string;

  @ManyToOne((type) => Foo, (foo) => foo.bar)
  public foo: Foo;
}

example.ts

import 'reflect-metadata';
import {
  ConnectionOptions,
  createConnection,
} from 'typeorm';
import Foo from './Foo';
import Bar from './Bar';

(async () => {
  const databaseOptions: ConnectionOptions = {
    type: 'sqlite',
    database: ':memory:',
    entities: [
      Foo,
      Bar,
    ],
    logging: true,
    logger: 'advanced-console',
    synchronize: true,
  };
  const databaseConnection = await createConnection(databaseOptions);
})();


Questions

  1. Is this a bug or expected behavior?
  2. If this is a bug could we discuss how one would go about fixing this? I'm not opposed to fixing it I'm just thrown by how decorators work and how to debug them.
@malbertSC
Copy link

malbertSC commented Nov 10, 2017

I'm getting this problem in postgres too with a column using a @PrimaryGeneratedColumn() decorator and Synchronize=true set in my ormconfig. If I set that to false and run the CLI schema:sync, it creates the PK constraint properly, but then I get an error

Entity "Ingredient" does not have a primary column. Primary column is required to have in all your entities. Use @PrimaryColumn decorator to add a primary column

... and if I replace @PrimaryGeneratedColumn() with @PrimaryColumn() I get:

Column type for Ingredient#id is not defined or cannot be guessed. Try to explicitly provide a column type to the @Column decorator. ColumnTypeUndefinedError:

For reference, here's my (extremely simple) entity:

@Entity()
export class Ingredient {
    @PrimaryGeneratedColumn()
    id: number;

    @Column()
    name: string;
}

Edit - turns out in our case it was caused by calling createConnection in a process kicked off by node-foreman

@pleerock
Copy link
Member

@RLovelett uuid column types are used as real column types when database supports them. Does sqlite support uuid?

@pleerock
Copy link
Member

AFAIK no, thats why you don't see uuid column type in sqlite. In postgres for example uuid column type is used because its supported by this database.

@RLovelett
Copy link
Author

RLovelett commented Nov 20, 2017

This response appears to be incongruent with the support found in the typeorm library. I do agree, sqlite does not support uuid column natively. Though that seems irrelevant in light of the code inside of the typeorm library.

Sqlite does support varchar as a valid primary key column. Typeorm library is exposing uuid as a valid primary key column which it stores as a varchar.

What is more, it appears typeorm even explicitly supports and tests uuid as a primary column in sqlite by auto-generating the uuid at insert.

Given your response and the existence of the code there is 100% a bug here. It is still however not clear to me what the bug is. It appears to either be:

  1. The typeorm library does support the uuid column in sqlite and it does so erroneously. To fix the bug the code that supports the uuid column type on sqlite should be removed.
  2. The typeorm library does support the uuid column in sqlite and it does so intentionally. To fix the bug the code should be patched to properly generate the CREATE TABLE syntax for sqlite.

In its current implementation typeorm is a buggy implementation of 1 and 2.

@pleerock pleerock reopened this Nov 20, 2017
@pleerock
Copy link
Member

Sorry looks like I misunderstood issue you have. Since sqlite does not support database-level uuid orm tries to generate it by its own and uses varchar column to store uuid there. Looks like issue you have is missing primary key on that column. I'll try to reproduce this issue tomorrow

pleerock pushed a commit that referenced this issue Nov 22, 2017
@pleerock
Copy link
Member

This issue is fixed and released in 0.1.6.

pleerock pushed a commit that referenced this issue Nov 22, 2017
* master:
  fixes #1128 #1161
  fixed broken issue
  Failing test for issue #1034

# Conflicts:
#	package.json
@RLovelett
Copy link
Author

@pleerock thank you. That worked great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants