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

Type ObjectID for model property #1875

Open
mrbatista opened this issue Oct 18, 2018 · 31 comments

Comments

Projects
None yet
10 participants
@mrbatista
Copy link
Contributor

commented Oct 18, 2018

Is there a support in Loopback 4 for ObjectID type when using a mongoDB database?

@marioestradarosa

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

I created a mongoDB DataSource, my model contains the following:

@model()
export class Customer extends Entity {
  @property({
    type: 'string',
    id: true,
  })
  _id?: string;

  @property({
    type: 'string',
  })
  fname?: string;

  constructor(data?: Partial<Customer>) {
    super(data);
  }

and I could post some records _(without including id property, since it is automatically generated by mongoDB) and was able to retrieve some data:

[{"_id":"5bc8e9ac964667e062645686","fname":"mario"},{"_id":"5bc8e9b8964667e062645687","fname":"laura"}]

Is this what you meant? , or you meant a direct manipulation of the ObjectID object by the connector?
https://docs.mongodb.com/manual/reference/method/ObjectId/

@mrbatista

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

@marioestradarosa I want to define the property type as ObjectID:

@model()
export class Customer extends Entity {
  @property({
    type: 'ObjectID',
    id: true,
  })
  _id?: ObjectID;

  @property({
    type: 'string',
  })
  fname?: string;

  @property({
    type: 'ObjectID',
  })
  userId?: ObjectID;

  constructor(data?: Partial<Customer>) {
    super(data);
  }
}

In the mongoDB connector there is isObjectIDProperty method that check the type of property. This is the rilevant line.
If you check in the database the type of property _id of your example model is string and not ObjectID. In order to force coercion it is necessary to set decorator to:

@model({settings: {strictObjectIDCoercion: true})
@marioestradarosa

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

@mrbatista for what I understood in the documentation for this connector the flag {strictObjectIDCoercion: true} is only used to avoid the automatic conversion of strings (to ObjectID) of length 12 and 24 and have the format /^[0-9a-fA-F]{24}$/. please correct me if I'm wrong.

ObjectID however, can't be set as the property type directly in the model for now.

If you check in the database the type of property _id of your example model is string and not ObjectID

But in the mongodb, it was converted to object id automatically. Architecturally, by default the _id field is an ObjectID.

{
    "_id": {
        "$oid": "5bc8e9ac964667e062645686"
    },
    "fname": "mario"
}

In your case, are you looking to provide these $oid values automatically to MongoDB? like in your case where you specified _id and userId ?

@marioestradarosa

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

@israelglar

This comment has been minimized.

Copy link

commented Oct 25, 2018

I'm facing a similar problem.
To be able to create models with id: ObjectId what I did was create:
mongo-entity.model.ts

@model()
export class MongoEntity extends Entity {
  @property({
    type: 'string',
    id: true
  })
  id: string;

  constructor(data?: Partial<MongoEntity>) {
    super(data);
  }
}

mongo-entity.repository.ts

const ObjectId = require('mongodb').ObjectId

export class MongoEntityRepository<E extends MongoEntity, ID>
  extends DefaultCrudRepository<E, ID> {

  async create(entity: E): Promise<E> {
    entity.id = new ObjectId()
    return super.create(entity)
  }
}

And now every Model extends MongoEntity and every Repository extends MongoEntityRepository.
But I can't do the same thing for relations.

Im my case, I'm trying to create a token authentication similar to lb3 with 2 models Profile and AccessToken.
AccesToken belongsTo Profile

access-token.model.ts

@model()
export class AccessToken extends MongoEntity {
  //Other properties

  @belongsTo(() => Profile)
  userId: string;

  constructor(data?: Partial<AccessToken>) {
    super(data);
  }
}

access-token.repository.ts

export class AccessTokenRepository extends DefaultCrudRepository<
  AccessToken,
  typeof AccessToken.prototype.id
  > {
  public readonly user: BelongsToAccessor<
    Profile,
    typeof AccessToken.prototype.userId
    >;

  async create(token: AccessToken): Promise<AccessToken> {
    token.id = jwt.sign({ userId: token.userId }, 'secret')
    return super.create(token)
  }

  constructor(
    @inject('datasources.GlarDB') dataSource: GlarDBDataSource,
    @repository.getter('ProfileRepository')
    profileRepositoryGetter: Getter<ProfileRepository>,
  ) {
    super(AccessToken, dataSource);
    this.user = this._createBelongsToAccessorFor(
      'userId',
      profileRepositoryGetter,
    );
  }
}

profile.controller.ts This is where the AccessToken is created

@post('/profiles/login', {
    responses: {
      '200': {
        description: 'Profile model instance',
        content: { 'application/json': { 'x-ts-type': Profile } },
      },
    },
  })
  async login(
    @requestBody() profile: { username: string, password: string }
  ): Promise<AccessToken> {
    const profileExists: Profile | null = await this.profileRepository.findOne({
      where: {
        username: profile.username,
        password: profile.password
      }
    })

    if (!profileExists) throw new HttpErrors.NotFound()

    const tokenCreated = new AccessToken({ userId: profileExists.id })
    return await this.accessTokenRepository.create(tokenCreated)
  }

The documents created in Mongo are:
The profile.id a ObjectId:

{
    "_id" : ObjectId("5bd19395171e9d5560960712"),
    "username" : "usertest",
    "password" : "passtest",
    "name" : "nametest",
    "email" : "email@test.com"
}

But the accessToken.userId is a string:

{
    "_id" : "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VySWQiOiI1YmQwODVhMTk4M2NmNDVhNTAyOGE1NGUiLCJpYXQiOjE1NDA0NjEyNzl9.C-PPE9_WezZU4Wvo5J6Qm4m-GukKQpch7-5cliq9ZeY",
    "ttl" : 1209600,
    "created" : ISODate("2018-10-25T09:54:39.787Z"),
    "userId" : "5bd085a1983cf45a5028a54e"
}

I noticed that _createBelongsToAccessorFor return type is <Profile, string> because the AccesToken.userId type is string.
If a ObjectId type existed, maybe that would be fixed. I already tried to create/import one, but could't figure how to make it work with the belongsTo relation.

@marioestradarosa Any help on that subject? Thanks

@marioestradarosa

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Hi @israelglar , thanks for the explanation on the Object ID. I will try to work on an exampled based on your code. I see now what @mrbatista was saying about the Object ID.

I see the initial id was created perfectly because its ID property happens to be id and in the class you explicitly say entity.id = new ObjectId(). So the problem is that the userId foreign key is string and should contain also the same value as the id in the profile, is this correct?.

If the latter is correct, then _id" : ObjectId("5bd19395171e9d5560960712"), should be in the tokens "userId" : "ObjectId("5bd19395171e9d5560960712") ?

@israelglar

This comment has been minimized.

Copy link

commented Oct 26, 2018

That's exactly the problem. I already tried many things to make the userId an ObjectId like the Id but with no success :/
Good luck :D I hope you can do it!

@mrbatista

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2018

@israelglar set userId as type any

@israelglar

This comment has been minimized.

Copy link

commented Oct 29, 2018

@mrbatista Nice, it works :) I need to set userId as type any and create a new ObjectId on AccessToken creation like this:
access-token.model.ts

@belongsTo(() => Profile)
  userId: any; // tslint:disable-line

access-token.repository.ts

async create(token: AccessToken): Promise<AccessToken> {
  token.id = jwt.sign({userId: token.userId}, 'secret', {expiresIn: '7d'});
  token.userId = new ObjectId(token.userId);
  return super.create(token);
}

It's a bit of a inconvenience to do this for every model and lose the type on userId, but it's a solution for now.

@marioestradarosa You know if there's a possibility to include a ObjectId type support on lb4?

@marioestradarosa

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

You know if there's a possibility to include a ObjectId type support on lb4?

Yes, that's the original issue all about. Thanks @israelglar for making the example and @mrbatista to help on this. @bajtos already labeled it.

I believe that this new type should also be listed when creating the model when mongo DB datasources are selected, or is this ObjectID something already implemented in other document oriented databases?

@bajtos

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

I believe that this new type should also be listed when creating the model

+1

is this ObjectID something already implemented in other document oriented databases?

AFAIK, ObjectID is MongoDB specific. However, we allow relations across different databases, e.g. a "Customer" model stored in MongoDB can have many "Order" instances stored in MySQL, therefore we should support ObjectID properties also for models stored in different databases.

@codejamninja

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

You could try adding the strictObjectIDCoercion when you run one of the repository methods.

Please reference the following issue.

#2085

@sertal70

This comment has been minimized.

Copy link

commented Dec 7, 2018

Working on the TodoList tutorial, I tried to set strictObjectIDCoercion in the Todo model but when I call the endpoint:

http://localhost:3000/todo-lists/5c0a938efc0c0d7f21bbcac7/todos

to POST the Todo:

{
  "title": "wine"
}

it results in the following document inserted in Todo collection:

{
    "_id" : ObjectId("5c0ab25ab6db018f62e85fc6"),
    "title" : "wine",
    "todoListId" : {
        "0" : "5",
        "1" : "c",
        "2" : "0",
        "3" : "a",
        "4" : "9",
        "5" : "3",
        "6" : "8",
        "7" : "e",
        "8" : "f",
        "9" : "c",
        "10" : "0",
        "11" : "c",
        "12" : "0",
        "13" : "d",
        "14" : "7",
        "15" : "f",
        "16" : "2",
        "17" : "1",
        "18" : "b",
        "19" : "b",
        "20" : "c",
        "21" : "a",
        "22" : "c",
        "23" : "7"
    }
}

Instead I found that the @israelglar workaround works very well, but it must be extended to all methods where the foreign key is involved: find(), delete(), etc.

For instance here there is the find() method on TodoRepository to make working the endpoint GET /todo-lists/{id}/todos:

async find(filter?: Filter<Todo> | undefined, options?: AnyObject | undefined): Promise<Todo[]> {
  if (filter && filter.where && filter.where.todoListId) {
    filter.where.todoListId = new ObjectId(filter.where.todoListId);
  }
  return super.find(filter, options);
}

It can result in writing a lot of (potentially unnecessary) code, I hope that ObjectId type will be exposed soon in model definition to avoid this.

@codejamninja

This comment has been minimized.

Copy link
Member

commented Dec 8, 2018

@sertal70 did you look at #2085 (comment) ?

@sertal70

This comment has been minimized.

Copy link

commented Dec 8, 2018

@codejamninja sure I did and really don't understand why the setting strictObjectIDCoercion: true didn't work in my case. On the other hand, the proposed workaround requires to load all records and then filter out those that are not relevant to the relation, it is something not really feasible in production when you have to deal with thousand of records and API calls.

As my test proves, to some extent ObjectId data type is already managed by the framework but for some reason it is not exposed up to the model definition section. Maybe the fix could be really simple...

@codejamninja

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

@sertal70, can you send a link to an example application using ObjectId. I don't think I fully understand how to do it.

@sertal70

This comment has been minimized.

Copy link

commented Dec 9, 2018

Sure @codejamninja I uploaded the modified tutorial in a public repo here.

Remember to set a valid MongoDB URI in mongodb.datasource.json here before running it.

@codejamninja

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Cool, thanks

@bajtos bajtos added the db:MongoDB label Dec 11, 2018

@sertal70

This comment has been minimized.

Copy link

commented Dec 18, 2018

@codejamninja did you have the chance to look at my repo? What's your thought about it?

@codejamninja

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

I did. It seems like a much simpler approach. I actually haven't got a chance to test it yet. I will hopefully get around to it sometime this week.

Does the this._createHasManyRepositoryFactoryFor end up using the overridden find function?

For example, will the following code . . .

    return await this.todoListRepo.todos(id).find(filter);

https://github.com/sertal70/lb4-todolist-backend/blob/master/src/controllers/todo-list-todo.controller.ts#L56

. . . end up executing find() function in the todo repository?

  async find(filter?: Filter<Todo> | undefined, options?: AnyObject | undefined): Promise<Todo[]> {
    if (filter && filter.where && filter.where.todoListId) {
      filter.where.todoListId = new ObjectId(filter.where.todoListId);
    }
    return super.find(filter, options);
  }

https://github.com/sertal70/lb4-todolist-backend/blob/master/src/repositories/todo.repository.ts#L22

@sertal70

This comment has been minimized.

Copy link

commented Dec 19, 2018

Yes, it does.

@dhmlau dhmlau added the p1 label Feb 12, 2019

@dhmlau dhmlau added 2019Q2 and removed TOB labels Feb 12, 2019

@Shamanpreet

This comment has been minimized.

Copy link

commented Apr 3, 2019

import {
	DefaultCrudRepository,
	BelongsToAccessor,
	repository,
} from '@loopback/repository';
import {User, Accesstoken} from '../models';
import {UserRepository} from '../repositories';
import {DbDataSource} from '../datasources';
import {inject,	Getter,} from '@loopback/core';

var jwt =require('jwt-decode');

export class AccesstokenRepository extends DefaultCrudRepository<
  Accesstoken,
  typeof Accesstoken.prototype.id
> {
  public readonly user: BelongsToAccessor<
    User,
    typeof Accesstoken.prototype.user_id
    >;

  async create(token: Accesstoken): Promise<Accesstoken> {
    token.id = jwt.sign({ user_id: token.user_id }, 'secret', {expiresIn: '7d'});
    token.user_id = new Object(token.userId);
    return super.create(token)
  }	  

  constructor(
    @inject('datasources.db') dataSource: DbDataSource,
    @repository.getter('UserRepository')
     userRepositoryGetter: Getter<UserRepository>,
  ) {
    super(Accesstoken, dataSource);
    this.user = this.createBelongsToAccessorFor(
      'user_id',
      userRepositoryGetter,
    );
  }
}
@Shamanpreet

This comment has been minimized.

Copy link

commented Apr 3, 2019

please help not genrate token id

@codejamninja

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

@Shamanpreet please format your code in a code block. It's very hard to read.

You can format it in a code block using three backticks.

```ts
// my code here
```
@emonddr

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Folks still asking about #2085 .

@emonddr

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

  1. We will investigate for a quick workaround and document it : a) use a workaround listed here or b) make use of a property decorator
  2. Long term solution : #1306
@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

With the above comment, I'd like to propose the following acceptance criteria:

Acceptance Critera

  • Review the workaround listed in this ticket
  • Identify which approach is the best and then document it.
@bajtos

This comment has been minimized.

Copy link
Member

commented May 7, 2019

ObjectID has been always problematic, see the following threads from LoopBack 1.x/2.x/3.x days:

@bajtos

This comment has been minimized.

Copy link
Member

commented May 7, 2019

In #2085 (comment), I am proposing the following pattern for defining ObjectID properties:

@model({
  settings: {
    strictObjectIDCoercion: true
  }
})
export class User extends Entity {
  @property({
    type: 'string',
    id: true,
    mongodb: {
      dataType: 'ObjectID' // or perhaps 'objectid'?
    }
  })
  id?: string;
  // ...
}

In the mongoDB connector there is isObjectIDProperty method that check the type of property. This is the rilevant line.

The full implementation of MongoDB.prototype.isObjectIDProperty can be found here: https://github.com/strongloop/loopback-connector-mongodb/blob/efbee59bfe8362822483d5e1a0867e90c1b00c4d/lib/mongodb.js#L1852-L1871

AFAICT, this code will not recognize connector-specific property settings at the moment. Hopefully, it should be an easy fix to implement.

I would open a PR myself, but don't have enough time for that right now. I think the most time-consuming part is to write good tests for this new behavior. We should test at least Model.find (queries) and Model.create (updates). For queries, it would be good to test non-trivial where conditions like {where: {id: { inq: ['my-objectid-1', 'my-objectid-2'] }}} in addition to trivial {where: {id: 'my-objectid'}}.

@JesusTheHun

This comment has been minimized.

Copy link

commented May 7, 2019

I am more seduced by your second proposal of using a correct property type. This is far more intuitive and is what I expect from Loopback : make my life easier, not punish me for my database choice.

MongoDB is a very common database should enjoy all the fun of LoppBack 💯

Databse popularity, StackOverflow 2018
Databse popularity, StackOverflow 2019

@dhmlau dhmlau added 2019Q3 and removed 2019Q2 p1 labels May 14, 2019

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.