Add model def objectid support

[superkhau]

Short term fix for ObjectId support.

Connect to strongloop-internal/scrum-loopback#625
See also https://github.com/strongloop-internal/scrum-loopback/issues/566

[superkhau]

@ritch Review please.

[superkhau]

@slnode test please

[superkhau]

@bajtos Review please.

[ritch]

I don't think we should depend on mongodb directly in the juggler. We should contribute ObjectID some other way. It should work without the mongodb connector (eg. in the browser).

[ritch]

Another option is to optionally depend on mongodb (or similar, eg. an ObjectID module). Sort of like this:

try {
  mongodb = require('mongodb');
} catch(e) {
  if (!isNotFound(err)) { 
    throw e;
  }
}

[superkhau]

My initial impl had our own ObjectId definition that delegated to mongo's object id, which we can move towards in the long term update. Will add it to the notes there.

For the short term I will use the try/catch you suggested.

[superkhau]

@ritch LGTY?

[bajtos]

To be honest, I don't like adding mongodb dependency to juggler at all. Remember, there is no guarantee that mongodb module referenced by juggler will be the same module as referenced by loopback-connector-mongodb. In fact, this will be the most frequent case.

app
  node_modules
    loopback-datasource-juggler
      node_modules
        mongodb
    loopback-connector-mongodb
      node_modules
        mongodb

As a result, you are registering a type (constructor function) that is actually never used by the connector, because the connector has it's own local type (constructor function).

[superkhau]

TBH, I don't like it either, but it was the simplest way to get the full power of an ObjectId (all it's validations, unique string generation, other functions etc) in one shot. The other option is to just not use mongodb at all and come up with our own unique implementation for each function. However, this is not a 5 point job. I figure in the long term proposal task I have for this sprint, we can decide whether we want to come up with our own implementations for each function or use source code from other projects, etc.

[superkhau]

It would have an API similar to https://docs.mongodb.org/manual/reference/object-id/ basically.

[superkhau]

Another thing to consider is the option to allow user-configurable ObjectId types. Like in model-config.json, we allow {"objectid": "../path/to/your/custom/objectid"}, or something along those lines.

[bajtos]

Makes me wonder if there is a way how to allow loopback-connector-mongodb to call modelTypes.registerType(ObjectId) during connector/datasource initialisation. That way we can keep the implementation in mongodb and still have ObjectID available in the app.

Of course, that would not work for isomorphic app in the browser, so it would be still a short-term fix.

Anyways, I guess you spend more time thinking about this issue than I did, and if this is the best short-term solution we have, then I can live with it.

[superkhau]

Makes me wonder if there is a way how to allow loopback-connector-mongodb to call modelTypes.registerType(ObjectId) during connector/datasource initialisation. That way we can keep the implementation in mongodb and still have ObjectID available in the app.

I have thought about this too, but figured it would be harder to implement because we don't expose that part of the lib in DAO. The long term solution I had in mind and discussed with @ritch is an app-level registry for datatypes. During boot time, we would register all basic built in datatypes, then register any custom types. Then at run time, we can dynamically register/deregister new datatypes. Something along the lines of:

app.datatypes.add(ObjectId);
app.datatypes.list();
app.datatypes.remove(ObjectId);

Or use an a K/V array for datatypes, etc. We can discuss further in the long-term proposal with regards to implementations.

[bajtos]

@superkhau please make sure the redis build failure is not related to your change

[superkhau]

@bajtos Is there a way to target redis specifically? All my tests are green locally.

[bajtos]

Is there a way to target redis specifically? All my tests are green locally.

Sorry for the confusion, I was referring to dependant loopback-connector-redis. There are more test failures reported now. They may be unrelated to this patch, just make sure to check & verify it's true.

[superkhau]

The redis tests are failing on its own.

[superkhau]

@bajtos Reassigning to you for review as @ritch is probably too busy to review this. Please let me know how to move forward with this.

[bajtos]

Missing semicolon at EOL, please add.

[bajtos]

@superkhau please add few tests to the test-suite run by all connectors (see test/common.batch.js) to ensure this new ObjectID type is correctly persisted and loaded when using e.g. relational (SQL) databases.

[bajtos]

Is there any way how to register ObjectID as an alias for ObjectId? Or is it automatically handled?

[superkhau]

It is already registered as an alias in the ObjectId class definition, so it should work properly. I added an e2e test to prove it.

[superkhau]

@bajtos Updated with the comments from your review. Can you PTAL again?

[bajtos]

@superkhau AFAICT, this silently swallows any mongodb parse errors, which does not look like a good thing to me. Could you please explain what is the rationale for this, why we need to give require('mongodb').ObjectId errors a special treatment?

[superkhau]

This was suggested by @ritch in a previous comment in this issues history. I think it was for uses cases where mongodb is not available (ie. the browser).

[bajtos]

Fair enough. Please add a debug log to let the user know ObjectId will be undefined because mongodb was not found.

debug('Cannot load mongodb, ObjectId will be undefined. %s', e.stack);

[bajtos]

@superkhau I am missing unit-tests to be run by connectors, per my earlier comment:

please add few tests to the test-suite run by all connectors (see test/common.batch.js) to ensure this new ObjectID type is correctly persisted and loaded when using e.g. relational (SQL) databases.

[superkhau]

@slnode test please

[superkhau]

@bajtos Added data type tests here.

[superkhau]

@bajtos Added data type tests here: https://github.com/strongloop/loopback-datasource-juggler/pull/778/files#r47993341

[bajtos]

One more comment: #778 (comment)

@superkhau after you address ^^^, then please rebase the patch on top of the current master, clean up the commit history and merge this without waiting for me. IIUC this is a backwards-compatible change targeted for LoopBack LTS, in which case you need to back-port it to 2.x too.

[bajtos]

2.x refers to this: https://github.com/strongloop/loopback-datasource-juggler/tree/2.x

[mrbatista]

@superkhau when will this patch be merged?

Thanks.

[superkhau]

@mrbatista Soon I believe, I need to write a few more tests to confirm the data persistence with relational connectors, but after that it's all good.

[bajtos]

@superkhau I am not sure what's the status of this work. If we decide to eventually land this, then I would like you to try an alternative approach: get the implementation of ObjectID directly from bson/lib/bson/objectid.js. I believe the mongodb driver we use does basically the same, so it should be safe to bypass mongodb and load the definition directly. If it works, then this solution will reduce the footprint of juggler in applications that don't use MongodB.

[superkhau]

@bajtos This has been stale for a long time and is probably not a good solution now. Let's do what you propose (as a new feature/PR instead).

[bajtos]

ok, I'm closing this PR then.