Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

forbid overriding builtin services #1347

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
121 changes: 121 additions & 0 deletions lib/rules/no-restricted-service-registrations.js
@@ -0,0 +1,121 @@
'use strict';

const assert = require('assert');
const emberUtils = require('../utils/ember');
const {getImportIdentifier} = require('../utils/import');

const DEFAULT_ERROR_MESSAGE = 'Registering this service is not allowed';
const DEFAULT_RESTRICTED_REGISTRATIONS = ['router'];

/**
* @type {import('eslint').Rule.RuleModule}
*/
module.exports = {
meta: {
type: 'suggestion',
docs: {
description: 'disallow registering certain services under certain paths',
category: 'Services',
url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-restricted-service-registrations.md',
},
schema: {
type: 'array',
uniqueItems: true,
minItems: 1,
items: [
{
type: 'object',
required: ['services'],
properties: {
services: {
type: 'array',
uniqueItems: true,
minItems: 1,
items: {
type: 'string',
},
},
message: {
type: 'string',
},
},
additionalProperties: false,
},
],
},
},

DEFAULT_ERROR_MESSAGE,

create(context) {
if (!emberUtils.isTestFile(context.getFilename())) {
return {};
}

// Validate options.
for (const option of context.options) {
for (const service of option.services) {
assert(
service.toLowerCase() === service,
'Service name should be passed in kebab-case (all lower case)'
);
}
}

function checkForViolationAndReport(node, serviceName) {
const serviceNameKebabCase = emberUtils.convertServiceNameToKebabCase(serviceName); // splitting is used to avoid converting folder/ to folder-

for (const denylist of denylists) {
// Denylist services are always passed in in kebab-case, so we can do a kebab-case comparison.
if (denylist.services.includes(serviceNameKebabCase)) {
context.report({
node,
message: denylist.message || DEFAULT_ERROR_MESSAGE,
});
}
}
}

let getContextName;
let potentials = [];

return {
ImportDeclaration(node) {
if (node.source.value === '@ember/test-helpers') {
getContextName = getContextName || getImportIdentifier(node, 'getContext');
}
},

MemberExpression(node) {
if (node.object.name === 'owner' && node.property.name === 'register') {
potentials.push(node);
}

},

CallExpression(node) {
potentials = [];
},

'CallExpression:exit'(node) {
for (let potential of potentials) {
let first = node.arguments[0].value;

let [namespace, name] = first.split(':');

if (namespace !== 'service') {
return;
}

if (name === 'router') {
context.report({
node,
message: `Cannot register the router service`,
});
}
}
}

};
},
};
59 changes: 59 additions & 0 deletions tests/lib/rules/no-restructed-service-registrations.js
@@ -0,0 +1,59 @@
const RuleTester = require('eslint').RuleTester;
const rule = require('../../../lib/rules/no-restricted-service-registrations');

const { DEFAULT_ERROR_MESSAGE } = rule;

const ruleTester = new RuleTester({
parser: require.resolve('@babel/eslint-parser'),
parserOptions: {
ecmaVersion: 2020,
sourceType: 'module',
},
});

ruleTester.run('no-restricted-service-registrations', rule, {
valid: [
{
// Service name doesn't match (with property name):
code: `owner.register('service:foo')`,
Copy link
Member

@bmish bmish Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is owner.register('service:foo') ever used outside tests? I thought it was mostly for tests but I could be wrong. If it's just for tests, do we care about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it technically could happen -- I imagine the behavior would mostly be undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the speed of certain rules is important, and only looking at test files is a perf improvement we can make.

Maybe it's rare enough where we don't need to worry about app/addon trees?
I'm going to check ember-observer.

In any case, I do still want to prevent this:

let owner = this.owner;

owner.register('...');

this addon will need to keep track of all the ways people can get and manipulate the owner reference

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit more precise: only 12 usages: https://emberobserver.com/code-search?codeQuery=owner%5C.register%5C((%27%7C%22)service&fileFilter=addon&regex=true&sort=usages&sortAscending=false
all seem to be addon-test-support, which is fine -- so we can scope to tests. woohoo!

now, looking at the what we actually want to prevent:
https://emberobserver.com/code-search?codeQuery=owner%5C.register%5C((%27%7C%22)service%3Arouter&fileFilter=tests&regex=true&sort=usages&sortAscending=false

only 21 usages -- less than what I've seen internally 🙃
if only I could check app code with ember observer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the router is the only one I've ran in to. I don't use ember-data regularly enough to know if the store would cause problems 🤷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. So this doesn't seem super useful to me personally but if you feel strongly that it's useful and likely to help people, feel free to proceed.

If you want to proceed, the path forward that I'm leaning towards is:

  1. Create no-router-service-registration that could eventually be enabled as recommended. Limited to just the router service since it's unclear if any other services have a problem with this. If you determine this is likely to apply to other built-in services, then it could be named no-built-in-service-registrations.
  2. If people actually wanted restrict arbitrary service registrations, someone could create an optional no-restricted-service-registrations rule at a later point. Still, I'm unclear that this is actually needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a library author, i think optionally preventing registration of similar services is important.

i lean towards "no-restricted-service-registrations" with router as default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i'd use that a bunch internally)

Copy link
Member

@bmish bmish Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i lean towards "no-restricted-service-registrations" with router as default

Okay, I'm open to considering that.

I am just a bit worried about that option as there are many other no-restricted-* rules that are typically intended as purely optional (not recommended) rules which don't restrict anything by default but can be configured to restrict things as desired:

So it may be confusing to people that this new no-restricted-service-registrations rule restricts router by default.

options: [{ services: ['abc'] }],
},
{
// Namespace doesn't match
code: `owner.register('model:abc')`,
options: [{ services: ['abc'] }],
},
],
invalid: [
// Basic test usage
{
code: `this.owner.register('service:router', null)`,
output: null,
errors: [{ message: `Cannot register the router service.` }],
},
// owner assigned to a variable
{
code: `let a = this.owner; a.register('service:router', null)`,
output: null,
errors: [{ message: `Cannot register the router service.` }],
},
// using getContext().owner
{
code: `import { getContext } from '@ember/test-helpers'; getContext().owner.register('service:router', null)`,
output: null,
errors: [{ message: `Cannot register the router service.` }],
},
// using getContext, but renamed
{
code: `import { getContext as a } from '@ember/test-helpers'; a().owner.register('service:router', null)`,
output: null,
errors: [{ message: `Cannot register the router service.` }],
},
// using { owner } = getContext()
{
code: `let { owner: a } = getContext(); a.register('service:router', null)`,
output: null,
errors: [{ message: `Cannot register the router service.` }],
},
],
});