typeorm icon indicating copy to clipboard operation
typeorm copied to clipboard

Unclear documentation: create() doesn't actually insert new row

Open golergka opened this issue 6 years ago • 46 comments

Issue type:

[ ] question [ ] bug report [ ] feature request [x] documentation issue

Database system/driver:

[ ] cordova [ ] mongodb [ ] mssql [ ] mysql / mariadb [ ] oracle [ ] postgres [x] sqlite [ ] sqljs [ ] websql

TypeORM version:

[x] latest [ ] @next [ ] 0.x.x (or put your version here)

Steps to reproduce or a small repository showing the problem:

Currently, repository.create() creates new instance of entity, but doesn't put it into db - you have to manually call repository.save() afterwards so that typeorm would actually run INSERT statement. I think that a lot of developers may not understand it from current documentation, and that it should be said so explicitly to avoid confusion.

(Will submit PR to docs myself if maintainers approve this).

golergka avatar Mar 20 '18 14:03 golergka

a huge break change?

xujif avatar Mar 21 '18 02:03 xujif

I think this problem exist only for people who used sequelize, since create creates and saves in there. Can you please tell me where exactly "create" was not explained properly in docs?

I know we have this line here.

create - Creates a new instance of User. Optionally accepts an object literal with user properties which will be written into newly created user object

And this explanation clearly says that create creates a new instance of User object and does not tell anything about saving into the database. Maybe you mean another place?

pleerock avatar Mar 21 '18 04:03 pleerock

@pleerock that's exactly the place I was talking about. You're absolutely right that it doesn't say that it inserts an element into a database, and I did feel like an idiot after I realized my mistake.

However, I don't think that I'm the only one, and adding a short disclaimer would prevent a lot of other developers from making a similar mistake as me.

golergka avatar Mar 21 '18 06:03 golergka

Thank you for your suggestion, I'll do it once somebody else rise same issue.

pleerock avatar Mar 26 '18 11:03 pleerock

I had the same issue, it wasn't until I read this post that I realised what a dork I am. :)

MitosisByProxy avatar Oct 13 '18 07:10 MitosisByProxy

@golergka Same here, was really confused by the save vs insert for a moment....

vcfvct avatar Jan 15 '19 16:01 vcfvct

Maybe we shall just remove this create method 🤔

pleerock avatar Jan 15 '19 17:01 pleerock

😈 yeah create without Promise will hit DB.

Maybe just rename to createInstance or just add big note to docs. We have merge, hasId, getId. It will be shame do not have create-like method.

vlapo avatar Jan 15 '19 19:01 vlapo

@vlapo it has almost zero value. What is the point to do getRepository(User).create() when you can just do new User() ? I have same thoughts about merge, we can use object rest to do same and its much more nicer

pleerock avatar Jan 16 '19 07:01 pleerock

@pleerock

new User() and create() are different as for the moment the only possible way to static check property assignment is the following:

const entityLike: Omit<Entity, 'id'> = {
... 
}

const entity = repository.create(entityLike)

It can be much easier if you allow custom constructors for entities.

vimmerru avatar Jan 26 '19 17:01 vimmerru

It's indeed really confusing. If it is simply an utility function for creating an object instance without interacting with the database, I really don't see any point that it should exists in TypeORM (especially in the Repository and EntityManager). 😹

zhenwenc avatar May 06 '19 17:05 zhenwenc

We should move create and merge functions from repository into some util class? I think they have some purpose in this world but not in repository. What do you think @Kononnable ?

vlapo avatar Jun 07 '19 20:06 vlapo

Didn't check, but if merge is doing nothing more than spread syntax we can just remove it. It was added long time ago when spread syntax wasn't so popular(or it didn't even exist in typescript). create - in @next we've changed how entity constructors are called so it should do same thing. I think no one would look for them in utils if the same thing is achievable by using simpler syntax(and no extra import).

Kononnable avatar Jun 12 '19 19:06 Kononnable

@golergka The semantics of this library are a bit off. I rectify this on my end by creating a class called Model that extends BaseEntity, and have all my models extend Model instead of BaseEntity i.e

import {
    BaseEntity,
    ObjectType,
    ObjectID,
    FindOneOptions,
    FindConditions,
    DeepPartial,
} from 'typeorm';

export class Model extends BaseEntity {

    constructor(fields?: object) {
        super();
        if (fields) this.setFields(fields);
    }

    setFields(fields: any) {
        const entries = Object.entries(fields);
        for (const [k, v] of entries) {
            this[k] = v;
        }
    }

    /**
     * Creates a new entity instance.
     */
    static create<T extends Model>(this: ObjectType<T>): T;

    /**
     * Creates a new entities and copies all entity properties from given objects into their new entities.
     * Note that it copies only properties that present in entity schema.
     */
    static create<T extends Model>(this: ObjectType<T>, entityLikeArray: DeepPartial<T>[]): T;

    /**
     * Creates a new entity instance and copies all entity properties from this object into a new entity.
     * Note that it copies only properties that present in entity schema.
     */
    static create<T extends Model>(this: ObjectType<T>, entityLike: DeepPartial<T>): T;

    /**
     * Creates a new entity instance and copies all entity properties from this object into a new entity.
     * Note that it copies only properties that present in entity schema.
     */
    static create<T extends Model>(this: ObjectType<T>, entityOrEntities?: DeepPartial<T> | DeepPartial<T>[]): Promise<T> {
        const newT = (this as any).getRepository().create(entityOrEntities);
        return newT.save();
    }

}

In short this allows:

  • ModelChild.create to create row[s] in the DB and also return the created item[s].
  • expected const modelChildInstance = new ModelChild({...data}); behavior.

iyobo avatar Jul 28 '19 07:07 iyobo

Same here, was a bit confused on this. It's not that the docs are bad or anything, but a rename to something else like vlapo stated

Maybe just rename to createInstance or just add big note to docs. We have merge, hasId, getId. It will be shame do not have create-like method.

Also iyobo has a good point.

cuzzea avatar Nov 26 '19 17:11 cuzzea

This is very confusing

reverie avatar Mar 10 '20 22:03 reverie

I find the create method useful, it allows me to easily create multiple instances and skip having to do assignments e.g.

const values = { name: 'Bloop', email: '[email protected]' }
const myEntity = new MyEntity();
Object.assign(myEntity, values);
myEntityRepo.save(myEntity);

can simply be written as:

const myEntity = myEntityRepo
    .create({ name: 'Bloop', email: '[email protected]' })
    .save();

You can also use arrays to create multiple entity instances:

const myEntities = MyEntity.create([
    { name: 'Bloop', email: '[email protected]' },
    { name: 'Bleep', email: '[email protected]' }
]);
myEntityRepo.save(myEntities);

I personally find the create method to be a useful shortcut and don't see where the confusion persists, a quick look at the docs and you can easily work out whats going on.

cour64 avatar Mar 21 '20 15:03 cour64

@cour64 Instead of create then save, have you tried simply save? Like:

  const myEntity = myEntityRepo.save({ name: 'x', email: 'y' });

  const myEntities = myEntityRepo.save([
    { name: 'Bloop', email: '[email protected]' },
    { name: 'Bleep', email: '[email protected]' },
  ]);

zhenwenc avatar Mar 21 '20 22:03 zhenwenc

Maybe a createAndSave method will be great here.

digabriel avatar Apr 18 '20 03:04 digabriel

In my use case create allows me to create an instance with a DTO and then call save on that instance, skipping assignments.

Using only save above doesn't work in this case in my experience because it expects all properties of the entity.

NOTE: I am a noob developer


interface CreateUserDto {
    firstName: string,
    lastName: string,
    email: string,
    password: string,
  }
  
class UserModel {
    id: number;
    firstName: string;
    lastName: string;
    email: string;
    password: string;
    lastLogin: Date;
    createdAt: Date;
}

class UserService {
    async createUser(createUserDTO: CreateUserDto): Promise<UserModel> {

       return await UserModel.create(createUserDTO).save();

    }
}

AutoGnome avatar Jul 18 '20 14:07 AutoGnome

I think the create method should be marked as deprecated and removed in a later release. Same with merge.

It's confusing & provides little value. What other ORM has this?

The examples of its use in this thread can be handled by creating a base entity for your project that has a one line function. For objects, there's already a mechanism to create a new instance - a constructor.

It seems to mostly have existed as a helper for the internals to create instances & combine instances. Mostly internal subject executor and relationship loader code.

If you'd like to implement it yourself for your entities you can absolutely do that - but on the repository it doesn't make much sense when you can just as easily set up a constructor or write your own static "create" method that is less prone to errors than this one will be.

@Autognome are you sure it requires all the properties? Pretty sure it doesn't & you can call save with partials. If you can't that's a separate bug.

This works:

import {Column, Entity, PrimaryGeneratedColumn} from "../../../../src";

@Entity()
export class Address {
    @PrimaryGeneratedColumn()
    id: number;
    @Column()
    city: string;
    @Column({ nullable: true })
    street: string;
}

const addressRepository = connection.getRepository(Address);
const paris = await addressRepository.save({ city: 'Paris' });

imnotjames avatar Oct 16 '20 03:10 imnotjames

I just spent 20mins scratching my head wondering why create is not inserting any records to the DB lol

chriszhangusc avatar Jan 07 '21 03:01 chriszhangusc

I also was confused by this. Coming from activerecord, I expected that create was a combination of new and save.

benhickson avatar Mar 15 '21 21:03 benhickson

I was confused as well. But event more by the fact that there are two methods basically doing the same thing but with different return contract: insert and save. 🤣

ericmorand avatar Apr 09 '21 13:04 ericmorand

@cour64 Instead of create then save, have you tried simply save? Like:

  const myEntity = myEntityRepo.save({ name: 'x', email: 'y' });

  const myEntities = myEntityRepo.save([
    { name: 'Bloop', email: '[email protected]' },
    { name: 'Bleep', email: '[email protected]' },
  ]);

What if you want to create nested instances and then call save in the main one to do a cascade save in one operation? I think it is useful for creating instances with object's syntax but it can be confusing so maybe renaming it to createInstance should be fine.

scratchmex avatar Apr 17 '21 22:04 scratchmex

I had the same issue, it wasn't until I read this post. I don't understand, when this create() method is useful?

yes, now i understand why is useful, in fact, is really useful. I think that just is necessary to highlight in the documentation, that the create() method doesn't save, just create an object.

JhonCordoba avatar May 02 '21 16:05 JhonCordoba

Yup, same issue as everyone else here. I just spend 4 hours trying to debug why my test setup was not working until i figured out using a debugger that create does not in fact insert into the DB. This goes against basically every convention of what "create" represents. So no offense you guys - a beautiful ORM all in all, but this is a bad design decision IMHO.

Goldziher avatar May 21 '21 08:05 Goldziher

@Goldziher , create creates an entity. I think it is a good design decision. At no point do the name create implies that the entity is persisted. I agree though that I don't see a real use case to the create function. I mean, instanciating an entity basically does the same thing.

ericmorand avatar May 21 '21 08:05 ericmorand

I had the same issue, it wasn't until I read this post. I don't understand, when this create() method is useful?

yes, now i understand why is useful, in fact, is really useful. I think that just is necessary to highlight in the documentation, that the create() method doesn't save, just create an object.

JhonCordoba avatar May 24 '21 17:05 JhonCordoba

also, .create matches the Create of CRUD, which to me aligns it with "hey, something's going to happen here".

benhickson avatar May 24 '21 18:05 benhickson