Loopback-next: Mongo fails to find user by id (hasMany relation not working)

[codejamninja]

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 https://github.com/strongloop/loopback-next/issues/1875

[codejamninja]

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]

@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)?

[codejamninja]

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

[b-admike]

@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]

Also related https://github.com/strongloop/loopback-next/issues/1987. @loredanacirstea @jormar, my proposed solution at https://github.com/strongloop/loopback-next/issues/1987#issuecomment-440923089 is not comprehensive (see https://github.com/strongloop/loopback-next/issues/2085#issuecomment-443029894 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]

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]

Hi. Any update on this? Please fix this issue at the very earliest. It is a major bug and has been a part of previous milestones.

[bajtos]

Please fix this issue at the very earliest. It is a major bug and has been a part of previous milestones.

@theiosdevguy Please check your tone. This is an open-source project we are giving away for free, you are in no way entitled to receive any bug fixes from us. Unless you are paying IBM for support, in which case please report this issue via the usual support channels.

Your second best option is to stop being a freeloader and contribute the fix yourself. We are happy to help you along the way, see https://loopback.io/doc/en/contrib/index.html to get started. Because hey, if this issue is not important enough for you to let you find time to fix it, then why should anybody else invest their valuable time?

If neither of those options is viable to you, then please press the thumbs-up 👍 button at the top (just below the issue description area) to upvote the issue; and then patiently wait until somebody finds time and contributes the fix.

[dhmlau]

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

[hacksparrow]

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]

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]

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

[hacksparrow]

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]

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. https://github.com/strongloop/loopback-connector-mongodb/pull/501, https://github.com/strongloop/loopback-connector-mongodb/pull/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]

In https://github.com/strongloop/loopback-next/issues/1875#issuecomment-490074828, I am proposing to fix MongoDB.prototype.isObjectIDProperty to recognize MongoDB-specific dataType setting in model property definition.

[bajtos]

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]

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]

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

[devoNOTbevo]

@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]

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]

PR https://github.com/strongloop/loopback-connector-mongodb/pull/517

[davidpestana]

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]

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

[hacksparrow]

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]

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