POST (createOne) operation semantics behaviour for a resource that already exists

[dcs3spp]

Hi,

I am new to the library and have a query regarding the default semantics/behaviour of POST (createOne) in typeorm-crud.service.ts. This uses the save method on the typeorm repository, which means that if the resource already exists then existing property values are updated.

public async createOne(req: CrudRequest, dto: T): Promise<T> {
    const entity = this.prepareEntityBeforeSave(dto, req.parsed.paramsFilter);

    /* istanbul ignore if */
    if (!entity) {
      this.throwBadRequestException(`Empty data. Nothing to save.`);
    }

    return this.repo.save<any>(entity);
}

Does the crud library provide a way to override this behaviour so that if a resource already exists then a 409 error status is thrown?

I have included the source code below that I am using for my entity, controller and service....

Entity

@Entity('course', {schema: 'coursemanagement' })
export class Course {
    @ApiModelProperty()
    @IsDefined({ groups: [CREATE, UPDATE] })
    @IsNumber({}, { always: true })
    @Column('integer', {
        name: 'CourseID',
        nullable: false,
        primary: true,
        })
    public CourseID: number = 0;

    @ApiModelProperty()
    @IsDefined({ groups: [CREATE, UPDATE] })
    @IsString({ always: true })
    @Column('character varying', {
        length: 50,
        name: 'CourseName',
        nullable: false,
        })
    public CourseName: string = '';
}
export default Course;

Controller

import { Controller, Inject } from '@nestjs/common';
import { CourseService } from './course.service';
import { Crud, CrudController, Override, ParsedRequest, ParsedBody, CrudRequest } from '@nestjsx/crud';
import { Course } from './course.entity';
import { Logger } from 'winston';

@Crud({
  model: {
    type: Course,
  },
  params: {
    id: {
      field: 'CourseID',
      type: 'number',
      primary: true,
    },
  },
  routes: {
    only: [ 
      'createOneBase', 
      'deleteOneBase', 
      'getManyBase',
      'getOneBase',
      'updateOneBase', 
    ]
  }
})
@Controller('course')
export class CourseController implements CrudController<Course> {
  constructor(@Inject('winston') private readonly logger: Logger, public service: CourseService) {
}

  /**
   * Provide intellisense, see https://github.com/nestjsx/crud/wiki/Controllers#intellisense
   */
  get base(): CrudController<Course> {
    return this;
  }
}

Service

import { InjectRepository } from '@nestjs/typeorm';
import { Logger } from 'winston';
import { TypeOrmCrudService } from '@nestjsx/crud-typeorm';

import { Course } from './course.entity';
import { CourseRepository} from './course.repository';

@Injectable()
export class CourseService extends TypeOrmCrudService<Course> {
  constructor(@Inject('winston') private readonly logger: Logger,
              @InjectRepository(CourseRepository) private readonly courseRepository: CourseRepository) {
    super(courseRepository);
  }
}

[dcs3spp]

Not sure if there is a better way but ended up implementing my own version via inheriting from TypeOrmCrudService to provide the semantics expected of POST operation.

Is there any progress relating to feature requests #226 and #209 to facilitate overriding behaviour in TypeOrmCrudService?

export class MyTypeOrmCrudService<T> extends TypeOrmCrudService<T> {

  constructor(protected repo: Repository<T>) {
    super(repo);
  }

  /**
   * Create one
   * The createOne method is overriden to provide the following semantics.
   * If the entity exists a ConflictException is thrown.
   * If the entity does not exist then it is saved in the repository.
   * 
   * Requires typescript 3.1.6 due to a bug/issue with typescript compiler
   * and the typeorm library when using `this.repo.findOne(entity)` 
   * Refer to:
   * - https://github.com/microsoft/TypeScript/issues/21592
   * - https://github.com/typeorm/typeorm/pull/4470 
   * 
   * If using VSCode, use local typescript compiler in settings.json file
   * `{
   * "typescript.tsdk": "node_modules/typescript/lib"
   * }`
   * 
   * Alternatively, modify typeorm-find-options/FindConditions.d.ts as detailed in code snippet 
   * below to address excessive stack depth error when using this.repo.findOne(entity)
   * see typeorm issue #4470 @ https://github.com/typeorm/typeorm/pull/4470 (awaiting merge)
   * `export declare type FindConditions<T> = {
   *     [P in keyof T]?: T[P] extends never ? FindConditions<T[P]> | FindOperator<FindConditions<T[P]>> : FindConditions<T[P]> | FindOperator<FindConditions<T[P]>>;
   * };`
   * @param req
   * @param dto
   */
  public async createOne(req: CrudRequest, dto: T): Promise<T> {
    const entity: T = this.createEntity(dto, req.parsed.paramsFilter);

    /* istanbul ignore if */
    if (!entity) {
      this.throwBadRequestException(`Empty data. Nothing to save.`);
    }

    // we have to pass entity as any here to avoid typeorm issue #4470 with
    // typescript compiler 3.1.6
    const result = await this.repo.findOne(<any>entity);
    if (result) {
      this.throwConflictException('Attempt to save duplicate entity');
    }
    return this.repo.save<any>(entity);
  }

  private get entityClassType(): ClassType<T> {
    return this.repo.target as ClassType<T>;
  }

  protected createEntity(dto: T, paramsFilter: QueryFilter[]): T {
    /* istanbul ignore if */
    if (!isObject(dto)) {
      return undefined;
    }

    if (hasLength(paramsFilter)) {
      for (const filter of paramsFilter) {
        dto[filter.field] = filter.value;
      }
    }

    /* istanbul ignore if */
    if (!hasLength(objKeys(dto))) {
      return undefined;
    }

    return dto instanceof this.entityClassType ? dto : plainToClass(this.entityClassType, dto);
  }

  protected throwConflictException(name: string): ConflictException {
    throw new ConflictException(`${name} not found`);
  }
}

[zarv1k]

Hey guys! I think this is not a case this library should handle. As for me it should be done by the validation layer (class-validator) which is used by crud library.

Here is some example of implementing such a kind of Unique validator. As you can see, it could be used not for just for checking primary keys uniqueness. Moreover such an approach does not require any modifications of this library at all.

This approach is flexible and works perfect.

Also note that it won't work unless you set up DI container (useContainer from 'class-validator' package) as described here

[dcs3spp]

Thanks for responding @zarv1k, appreciated and very helpful, thank you :)

I will take a look at the validators approach for future consideration, since I have already adopted a solution that derives from TypeOrmCrudService. I can now use Typeorm 3.6.3 which fixes the bug described in the document header of my custom class, MyTypeOrmCrudService::createOne, posted earlier in this issue.

Moving forward, whatever is decided, I think it would be useful if the official documentation for the library explained the default semantics for createOne and highlighted the options for overriding the default behaviour, e.g. via validators. This would be useful for new users that are considering adopting the library.

[cleivson]

Hey guys! I think this is not a case this library should handle. As for me it should be done by the validation layer (class-validator) which is used by crud library.

Here is some example of implementing such a kind of Unique validator. As you can see, it could be used not for just for checking primary keys uniqueness. Moreover such an approach does not require any modifications of this library at all.

This approach is flexible and works perfect.

Also note that it won't work unless you set up DI container (useContainer from 'class-validator' package) as described here

Although I agree that most of the validation must be done in the validation layer, like uniqueness of a username or something like this, I don't think we should reimplement the logic for checking if a primary key is unique or not.

That's counter intuitive for most of the databases that provide CRUD capabilities and it's a bug for me since when one calls the createOne method he does not expect to update a resource, he's probably in the context of creating a new resource.

I believe all of this could be resolved by changing the call to save method as pointed by OP for a call to insert method.

[dcs3spp]

I agree with the points made by @cleivson. For new users like myself, this was confusing and counter intuitive. Whatever outcome is decided, I think that the official documentation should definitely explain the current default semantics for the createOne method to save new users some time.

From the perspective of new users or businesses that are considering using the library.......if the decision is made that this is not a bug then it would be helpful if workarounds for overriding the default behaviour were officially documented and/or demonstrated with a small example playground repository/application.

Near the time of originally posting I tried using insert but encountered problems with cascades and relations...

/**
 * Inserts a given entity into the database.
 * Unlike save method executes a primitive operation without cascades, relations and other operations included.
 * Executes fast and efficient INSERT query.
 * Does not check if entity exist in the database, so query will fail if duplicate entity is being inserted.
 */
insert(entity: QueryDeepPartialEntity<Entity>|(QueryDeepPartialEntity<Entity>[])): Promise<InsertResult> {
    return this.manager.insert(this.metadata.target as any, entity);
}

[michaelyali]

yeah, friends, that's all pretty much interesting. I have several questions from my side too:

1. How can we check for entity existence in a DB if there is only one autogenerated primary param and it's not passed in the request body?
2. If we give up on typeorm save method, how should we save relational data that might come in the request body (I suppose, reinvent our own save method, right?)

[dcs3spp]

Hi @zMotivat0r,

1. The requirements of my application uses a natural primary key. A good point. In that scenario is there any need to check for a pre-existing entity before creating, since the entity has an autogenerated id as the primary key. As a new user do not know the intricate details of the @nestjsx/crud source code very well......Maybe, allow the semantics to be specified as an additional property in the model section of the Crud decorator? The createOne method could check for the presence of this additional property. If the property is present the createOnemethod bypasses checking for pre-existence of the entity since the semantics have been specified that it uses an autogenerated id???
2. Basically in agreement with you. Typeorm insert does not do cascades or relations. Therefore I decided to use the typeorm save method in the code posted earlier. Why reinvent the wheel, typeorm save handles all cascades and relations automatically. Typeorm save updates the entity if it pre-exists hence the need for the findOne operation in code from original posting.