feat: allows different naming conventions for db columns for discover #1819

agnes512 · 2020-01-27T19:34:26Z

Connects to loopbackio/loopback-next#3343

Allows users to keep the property name as the same as the db columns names. It can be set by options.useDefaultCase to false.

To close #3343, we need to:

modify Juggler (this PR)
update LB3 docs ( docs: add explanations of naming convention of discover loopback.io#928)
update lb4 discover CLI and docs ( need this PR to be landed first)

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

lib/datasource.js

nabdelgadir

Nice work!

lib/datasource.js

jannyHou · 2020-01-28T20:23:09Z

lib/datasource.js

@@ -1586,11 +1594,11 @@ DataSource.prototype.discoverSchemas = function(tableName, options, cb) {
    // Default name mapper
    nameMapper = function mapName(type, name) {
      if (type === 'table' || type === 'model') {
-        return fromDBName(name, false);
+        return pascalCase(name);


@agnes512 A question about the behaviour change:

Before we allowed camel case for table name, and IIUC this PR converts all the table names to be pascal case, there is no option to config it to be other naming conventions including camel case.
Is it intended?

I don't think we allowed table/model names to be camelCase before because every time we checked the type:

if (type === 'table' || type === 'model') { return fromDBName(name, false); // this is saying not to use camelCase, use pascal instead

the false in the second param was set, no options were allowed to configure it.

And since it was converting all table/model names to pascalCase, I replace it with a formal function pascalCase() to make it more legit.

Before the function fromDBName handled cases change for type table, model, fk, and column. ~~I change it to just handle column names only.~~ I change it to just handle column names and fk only
( I realized that I made a mistake here, I should let it handle fk as well as it's part of property names).

Another reason I think table and model names should just be pascal case is because lots of our naming conventions are based on the model name. We assume model names will be Pascal case, and file names that are generated based on the model names will be snake case, etc. I think allowing customized table/model names might cause some unexpected issues.

jannyHou · 2020-01-28T20:23:52Z

lib/datasource.js


  const nameMapper = options.nameMapper || function mapName(type, name) {
    if (type === 'table' || type === 'model') {
-      return fromDBName(name, false);
+      return pascalCase(name);


same question as https://github.com/strongloop/loopback-datasource-juggler/pull/1819/files#r372036050 :)

jannyHou

@agnes512 Good to see we are making progress on cleaning up the naming conventions 👍 I left a question regarding the behaviour change, otherwise LGTM.

raymondfeng · 2020-01-29T16:22:36Z

lib/datasource.js

+ * @param {*} defcamelCase boolean. Converts the name to camelcase if true as
+ * it's the default case for variables / properties in LoopBack.
+ */
+function fromDBName(colName, defCamelCase) {


It's probably better to make 2nd argument an optional function (default to camelCase) for case conversion. Using a boolean argument is not very friendly for JS.

fromDBName(colName) or fromDBName(colName, camelCase) // convert to camel case

fromDBName(colName, noCase) // no conversion

fromDBName(colName, pascalCase) // convert to pascal case

raymondfeng · 2020-01-29T16:24:17Z

lib/datasource.js

@@ -1574,6 +1580,8 @@ DataSource.prototype.discoverSchemas = function(tableName, options, cb) {
  const dbType = this.connector.name;

  let nameMapper;
+  const defCamelCase = options.useDefaultCase === undefined ? true : options.useDefaultCase;


Maybe a better way is options.disableCamelCase (default to false).

jannyHou · 2020-01-29T17:19:43Z

lib/datasource.js

      } else if (type == 'fk') {
-        return fromDBName(name + 'Rel', true);
+        if (disableCamelCase === true) {


any reason we cannot use if(disableCamelCase)?

👍 I thought I need to check the type as well turns out I didn't need to, changed.

lib/datasource.js

jannyHou

👍
Just have a question for using if(disableCamelCase === true), otherwise LGTM.

jannyHou · 2020-01-29T17:25:53Z

And please make sure the CI passes. Oracle seems random, postgresql seems related...😞

lib/datasource.js

raymondfeng · 2020-01-29T18:09:34Z

lib/datasource.js

  cb = cb || utils.createPromiseCallback();

  const self = this;
  const dbType = this.connector.name;

  let nameMapper;
+  const disableCamelCase = options.disableCamelCase === undefined ? false : options.disableCamelCase;


const disableCamelCase = !!options.disableCamelCase;

agnes512 · 2020-01-29T18:50:58Z

The failure of Postgres is because of this line,

 db.discoverAndBuildModels('GeoPoint', {schema: 'strongloop'}, function(err, schema) {
      schema.Geopoint.find(function(err, data) {

With the change, the schema name is supposed to be GeoPoint instead of Geopoint. Can we land this PR first and I will fix it in Postgres connector?

agnes512 requested review from dhmlau, emonddr, jannyHou and zbarbuto as code owners January 27, 2020 19:34

agnes512 force-pushed the discover-naming branch from b1b43ee to 36e9c17 Compare January 27, 2020 19:46

agnes512 mentioned this pull request Jan 27, 2020

docs: add explanations of naming convention of discover loopbackio/loopback.io#928

Merged

agnes512 force-pushed the discover-naming branch from 36e9c17 to 2edd077 Compare January 27, 2020 20:13

emonddr suggested changes Jan 28, 2020

View reviewed changes

lib/datasource.js Outdated Show resolved Hide resolved

lib/datasource.js Outdated Show resolved Hide resolved

nabdelgadir approved these changes Jan 28, 2020

View reviewed changes

lib/datasource.js Outdated Show resolved Hide resolved

lib/datasource.js Outdated Show resolved Hide resolved

lib/datasource.js Outdated Show resolved Hide resolved

agnes512 force-pushed the discover-naming branch 2 times, most recently from 8aeec59 to 60f1726 Compare January 28, 2020 19:14

jannyHou reviewed Jan 28, 2020

View reviewed changes

emonddr approved these changes Jan 29, 2020

View reviewed changes

raymondfeng reviewed Jan 29, 2020

View reviewed changes

jannyHou reviewed Jan 29, 2020

View reviewed changes

lib/datasource.js Outdated Show resolved Hide resolved

jannyHou approved these changes Jan 29, 2020

View reviewed changes

agnes512 force-pushed the discover-naming branch from f86c420 to d13a893 Compare January 29, 2020 17:27

raymondfeng reviewed Jan 29, 2020

View reviewed changes

lib/datasource.js Outdated Show resolved Hide resolved

raymondfeng reviewed Jan 29, 2020

View reviewed changes

allows diff db cols naming conventions 4 discover

baeb76e

agnes512 force-pushed the discover-naming branch from d13a893 to baeb76e Compare January 29, 2020 18:26

raymondfeng approved these changes Jan 29, 2020

View reviewed changes

agnes512 merged commit f4ec02b into master Jan 29, 2020

delete-merged-branch bot deleted the discover-naming branch January 29, 2020 18:57

agnes512 mentioned this pull request Jan 30, 2020

fix discovered schema name with pascal case loopbackio/loopback-connector-postgresql#410

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allows different naming conventions for db columns for discover #1819

feat: allows different naming conventions for db columns for discover #1819

agnes512 commented Jan 27, 2020 •

edited

Loading

nabdelgadir left a comment

jannyHou Jan 28, 2020

agnes512 Jan 28, 2020 •

edited

Loading

jannyHou Jan 28, 2020

jannyHou left a comment

raymondfeng Jan 29, 2020

raymondfeng Jan 29, 2020

jannyHou Jan 29, 2020

agnes512 Jan 29, 2020

jannyHou left a comment

jannyHou commented Jan 29, 2020

raymondfeng Jan 29, 2020

agnes512 commented Jan 29, 2020

feat: allows different naming conventions for db columns for discover #1819

feat: allows different naming conventions for db columns for discover #1819

Conversation

agnes512 commented Jan 27, 2020 • edited Loading

Checklist

nabdelgadir left a comment

Choose a reason for hiding this comment

jannyHou Jan 28, 2020

Choose a reason for hiding this comment

agnes512 Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

jannyHou Jan 28, 2020

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

raymondfeng Jan 29, 2020

Choose a reason for hiding this comment

raymondfeng Jan 29, 2020

Choose a reason for hiding this comment

jannyHou Jan 29, 2020

Choose a reason for hiding this comment

agnes512 Jan 29, 2020

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

jannyHou commented Jan 29, 2020

raymondfeng Jan 29, 2020

Choose a reason for hiding this comment

agnes512 commented Jan 29, 2020

agnes512 commented Jan 27, 2020 •

edited

Loading

agnes512 Jan 28, 2020 •

edited

Loading