fix: warn about strict cannot be false with relational dbs

[agnes512]

Fixes loopbackio/loopback-next#4042

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• Commit messages are following our guidelines

[bajtos]

Looks mostly good so far.

Is it possible to write a test to verify the behavior by spying on console.warn methods? I quickly checked juggler and LB3 codebase and could not find any test to show as an example. I also did a quick search on the internet and it looks like intercepting console methods is not trivial because Mocha may be using them too. I guess it's ok to make these changes without test coverage.

I don't know how we warn users in Juggler.

Using console.warn is fine.

Can you please verify that your fix is covering all different ways we have for defining a model? Most importantly, is the warning triggered when models are created before they are attached to a datasource? That's what we are doing in LoopBack 3 applications.

Here is the code creating a new model class via Model.extend:
https://github.com/strongloop/loopback/blob/6a7fc529fcbf219d3a2f6b77cdf221ee5c3b1dbf/lib/registry.js#L98-L128

And here we are attaching a model to a datasource - I think that's the point where your new warning should be printed. the code path that should trigger your new warning.
https://github.com/strongloop/loopback/blob/6a7fc529fcbf219d3a2f6b77cdf221ee5c3b1dbf/lib/registry.js#L235

[agnes512]

The reason why I don't have a test for the changes is because I am thinking to add them to lb4 tests since we can easily test them on real DBs. Thoughts?

Can you please verify that your fix is covering all different ways we have for defining a model? Most importantly, is the warning triggered when models are created before they are attached to a datasource? That's what we are doing in LoopBack 3 applications.

Good point, I didn't consider is before. I just chased through and found where we check isRelational.

I did some debug. I didn't create a LB3 app and check it with relational connectors( I don't know how 🙈). I used LB4 code and MySQL as dataSource (I think it's fine since both of them use connectors and Juggler?).

It shows in LB4 that it creates models with DataSource.prototype.createModel,strict i checked in this step, then attach to DataSource with DataSource.prototype.attach. If LB3 follows the same path, does that mean the change matches our expectation?

[nabdelgadir]

Use double quotes to avoid escaping: "WARNING: relational database doesn't support {strict: false} mode."

[bajtos]

I did some debug. I didn't create a LB3 app and check it with relational connectors( I don't know how 🙈). I used LB4 code and MySQL as dataSource (I think it's fine since both of them use connectors and Juggler?).

It shows in LB4 that it creates models with DataSource.prototype.createModel,strict i checked in this step, then attach to DataSource with DataSource.prototype.attach. If LB3 follows the same path, does that mean the change matches our expectation?

AFAIK, LB3 uses a different code path for creating models. Creating a LB3 app is easy, see https://loopback.io/lb3/getting-started. You can also use our Coffee Shop example https://github.com/strongloop/loopback-getting-started.

As for adding a relational connector, just install the connector and change the datasource configuration accordingly. In LB3, datasources are configured in server/datasources.json file, see https://github.com/strongloop/loopback-getting-started/blob/master/server/datasources.json

AFAICT, the Coffee Shop example is already using MySQL to store the data! I think it should be enough to git clone the app, add strict: false model setting to common/models/coffee-shop.json and verify that your warning is printed.

On the other hand, considering that LB3 is in LTS, maybe it's enough to implement the warning for LB4 only?

[agnes512]

AFAIK, LB3 uses a different code path for creating models.

You're right. I ran the coffee shop example with MySQL. The change only affects LB4. I think LB3 uses model-builder.js instead.

On the other hand, considering that LB3 is in LTS, maybe it's enough to implement the warning for LB4 only?

I think it's fair to just have the warning and tests for LB4. I will add them once the change gets released.

[agnes512]

@slnode test please

[agnes512]

@slnode test please