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
codejamninja opened this issue Nov 27, 2018 · 25 comments

Comments

@codejamninja
Copy link
Member

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

@codejamninja

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

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

@codejamninja

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2018

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

@b-admike

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

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;
  }
  ......
}
@dhmlau dhmlau referenced this issue Jan 18, 2019
9 of 29 tasks complete
@dhmlau dhmlau referenced this issue Jan 30, 2019
14 of 37 tasks complete
@dhmlau dhmlau referenced this issue Mar 1, 2019
18 of 34 tasks complete
@dhmlau dhmlau referenced this issue Apr 1, 2019
20 of 46 tasks complete

@dhmlau dhmlau added p1 2019Q2 and removed TOB labels Apr 1, 2019

@theiosdevguy

This comment was marked as off-topic.

Copy link

commented Apr 17, 2019

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

This comment was marked as off-topic.

Copy link
Member

commented Apr 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

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

@dhmlau dhmlau added this to the May 2019 milestone milestone Apr 29, 2019

@dhmlau dhmlau referenced this issue Apr 30, 2019
27 of 32 tasks complete
@hacksparrow

This comment has been minimized.

Copy link
Member

commented May 6, 2019

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented May 6, 2019

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

@hacksparrow

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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. strongloop/loopback-connector-mongodb#501, strongloop/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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented May 7, 2019

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

This comment has been minimized.

Copy link
Member

commented May 8, 2019

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

@devoNOTbevo

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@davidpestana

This comment has been minimized.

Copy link

commented May 15, 2019

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

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

@dhmlau dhmlau closed this Jun 21, 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.