Fix datasource not correctly retaining name value

[nitro404]

Description

Data source names are not correctly retained and assigned to the object instance in some cases.

Hopefully my changes are correct, I'm not super familiar with the code base so it would be helpful to have a second opinion. I can add extra test cases if needed, not sure what might be required, if anything.

Related issues

• connect to Data source names are not correctly retained in some cases #1547
• see also Add warning for datasources with mismatched names #1542

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style
  guide

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[nitro404]

@bajtos Tagging you here just in case you have any input on this matter, since this related to my prior pull request on Loopback.

[kjdelisle]

Looks good, if you can add some unit tests to validate those logic branches, then I'll approve and merge. :)

[nitro404]

Amended my commit with some new unit tests for data source names for a few different cases and added some extra checks to ensure that data sources are correctly assigned their name. I had forgotten to check the settings object for a name as well, so this is also no taken into account and tested. Please let me know if any additional tests or changes are required. Thanks!

[bajtos]

I am not very familiar with this part of our codebase either. I don't see any obvious problems, and since there are new tests added to verify the changes, I am happy to approve this pull request.

@kjdelisle could you please take care of landing it?

[bmatson]

@bajtos @kjdelisle @nitro404 I accidentally updated my fork to the head yesterday and started getting your new warning about datasource names. My code still worked, however your warning was spit out. After digging into the code in datasource.js, it appears that everyone who is using a "normal" datasource setup (at least what's documented in the LB 3 docs) is going to get your error. For a datasource like this:
{"db": {
"name": "db",
"connector": "mongodb",
"url": "mongodb://localhost/engdb"
}
}
When run against the latest release of loopback (which has some other fixes related to passing names through). The name "db" and the settings are correctly passed to the setup function, however the if statement at line 348 sets connector = "mongodb", then the code at line 371 overrides the passed in name with the connector value, which triggers the warning you just added.

Here's the offending code:

if (typeof connector === 'string') {
    name = connector;
    connector = undefined;
  }
  name = name || (connector && connector.name);
  this.name = name;

The name variable then gets used a bunch after this, so I'm not sure that just deleting the if statement is the right fix, it looks to me like the follow-on code mixes the connector type and the datasource name in some really unclear ways.

[nitro404]

Hi @bmatson, thanks for your comment. I too had noticed this when working with the latest changes to Loopback as well and set up this pull request as a fix for this issue. There was a use case I had not accounted for in my sandbox testing environment where the connector name was still being applied as the name of the actual data source. Test cases that were previously absent have also been added to this module to account for this as well. Try applying the changes from this pull request to your datasource juggler to see if it resolves the warning. Thanks!

[bmatson]

@nitro404 The issue I'm pointing out does happen with this PR, and probably needs to be addressed in its scope before the next "official" release of datasource-juggler (since the initial version of the PR has been landed on master).

[nitro404]

Oh interesting, I thought you meant you were getting this with the latest version of Loopback. Fair enough, I'll have to look into it. Sorry for the misunderstanding.

[nitro404]

@bmatson I'm not able to replicate your issue with the changes in this pull request.

@kjdelisle Any chance we could get this pull request landed and versioned soon? There's currently warnings being output on the published module version that this pull request fixes.