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

Closed
mrbatista opened this issue Oct 18, 2018 · 34 comments
Closed

Type ObjectID for model property #1875

mrbatista opened this issue Oct 18, 2018 · 34 comments
Labels
db:MongoDB Topics specific to MongoDB feature Repository Issues related to @loopback/repository package

Comments

@mrbatista
Copy link
Contributor

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

@marioestradarosa
Copy link
Contributor

marioestradarosa 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
Copy link
Contributor Author

@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
Copy link
Contributor

marioestradarosa 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
Copy link
Contributor

So, in this method https://github.com/strongloop/loopback-connector-mongodb/blob/efbee59bfe8362822483d5e1a0867e90c1b00c4d/lib/mongodb.js#L1852 we never reach the return true on line 1858 for now.

@israelglar
Copy link

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
Copy link
Contributor

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
Copy link

israelglar 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
Copy link
Contributor Author

mrbatista commented Oct 28, 2018

@israelglar set userId as type any

@israelglar
Copy link

israelglar 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?

@bajtos bajtos added feature Repository Issues related to @loopback/repository package TOB labels Oct 30, 2018
@marioestradarosa
Copy link
Contributor

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
Copy link
Member

bajtos 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.

@clayrisser
Copy link
Contributor

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

Please reference the following issue.

#2085

@sertal70
Copy link

sertal70 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.

@clayrisser
Copy link
Contributor

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

@sertal70
Copy link

sertal70 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...

@clayrisser
Copy link
Contributor

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

@sertal70
Copy link

sertal70 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.

@clayrisser
Copy link
Contributor

Cool, thanks

@bajtos bajtos added the db:MongoDB Topics specific to MongoDB label Dec 11, 2018
@sertal70
Copy link

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

@clayrisser
Copy link
Contributor

clayrisser 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
Copy link

Yes, it does.

@dhmlau dhmlau added the p1 label Feb 12, 2019
@dhmlau dhmlau added 2019Q2 and removed TOB labels Feb 12, 2019
@Shamanpreet
Copy link

Shamanpreet 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
Copy link

please help not genrate token id

@clayrisser
Copy link
Contributor

@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
Copy link
Contributor

emonddr commented Apr 17, 2019

Folks still asking about #2085 .

@emonddr
Copy link
Contributor

emonddr 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 : Implement coercion between HTTP and LB4 layers(converter and deserialize) #1306

@dhmlau
Copy link
Member

dhmlau 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
Copy link
Member

bajtos commented May 7, 2019

@bajtos
Copy link
Member

bajtos 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
Copy link

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 labels May 14, 2019
@dhmlau
Copy link
Member

dhmlau commented May 15, 2019

@bajtos
Copy link
Member

bajtos commented Sep 12, 2019

A short-term fix has been implemented in the MongoDB connector, the solution is to define your ObjectID properties as {type: 'string', mongodb: {dataType: 'ObjectID'}}.

Unfortunately, this does not work for the primary key (id), where the juggler will always define the PK property as {type: ObjectID}. Based on our experience so far, this should not be a problem in real-world usage.

We are going investigate further improvements as part of #3720.

I am closing this issue as resolved.

@Frankie0701145
Copy link

@bajtos You are saying the foreign key should be defined likes.
@Property({
type: 'string',
required: true,
mongodb: {
dataType: 'ObjectID'
}
})
userId: any

@Frankie0701145
Copy link

The type to be any

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
db:MongoDB Topics specific to MongoDB feature Repository Issues related to @loopback/repository package
Projects
None yet
Development

No branches or pull requests