Promise-ify built-in Role model

[bajtos]

Description

Add Promise support to Role methods.

Related issues

• Support promises #418

Checklist

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

@superkhau please review
cc @pierreclr @ebarault

[ebarault]

LGTM,

As for isInRole

• cleaner than my own implementation, while not requiring a full refactor using a full promise flow 👍
• if not further objection i will merge beforehand with my branch to ease further rebasing on my side
• isn't this a good moment to fix the inconsistent return type of this function? (should return always a boolean but return sometime a role instance)

is there a similar PR for methods from ACL ? (resolvePrincipal, isMappedToRole)

[bajtos]

@ebarault thank you for chiming in!

isn't this a good moment to fix the inconsistent return type of this function? (should return always a boolean but return sometime a role instance)

Good point! I am reluctant to change the return type in the callback variant, I am concerned there may be applications/code relying on this incorrect behaviour.

Let's fix it for the Promise variant only, I'll rework the commit shortly.

is there a similar PR for methods from ACL ? (resolvePrincipal, isMappedToRole)

I don't think so. Would you mind submitting one yourself?

[ebarault]

fix the promise variant only : 👍
pr for promisifying ACLs methods : OK, with a plan to land all this early next week (incl. #2971)

[bajtos]

Note: I am intentionally not back-porting this patch to 2.x, because I consider adding Promise support as a new feature.

[ebarault]

please see #3163 for the similar PR for built-in ACL model

[ebarault]

@bajtos : I should have asked earlier: provided that bringing promise support to built-in model Role, would you consider enabling by default getRoles() returning only role names for the promise version?
(cf. here)

[bajtos]

I should have asked earlier: provided that bringing promise support to built-in model Role, would you consider enabling by default getRoles() returning only role names for the promise version?

Are your proposing to change the default value of options.returnOnlyRoleNames (see #2975) depending on whether the method is invoked in callback or Promise mode?

I am concerned that this may confuse users. So far, our APIs behave identically when invoked with a callback or when expecting a Promise to be returned. (With the exception of Role.isInRole, where Promise is always resolved with a boolean instead of a truthy/falsy value passed to the callback).

I feel the option returnOnlyRoleNames is a more significant change and the inconsistence would be too big.

@superkhau @raymondfeng @ritch thoughts?

A lesser concern is about performance - to return role names, we have to query Role model in addition to RoleMapping, see common/models/role.js#L478. Because our connectors don't support SQL JOIN, this means an extra database request to make.

[superkhau]

I'm +1 for consistency and less confusion with the current APIs. We should not diverge based on the promise variant IMO.

[ebarault]

having getRoles() return an array of both role names and role ids is inconsistent to me in the first place (i can recall a handful of issues about this too).

I get the performance issue: but it's by far not the bottleneck of role-resolving in loopback as of now (see [this discussion](5 hours ago) superkhau participated in)
Note although that the current access control behavior by parsing all the role resolvers enables to imagine things like this)

Anyhow, as this seemed a good moment with this PR just worked-out, i just wanted to bring the subject to your attention. I can totally live with the returnOnlyRoleNames option

[bajtos]

having getRoles() return an array of both role names and role ids is inconsistent to me in the first place (i can recall a handful of issues about this too).

I agree this is inconsistent and confusing :(

Because the time has not yet come for a new semver-major release of loopback, where we would be able to make this breaking change, I am proposing the following improvements:

• Enhance API documentation for getRoles() (source) to clearly describe the options object (the flags) and explicitly recommend users to set returnOnlyRoleNames when calling this method.
• Add a new method getRoleNames (or getAllRoleNames?) that will call getRoles under the hood and always set returnOnlyRoleNames to true.
• Deprecate the invocation of getRoles() without returnOnlyRoleNames. This should nudge users to upgrade their code to either call the new method, or explicitly set returnOnlyRoleNames to false. This deprecation should be clearly described in API docs too.

Thoughts? (cc @raymondfeng @superkhau)

@ebarault would you be interested in contributing these enhancements?

[superkhau]

I'm good with any of those solutions, but if I had to choose one, it would be the first solution as it sounds the simplest to implement.

[ebarault]

@bajtos: yes i can contribute the changes. were you proposing to select one of these options or all of them combined? I'm good with proposal 1 and 3. Introducing a new function would also bring confusion imo. The definite change could be endorsed on next lb major release.

[bajtos]

@ebarault excellent! The three options are sort of independent, feel free to implement only those that make most sense to you.