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

Mongo fails to find user by id (hasMany relation not working) #2085

Closed
clayrisser opened this issue Nov 27, 2018 · 25 comments
Closed

Mongo fails to find user by id (hasMany relation not working) #2085

clayrisser opened this issue Nov 27, 2018 · 25 comments
Assignees
Labels
bug db:MongoDB Topics specific to MongoDB major Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package

Comments

@clayrisser
Copy link
Contributor

clayrisser commented Nov 27, 2018

Description / Steps to reproduce / Feature proposal

When I set stringObjectIDCoercion to true, I get an error when looking up the user by id when using mongo as the datasource.

I need to set stringObjectIDCoercion to true because if I don't, one-to-many relationships don't work in mongo.

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

  @property({
    type: 'string',
    required: true
  })
  username: string;

  @property({
    type: 'string',
    required: true
  })
  password: string;

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

Current Behavior

{
  "error": {
    "statusCode": 404,
    "name": "Error",
    "message": "Entity not found: User with id \"5bfd9b3e4a7fe2484152e30c\"",
    "code": "ENTITY_NOT_FOUND"
  }
}

Expected Behavior

{
  "id": "string",
  "username": "string",
  "password": "string"
}

I have tried setting the id type to ObjectID when a document is created, but it doesn't seem to fix it.

I think this issue is related to #1875

@clayrisser
Copy link
Contributor Author

clayrisser commented Nov 29, 2018

Ok, after much digging ⛰️⛏️ I figured out where the issue is, although I'm not exactly sure which library is responsible for fixing it.

Basically, if you are using the mongo juggler adapter in loopback 4, and decide to implement a one-to-many relationship, the find() function in the HasManyRepository will not work. By not work, I mean that it will return an empty array [] even when the records exist in the database.

https://github.com/strongloop/loopback-next/blob/master/packages/repository/src/relations/has-many/has-many.repository.ts#L86-L95

A solution to this problem is to add the strictObjectIDCoercion flag to the model settings.

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

While this solution does indeed work, it breaks the findById() function and possibly other functions in the DefaultCrudRepository. Specifically, it is not able to find records in the database by their id.

https://github.com/strongloop/loopback-next/blob/master/packages/repository/src/repositories/legacy-juggler-bridge.ts#L236-L244

The only way I have been able to reconcile these two "bugs" is to only pass the strictObjectIDCoercion flag when calling the find() function from the HasManyRepository.

const devices = await this.userRepository.devices(userId).find(
  {},
  {
    strictObjectIDCoercion: true
  }
);

However, please note that even when I do this, I still cannot look up a device by its id (other properties do work). So, for example, the following will not work.

const devices = await this.userRepository.devices(userId).find(
  { id: deviceId },
  {
    strictObjectIDCoercion: true
  }
);
console.log('devices', devices); // returns []

So, for now, to look up a has-many relationship by its id, I have to find all of them and then filter for the id. This works, but it is certainly not ideal because I have to load everything into memory.

@get('/users/{userId}/devices/{deviceId}')
async findById(
  @param.path.string('userId') userId: string,
  @param.path.string('deviceId') deviceId: string
): Promise<Device> {
  const devices = await this.userRepository.devices(userId).find(
    {},
    {
      strictObjectIDCoercion: true
    }
  );
  const device = _.find(devices, device => device.id === deviceId);
  if (!device) throw new Error(`failed to find device '${deviceId}' on user '${userId}'`);
  return device;
}

So in summary, I do not want to have to add the strictObjectIDCoercion flag every time I want to use the find() function in the HasManyRepository. I also would like to be able to find a has-many relationship by the id of the item.

This "bug" could be fixed in one of the following projects, but I'm not sure which one is most ideal to fix it from.

@loopback/repository
loopback-datasource-juggler
loopback-connector
loopback-connector-mongodb

@bajtos bajtos added bug Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package major TOB labels Dec 4, 2018
@bajtos bajtos changed the title Mongo fails to find user by id Mongo fails to find user by id (hasMany relation not working) Dec 4, 2018
@bajtos
Copy link
Member

bajtos commented Dec 4, 2018

@codejamninja thank you for reporting this issue, it looks like a major bug to me!

It makes me wonder what's the difference between LB3 (where this works) and LB4 (where it does not). Is there perhaps a difference in the way how the model properties are defined (described for the connector)?

@clayrisser
Copy link
Contributor Author

I'm not exactly sure. Could you give me some things to look for?

@b-admike
Copy link
Contributor

b-admike commented Dec 5, 2018

@codejamninja Thank you for the detailed steps on the issue and in order to find the discrepancies between LB3 and LB4 as @bajtos, I thinking using something like https://github.com/strongloop/loopback-example-relations to check would be useful. I'm going to try it myself if I find some time later today.

@b-admike
Copy link
Contributor

b-admike commented Dec 5, 2018

Also related #1987. @loredanacirstea @jormar, my proposed solution at #1987 (comment) is not comprehensive (see #2085 (comment) above):

A solution to this problem is to add the strictObjectIDCoercion flag to the model settings.

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

While this solution does indeed work, it breaks the findById() function and possibly other functions in the DefaultCrudRepository. Specifically, it is not able to find records in the database by their id.

@ambienthack
Copy link

ambienthack commented Dec 22, 2018

Consider the following solution:

  1. Add 'isForeignKey' to PropertyDefinition of foreign key, inside belongsTo decorator
   const propMeta: PropertyDefinition = {
      ....

      isForeignKey: true,

    };
  1. Check for 'isForeignKey' inside mongodb connector
MongoDB.prototype.isObjectIDProperty = function (model, prop, value, options) {
  if (
    prop &&
    (prop.type === ObjectID ||
      (Array.isArray(prop.type) && prop.type[0] === ObjectID))
  ) {
    return true;
  } else if (prop && prop.isForeignKey === true) {
    return false;
  }
  ......
}

@theiosdevguy

This comment has been minimized.

@bajtos

This comment has been minimized.

@dhmlau
Copy link
Member

dhmlau commented Apr 29, 2019

@hacksparrow , i'm assigning this to you because you're working on this already.

@hacksparrow
Copy link
Contributor

Note for anyone trying to reproduce the issue

While creating a User, if you omit the id field, MongoDB will generate an id for the new user. If you query the generated id at /users/{id}, you will encounter 404.

If you specify the id value manually, querying /users/{id}, works as expected (200).

@imonildoshi
Copy link

imonildoshi commented May 6, 2019

I have found a solution to this, but need your inputs.

While finding a String which is actually an objectId, loopback-connector-mongodb converts it to ObjectId and then finds, which is perfect.

But while inserting, if a string is a foreign key and not an Id, it is not converted to ObjectId

What do you recommend? should we convert all the properties of regex /^[0-9a-fA-F]{24}$/ to objectId and then save

@bajtos @hacksparrow what do you suggest, should we convert all the strings into ObjectId while saving in mongodb connector?

@hacksparrow
Copy link
Contributor

Thanks for the input, @imonildoshi. I am looking at the possibilities.

@hacksparrow
Copy link
Contributor

hacksparrow commented May 7, 2019

Here is a technical breakdown of the bug. It all boils down to the strictObjectIDCoercion setting.

When strictObjectIDCoercion is false (default):

  • If id value is specified when creating a model, and the value doesn't look like a MongoDB ObjectId, it will be stored as a string in the database.
  • If id value looks like a MongoDB ObjectId, it will be converted to ObjectId when writing to or reading from the database.
  • If id value is not specified when creating a model, an ObjectId id will be generated for the model.
  • On querying /customers/{id}, if id looks like an ObjectId, the database will be queried for id of type ObjectId, else it will be queried for id of type string.

When strictObjectIDCoercion is true:

  • An id value looking like a MongoDB ObjectId makes no difference, it will be interpreted and stored as a string.
  • If id value is not specified when creating a model, an ObjectId id will be generated for the model automatically.
  • On querying /customers/{id}, even if id looks like an ObjectId, the database will be queried for id of type string.
  • Since automatically generated ids are of type ObjectId, they will never be found by /customers/{id}, when strictObjectIDCoercion is set to true.

Note: strictObjectIDCoercion is required to prevent involuntary conversion of a string that looks like a ObjectId to ObjectId.

@bajtos
Copy link
Member

bajtos commented May 7, 2019

I think the problem may be (partially) caused by the fact that the id property is declared with type string.

IIRC, LoopBack 3.x models usually did not define the id property explicitly, instead they relied on the connector to add that property - see https://github.com/strongloop/loopback-connector-mongodb/blob/3a79606ad67fc175020e5b0c20007a9771545bbb/lib/mongodb.js#L333-L335:

MongoDB.prototype.getDefaultIdType = function() {
  return ObjectID;
};

IMO, to support MongoDB, the id property should be declared as follows:

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

Maybe even using ObjectID class directly, but I am not sure how well will this play with the rest of LoopBack 4:

import {ObjectID} from 'loopback-connector-mongodb';

@model({
  settings: {
    strictObjectIDCoercion: true
  }
})
export class User extends Entity {
  @property({
    type: ObjectID,
    id: true,
  })
  id?: ObjectID;
  // ...
}

Recently, @b-admike was implementing coercion for decimal128 values in the MongoDB connector (see e.g. loopbackio/loopback-connector-mongodb#501, loopbackio/loopback-connector-mongodb#498 and other related pull requests). I believe the mechanism for decimal128 coercion works both for queries and writes and I think it should be easy to extend to support ObjectID type too, in case ObjectID does not work out of the box as I described in my first proposal.

@b-admike @raymondfeng @hacksparrow @codejamninja thoughts?

@bajtos
Copy link
Member

bajtos commented May 7, 2019

In #1875 (comment), I am proposing to fix MongoDB.prototype.isObjectIDProperty to recognize MongoDB-specific dataType setting in model property definition.

@bajtos
Copy link
Member

bajtos commented May 7, 2019

I am arguing that the current behavior is actually correct. Because stringObjectIDCoercion is enabled, and the id property is declared with type string, the connector is preserving string values as strings, exactly as told by the model definition.

Of course, then there is the issue of one-to-many relationships that don't work in mongo. We need to fix that, but not necessarily by changing the behavior of stringObjectIDCoercion: true.

@hacksparrow
Copy link
Contributor

I agree, the behavior is correct as per how stringObjectIDCoercion: true is supposed to work.

This doesn't look very intuitive, though:

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

I'd prefer:

export class User extends Entity {
  @property({
    type: ObjectID,
    id: true,
  })
  id?: ObjectID;
  // ...
}

Our APIs should be driven by intuitive DX.

@hacksparrow
Copy link
Contributor

@devoNOTbevo thanks for the input. This will help us come up with a generic solution instead of just focussing on the MongoDB case.

@devoNOTbevo
Copy link

devoNOTbevo commented May 8, 2019

@hacksparrow I realized there was a bug in my code for the accessor function and so deleting comment. Disregard. I was passing the wrong type in the first place.

@bajtos
Copy link
Member

bajtos commented May 9, 2019

This doesn't look very intuitive, though.
mongodb: {dataType: 'ObjectID'}
I'd prefer:
type: ObjectID

I agree that would be a much better developer experience.

However, it's also much more difficult to implement, because ObjectID is a MongoDB specific type. We don't want @loopback/repository and @loopback/repository-json-schema to depend on connector-specific code. To be able to support ObjectID, we need a mechanism allowing connectors to contribute additional types.

Consider the case where Category has many Product instances, the Category model is stored in MongoDB and Product model is stored in a SQL database. We need a mechanism to convert Category's id property from MongoDB-specific type ObjectID to a type that SQL understand and can use for Product's categoryId property/column. This is certainly doable, but requires even more design & implementation effort.

I am proposing to implement the simpler solution based on mongodb.dataType first, because it's easiest to implement, and then look into ways how to support ObjectID as a first-class property type.

@dhmlau
Copy link
Member

dhmlau commented May 13, 2019

@davidpestana
Copy link

Obviously this problem extends to all functions of the crud repository that require searching by id.

as:

updateById, update, and even find ({where: {id: instance.id}})

In the project I created a workarround creating a second ID in the model to perform the post-creation actions and in the repository I created the methods for it.

import * as uuid from 'uuid';

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

  @property({
    type: 'string',
    default: () => uuid()
  })
  key: string;
}

Repository.

export class SchedulerOrderRepository extends DefaultCrudRepository<
  SchedulerOrder,
  typeof SchedulerOrder.prototype.id
> {
  constructor(
    @inject('datasources.mongodb') dataSource: MongodbDataSource,

  ) {
    super(SchedulerOrder, dataSource);
  }

  findByKey(key:string):Promise<SchedulerOrder|null>{
    return this.findOne({where:{key:key}})
  }
}

I also thought about writing findById doing the ROW query to mongoDB but it seemed too great an effort if this issue is resolved soon.

Thank you

@dhmlau
Copy link
Member

dhmlau commented Jun 19, 2019

@hacksparrow, per our discussion, I believe this is done, and what we need is to release a major version of loopback-connector-mongodb?

@hacksparrow
Copy link
Contributor

hacksparrow commented Jun 20, 2019

Yes, loopback-connector-mongodb 5.x fixes this.

Also, we will now be able to configure a property as ObjectID using {type: String, mongodb: {dataType: 'objectID'}. objectID is case-insensitive.

Eg:

{
  xid: {type: String, mongodb: {dataType: 'ObjectID'}}
}

@dhmlau
Copy link
Member

dhmlau commented Jun 21, 2019

Closing this as done because the original scope is already completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug db:MongoDB Topics specific to MongoDB major Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package
Projects
None yet
Development

No branches or pull requests

10 participants