From 1d4eafcd3b6fd2e82c34712ac7bc809be745ae59 Mon Sep 17 00:00:00 2001 From: Neil Fraser Date: Tue, 26 Mar 2024 09:14:48 +0100 Subject: [PATCH] Make registry case-sticky Adding to a registry preserves case of the key. Adding a different case of the same key is an error. Thus any case is ok, as long as one is consistent. --- core/registry.ts | 70 ++++++++++++++++++++------ core/serialization/workspaces.ts | 4 +- core/toolbox/collapsible_category.ts | 7 +-- core/toolbox/separator.ts | 1 + core/toolbox/toolbox.ts | 7 +-- tests/mocha/field_registry_test.js | 8 ++- tests/mocha/registry_test.js | 74 ++++++++++++++++------------ 7 files changed, 109 insertions(+), 62 deletions(-) diff --git a/core/registry.ts b/core/registry.ts index 2106403f0..8d0523c74 100644 --- a/core/registry.ts +++ b/core/registry.ts @@ -24,7 +24,6 @@ import type {ToolboxItem} from './toolbox/toolbox_item.js'; import type {IPaster} from './interfaces/i_paster.js'; import type {ICopyData, ICopyable} from './interfaces/i_copyable.js'; import type {IConnectionPreviewer} from './interfaces/i_connection_previewer.js'; -import * as deprecation from './utils/deprecation.js'; /** * A map of maps. With the keys being the type and name of the class we are @@ -38,6 +37,15 @@ const typeMap: { } = Object.create(null); export const TEST_ONLY = {typeMap}; +/** + * A map of maps. Record the case-insensitive version of each name. + * e.g. {'field': {'field_angle': 'fIeLd_AnGle'}} + * This enables an error to be thrown if a name of conflicting case is used. + */ +const noCaseNameMap: { + [key: string]: {[key: string]: string} +} = Object.create(null); + /** * The string used to register the default class for a type of plugin. */ @@ -123,7 +131,7 @@ export function register( | AnyDuringMigration, opt_allowOverrides?: boolean, ): void { - type = normalizeName(`${type}`); + type = normalizeType(type); name = normalizeName(name); if (!registryItem) { throw Error('Can not register a null value'); @@ -132,10 +140,12 @@ export function register( // If the type registry has not been created, create it. if (!typeRegistry) { typeRegistry = typeMap[type] = Object.create(null); + noCaseNameMap[type] = Object.create(null); } // Validate that the given class has all the required properties. - validate(type, registryItem); + validateItem(type, registryItem); + validateCase(type, name); // Don't throw an error if opt_allowOverrides is true. // Don't throw an error if the registryItem is not changing. @@ -147,6 +157,7 @@ export function register( throw Error(`Name "${name}" with type "${type}" already registered`); } typeRegistry[name] = registryItem; + noCaseNameMap[type][name.toLowerCase()] = name; } /** @@ -157,16 +168,29 @@ export function register( * @param registryItem A class or object that we are checking for the required * properties. */ -function validate(type: string, registryItem: Function | AnyDuringMigration) { +function validateItem(type: string, registryItem: Function | AnyDuringMigration) { switch (type) { case String(Type.FIELD): if (typeof registryItem.fromJson !== 'function') { - throw Error('Type "' + type + '" must have a fromJson function'); + throw Error(`Type "${type}" must have a fromJson function`); } break; } } +/** + * Checks that the name hasn't been registered using a different case. + * + * @param type The type of the plugin. (e.g. Field, Renderer) + * @param name The plugin's name. (Ex. field_angle, geras) + */ +function validateCase(type: string, name: string) { + const caseName = noCaseNameMap[type]?.[name.toLowerCase()]; + if (caseName && caseName !== name) { + throw Error(`Inconsistent case with name "${name}" vs "${caseName}"`); + } +} + /** * Unregisters the registry item with the given type and name. * @@ -175,14 +199,16 @@ function validate(type: string, registryItem: Function | AnyDuringMigration) { * @param name The plugin's name. (Ex. field_angle, geras) */ export function unregister(type: string | Type, name: string) { - type = normalizeName(`${type}`); + type = normalizeType(type); name = normalizeName(name); + validateCase(type, name); const typeRegistry = typeMap[type]; if (!typeRegistry || !typeRegistry[name]) { console.warn(`Unable to unregister [${name}][${type}] from the registry`); return; } - delete typeMap[type][name]; + delete typeRegistry[name]; + delete noCaseNameMap[type][name.toLowerCase()]; } /** @@ -202,8 +228,9 @@ function getItem( name: string, opt_throwIfMissing?: boolean, ): (new (...p1: AnyDuringMigration[]) => T) | null | AnyDuringMigration { - type = normalizeName(`${type}`); + type = normalizeType(type); name = normalizeName(name); + validateCase(type, name); const typeRegistry = typeMap[type]; if (!typeRegistry || !typeRegistry[name]) { const msg = `Unable to find [${name}][${type}] in the registry.`; @@ -228,8 +255,9 @@ function getItem( * otherwise. */ export function hasItem(type: string | Type, name: string): boolean { - type = normalizeName(`${type}`); + type = normalizeType(type); name = normalizeName(name); + validateCase(type, name); const typeRegistry = typeMap[type]; if (!typeRegistry) { return false; @@ -238,16 +266,26 @@ export function hasItem(type: string | Type, name: string): boolean { } /** - * Normalize a string to be used as a key. By default, lowercase and trimmed. + * Normalize a string to be used as a type. Trim whitespaces and lowercase. * - * @param name The key's name. (Ex. 'FiELd_AnGlE ') + * @param name The type. (Ex. 'FiELd ') + * @returns A normalized string. (Ex. 'field') + */ +function normalizeType(type: any): string { + type = `${type}`.trim().toLowerCase(); + if (!type) throw Error('Empty type'); + return type; +} + +/** + * Normalize a string to be used as a key. Trim whitespaces. + * + * @param name The key's name. (Ex. 'field_angle ') * @returns A normalized string. (Ex. 'field_angle') */ function normalizeName(name: string): string { - // Do we need this type checking? We are using TypeScript after all. - // But there's a unit test that depends on throwing this error. if (typeof name !== 'string') throw Error('Invalid name'); - name = name.toLowerCase().trim(); + name = name.trim(); if (!name) throw Error('Empty name'); return name; } @@ -305,9 +343,9 @@ export function getAllItems( opt_throwIfMissing?: boolean, ): {[key: string]: T | null | (new (...p1: AnyDuringMigration[]) => T)} | null { if (typeof opt_cased !== 'undefined') { - deprecation.warn('Blockly.registry.getAllItems', 'v11', 'v12'); + console.warn('Blockly.registry.getAllItems deprecated "cased" argument'); } - type = normalizeName(`${type}`); + type = normalizeType(type); const typeRegistry = typeMap[type]; if (!typeRegistry) { const msg = `Unable to find [${type}] in the registry.`; diff --git a/core/serialization/workspaces.ts b/core/serialization/workspaces.ts index 07c909ccf..8103ac08d 100644 --- a/core/serialization/workspaces.ts +++ b/core/serialization/workspaces.ts @@ -24,7 +24,7 @@ export function save(workspace: Workspace): { [key: string]: AnyDuringMigration; } { const state = Object.create(null); - const serializerMap = registry.getAllItems(registry.Type.SERIALIZER, true); + const serializerMap = registry.getAllItems(registry.Type.SERIALIZER); for (const key in serializerMap) { const save = (serializerMap[key] as ISerializer)?.save(workspace); if (save) { @@ -47,7 +47,7 @@ export function load( workspace: Workspace, {recordUndo = false}: {recordUndo?: boolean} = {}, ) { - const serializerMap = registry.getAllItems(registry.Type.SERIALIZER, true); + const serializerMap = registry.getAllItems(registry.Type.SERIALIZER); if (!serializerMap) { return; } diff --git a/core/toolbox/collapsible_category.ts b/core/toolbox/collapsible_category.ts index faea8edcb..c6d7e0895 100644 --- a/core/toolbox/collapsible_category.ts +++ b/core/toolbox/collapsible_category.ts @@ -30,7 +30,8 @@ export class CollapsibleToolboxCategory implements ICollapsibleToolboxItem { /** Name used for registering a collapsible toolbox category. */ - static override registrationName = 'collapsibleCategory'; + /** Must be lowercase. */ + static override registrationName = 'collapsiblecategory'; /** Container for any child categories. */ protected subcategoriesDiv_: HTMLDivElement | null = null; @@ -75,7 +76,7 @@ export class CollapsibleToolboxCategory // Separators can exist as either a flyout item or a toolbox item so // decide where it goes based on the type of the previous item. if ( - !registry.hasItem(registry.Type.TOOLBOX_ITEM, itemDef['kind']) || + !registry.hasItem(registry.Type.TOOLBOX_ITEM, itemDef['kind'].toLowerCase()) || (itemDef['kind'].toLowerCase() === ToolboxSeparator.registrationName && prevIsFlyoutItem) @@ -109,7 +110,7 @@ export class CollapsibleToolboxCategory } const ToolboxItemClass = registry.getClass( registry.Type.TOOLBOX_ITEM, - registryName, + registryName.toLowerCase(), ); const toolboxItem = new ToolboxItemClass!( itemDef, diff --git a/core/toolbox/separator.ts b/core/toolbox/separator.ts index ec003daf6..b6bfddef9 100644 --- a/core/toolbox/separator.ts +++ b/core/toolbox/separator.ts @@ -25,6 +25,7 @@ import {ToolboxItem} from './toolbox_item.js'; */ export class ToolboxSeparator extends ToolboxItem { /** Name used for registering a toolbox separator. */ + /** Must be lowercase. */ static registrationName = 'sep'; /** All the CSS class names that are used to create a separator. */ diff --git a/core/toolbox/toolbox.ts b/core/toolbox/toolbox.ts index 19a88184c..c8e5e1c5f 100644 --- a/core/toolbox/toolbox.ts +++ b/core/toolbox/toolbox.ts @@ -366,11 +366,8 @@ export class Toolbox */ render(toolboxDef: toolbox.ToolboxInfo) { this.toolboxDef_ = toolboxDef; - for (let i = 0; i < this.contents_.length; i++) { - const toolboxItem = this.contents_[i]; - if (toolboxItem) { - toolboxItem.dispose(); - } + for (const toolboxItem of this.contents_) { + toolboxItem?.dispose(); } this.contents_ = []; this.contentMap_ = Object.create(null); diff --git a/tests/mocha/field_registry_test.js b/tests/mocha/field_registry_test.js index f8e4bb9b4..0ae086374 100644 --- a/tests/mocha/field_registry_test.js +++ b/tests/mocha/field_registry_test.js @@ -91,11 +91,9 @@ suite('Field Registry', function () { type: 'FIELD_CUSTOM_TEST', value: 'ok', }; - - const field = Blockly.fieldRegistry.fromJson(json); - - chai.assert.isNotNull(field); - chai.assert.equal(field.getValue(), 'ok'); + chai.assert.throws(function () { + Blockly.fieldRegistry.fromJson(json); + }, 'Inconsistent case'); }); }); }); diff --git a/tests/mocha/registry_test.js b/tests/mocha/registry_test.js index dca776050..029b9d788 100644 --- a/tests/mocha/registry_test.js +++ b/tests/mocha/registry_test.js @@ -55,9 +55,13 @@ suite('Registry', function () { // Changing the case or padding doesn't help. chai.assert.throws(function () { Blockly.registry.register('test', ' test_NAME ', {}); - }, 'already registered'); + }, 'Inconsistent case'); // But it's ok if explicitly allowed with 'true'. Blockly.registry.register('test', 'test_name', {}, true); + // But not ok if it's a different case. + chai.assert.throws(function () { + Blockly.registry.register('test', 'Test_Name', {}, true); + }, 'Inconsistent case'); }); test('Null Value', function () { @@ -87,16 +91,20 @@ suite('Registry', function () { }); suite('Name normalization', function () { - test('Caseless type', function () { + test('Padded', function () { + chai.assert.isTrue( + Blockly.registry.hasItem(' test ', ' test_name ') + ); + }); + + test('Case type', function () { chai.assert.isTrue(Blockly.registry.hasItem('TEST', 'test_name')); }); - test('Caseless name', function () { - chai.assert.isTrue(Blockly.registry.hasItem('test', 'TEST_NAME')); - }); - - test('Padded name', function () { - chai.assert.isTrue(Blockly.registry.hasItem(' test ', 'TEST_NAME')); + test('Case name', function () { + chai.assert.throws(function () { + Blockly.registry.hasItem('test', 'TEST_NAME'); + }, 'Inconsistent case'); }); }); }); @@ -133,16 +141,20 @@ suite('Registry', function () { }); suite('Name normalization', function () { - test('Caseless type', function () { + test('Padded', function () { + chai.assert.isNotNull( + Blockly.registry.getClass(' test ', ' test_name' ) + ); + }); + + test('Case type', function () { chai.assert.isNotNull(Blockly.registry.getClass('TEST', 'test_name')); }); - test('Caseless name', function () { - chai.assert.isNotNull(Blockly.registry.getClass('test', 'TEST_NAME')); - }); - - test('Padded name', function () { - chai.assert.isTrue(Blockly.registry.hasItem(' test ', 'TEST_NAME')); + test('Case name', function () { + chai.assert.throws(function () { + Blockly.registry.getClass('test', 'TEST_NAME'); + }, 'Inconsistent case'); }); }); }); @@ -179,16 +191,20 @@ suite('Registry', function () { }); suite('Name normalization', function () { - test('Caseless type', function () { - chai.assert.isNotNull(Blockly.registry.getObject('TEST', 'test_name')); + test('Padded', function () { + chai.assert.isNotNull( + Blockly.registry.getObject(' test ', ' test_name ') + ); }); - test('Caseless name', function () { - chai.assert.isNotNull(Blockly.registry.getObject('test', 'TEST_NAME')); + test('Case type', function () { + chai.assert.isTrue(Blockly.registry.hasItem('TEST', 'test_name')); }); - test('Padded name', function () { - chai.assert.isTrue(Blockly.registry.hasItem(' test ', 'TEST_NAME')); + test('Case name', function () { + chai.assert.throws(function () { + Blockly.registry.getObject('test', 'TEST_NAME'); + }, 'Inconsistent case'); }); }); }); @@ -200,11 +216,15 @@ suite('Registry', function () { }); teardown(function () { - Blockly.registry.unregister('test', 'casedname'); + Blockly.registry.unregister('test', 'test_name'); + Blockly.registry.unregister('test', 'casedNAME'); }); test('Has', function () { - chai.assert.isNotNull(Blockly.registry.getAllItems('test')); + chai.assert.deepEqual(Blockly.registry.getAllItems('test'), { + 'test_name': {}, + 'casedNAME': {}, + }); }); test('Does not have', function () { @@ -222,14 +242,6 @@ suite('Registry', function () { test('Ignore type case', function () { chai.assert.isNotNull(Blockly.registry.getAllItems('TEST')); }); - - test('Overwriting name case', function () { - Blockly.registry.register('test', 'CASEDname', {}, true); - chai.assert.deepEqual(Blockly.registry.getAllItems('test'), { - 'test_name': {}, - 'casedname': {}, - }); - }); }); suite('getClassFromOptions', function () {