Skip to content

Create canonical module alias if module name contains forgebox username #637

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.

Sign up for GitHub

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

Closed
homestar9 wants to merge 2 commits into from
Closed

Create canonical module alias if module name contains forgebox username #637

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e02c01
Create canonical module alias if module name contains forgebox username
homestar9 Jan 6, 2026
a1c2bde
Update system/web/services/ModuleService.cfc
homestar9 Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions system/web/services/ModuleService.cfc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,11 +452,26 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" {
// Store module configuration in main modules configuration
modulesConfiguration[ modName ] = mConfig;

// Link aliases by reference in both modules list and config cache
for ( var thisAlias in mConfig.aliases ) {
modulesConfiguration[ thisAlias ] = modulesConfiguration[ modName ];
variables.mConfigCache[ thisAlias ] = variables.mConfigCache[ modName ];
}
/// If module name contains ForgeBox username (@username), create a canonical alias
// This allows DSL injection like inject="coldbox:moduleSettings:modulename"
// to work even when the module is installed as modulename@username
if ( find( "@", modName ) ) {
var canonicalName = listFirst( modName, "@" );
// Only create alias if it doesn't conflict with an existing module
if ( !structKeyExists( modulesConfiguration, canonicalName ) ) {
modulesConfiguration[ canonicalName ] = modulesConfiguration[ modName ];
variables.mConfigCache[ canonicalName ] = variables.mConfigCache[ modName ];
if ( variables.logger.canDebug() ) {
variables.logger.debug(
"Created canonical alias [#canonicalName#] for ForgeBox module [#modName#]"
);
}
} else if ( variables.logger.canWarn() ) {
variables.logger.warn(
"Cannot create canonical alias [#canonicalName#] for ForgeBox module [#modName#] - name conflict with existing module"
);
}
}
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The new ForgeBox alias code has replaced the original module alias handling logic, which breaks existing functionality. The original code that processed the mConfig.aliases array (configured in ModuleConfig.cfc) has been completely removed. This means modules that define custom aliases will no longer have those aliases registered in modulesConfiguration and mConfigCache.

The ForgeBox alias logic should be added AFTER the original alias handling code, not as a replacement. Both alias mechanisms need to coexist - one for module-defined aliases and one for ForgeBox username canonicalization.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@homestar9 homestar9 Jan 6, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Copy link
Contributor Author

@homestar9 homestar9 Jan 7, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Copy link
Contributor Author

@homestar9 homestar9 Jan 7, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback


// Update the paths according to conventions
mConfig.handlerInvocationPath &= ".#replace(
Expand Down
115 changes: 115 additions & 0 deletions tests/specs/web/services/ModuleServiceTest.cfc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,121 @@ component extends="tests.resources.BaseIntegrationTest" {
expect( variables.moduleService.getModuleRegistry() ).toHaveKey( "test-module" );
} )
} );

describe( "ModuleService ForgeBox Alias Creation", function(){

it( "should create canonical alias when module name contains @", function(){
// Arrange
var moduleService = createMock( "coldbox.system.web.services.ModuleService" );
var modules = {};
var mConfigCache = {};

// Mock the logger
var mockLogger = createStub().$( "canDebug", true ).$( "debug" );
moduleService.$property( "logger", "variables", mockLogger );
moduleService.$property( "mConfigCache", "variables", mConfigCache );

// Simulate module config
var mConfig = {
name : "testmodule@testuser",
settings : { apiKey : "test-123" },
aliases : []
};

// Act - simulate what happens in ModuleService.cfc after line 453
var modName = "testmodule@testuser";
modules[ modName ] = mConfig;

// The fix code
if ( find( "@", modName ) ) {
var canonicalName = listFirst( modName, "@" );
if ( !structKeyExists( modules, canonicalName ) ) {
modules[ canonicalName ] = modules[ modName ];
mConfigCache[ canonicalName ] = mConfigCache[ modName ];
}
}

// Assert
expect( modules ).toHaveKey( "testmodule@testuser", "Full name should exist" );
expect( modules ).toHaveKey( "testmodule", "Canonical alias should be created" );
expect( modules[ "testmodule" ] ).toBe( modules[ "testmodule@testuser" ], "Alias should reference same config" );
});

it( "should NOT create alias when module name has no @", function(){
// Arrange
var modules = {};
var mConfigCache = {};
var mConfig = { name : "regularmodule", settings : {} };

// Act
var modName = "regularmodule";
modules[ modName ] = mConfig;

if ( find( "@", modName ) ) {
var canonicalName = listFirst( modName, "@" );
if ( !structKeyExists( modules, canonicalName ) ) {
modules[ canonicalName ] = modules[ modName ];
}
}

// Assert
expect( modules ).toHaveKey( "regularmodule" );
expect( structCount( modules ) ).toBe( 1, "Should only have one entry" );
Comment on lines +52 to +107
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

The test is duplicating the production code logic (lines 74-80) instead of testing the actual ModuleService.registerModule method. This makes the test fragile and doesn't verify the actual implementation. Consider calling the real registerModule method with a test module configuration to verify the alias creation behavior works end-to-end in the actual service.

Suggested change
// Arrange
var moduleService = createMock( "coldbox.system.web.services.ModuleService" );
var modules = {};
var mConfigCache = {};
// Mock the logger
var mockLogger = createStub().$( "canDebug", true ).$( "debug" );
moduleService.$property( "logger", "variables", mockLogger );
moduleService.$property( "mConfigCache", "variables", mConfigCache );
// Simulate module config
var mConfig = {
name : "testmodule@testuser",
settings : { apiKey : "test-123" },
aliases : []
};
// Act - simulate what happens in ModuleService.cfc after line 453
var modName = "testmodule@testuser";
modules[ modName ] = mConfig;
// The fix code
if ( find( "@", modName ) ) {
var canonicalName = listFirst( modName, "@" );
if ( !structKeyExists( modules, canonicalName ) ) {
modules[ canonicalName ] = modules[ modName ];
mConfigCache[ canonicalName ] = mConfigCache[ modName ];
}
}
// Assert
expect( modules ).toHaveKey( "testmodule@testuser", "Full name should exist" );
expect( modules ).toHaveKey( "testmodule", "Canonical alias should be created" );
expect( modules[ "testmodule" ] ).toBe( modules[ "testmodule@testuser" ], "Alias should reference same config" );
});
it( "should NOT create alias when module name has no @", function(){
// Arrange
var modules = {};
var mConfigCache = {};
var mConfig = { name : "regularmodule", settings : {} };
// Act
var modName = "regularmodule";
modules[ modName ] = mConfig;
if ( find( "@", modName ) ) {
var canonicalName = listFirst( modName, "@" );
if ( !structKeyExists( modules, canonicalName ) ) {
modules[ canonicalName ] = modules[ modName ];
}
}
// Assert
expect( modules ).toHaveKey( "regularmodule" );
expect( structCount( modules ) ).toBe( 1, "Should only have one entry" );
// Arrange: use the real ModuleService from beforeAll()
var moduleService = variables.moduleService;
var mConfig = {
name : "testmodule@testuser",
settings : { apiKey : "test-123" },
aliases : []
};
// Act: register the module and let ModuleService handle alias creation
moduleService.registerModule( mConfig.name, mConfig );
var modules = moduleService.getModules();
// Assert
expect( modules ).toHaveKey( "testmodule@testuser", "Full name should exist" );
expect( modules ).toHaveKey( "testmodule", "Canonical alias should be created" );
expect( modules[ "testmodule" ] ).toBe(
modules[ "testmodule@testuser" ],
"Alias should reference same config"
);
});
it( "should NOT create alias when module name has no @", function(){
// Arrange: snapshot current modules
var moduleService = variables.moduleService;
var beforeModules = duplicate( moduleService.getModules() );
var beforeCount = structCount( beforeModules );
var mConfig = {
name : "regularmodule_nocanonical_test",
settings : {}
};
// Act: register a module with no @ in its name
moduleService.registerModule( mConfig.name, mConfig );
var modules = moduleService.getModules();
// Assert: only one new module entry should have been added
expect( modules ).toHaveKey( "regularmodule_nocanonical_test" );
expect( structCount( modules ) ).toBe(
beforeCount + 1,
"Should only add one module entry when there is no @ in the name"
);

Copilot uses AI. Check for mistakes.
});
Comment on lines +88 to +108
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

Similar to the first test, this test duplicates the production code logic (lines 98-103) rather than testing the actual ModuleService implementation. Consider using an integration test approach that calls the real service methods.

Copilot uses AI. Check for mistakes.

it( "should NOT create alias if canonical name already exists (conflict)", function(){
// Arrange
var modules = {};
var mConfigCache = {};
var mockLogger = createStub().$( "canWarn", true ).$( "warn" );

// Canonical module exists
modules[ "mymodule" ] = { name : "mymodule", settings : { source : "canonical" } };

// Act - try to register ForgeBox module with same canonical name
var modName = "mymodule@testuser";
modules[ modName ] = { name : modName, settings : { source : "forgebox" } };

if ( find( "@", modName ) ) {
var canonicalName = listFirst( modName, "@" );
if ( !structKeyExists( modules, canonicalName ) ) {
modules[ canonicalName ] = modules[ modName ];
mConfigCache[ canonicalName ] = mConfigCache[ modName ];
} else if ( !isNull( mockLogger ) && mockLogger.canWarn() ) {
mockLogger.warn( "Cannot create canonical alias" );
}
}

// Assert
expect( modules ).toHaveKey( "mymodule" );
expect( modules ).toHaveKey( "mymodule@testuser" );
expect( modules[ "mymodule" ].settings.source ).toBe( "canonical", "Original should remain unchanged" );
expect( modules[ "mymodule@testuser" ].settings.source ).toBe( "forgebox", "ForgeBox module should be separate" );
expect( mockLogger.$once( "warn" ) ).toBeTrue( "Should log warning about conflict" );
});
Comment on lines +110 to +139
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

This test also duplicates the production code logic (lines 123-131) rather than testing the actual ModuleService. Additionally, the test creates a mockLogger but never properly wires it into the execution context, so the logger warning behavior isn't actually being tested against the real implementation.

Copilot uses AI. Check for mistakes.

it( "should handle multiple @ symbols correctly", function(){
// Arrange
var modules = {};
var mConfigCache = {};

// Act
var modName = "my-module@user@extra";
modules[ modName ] = { name : modName, settings : {} };

if ( find( "@", modName ) ) {
var canonicalName = listFirst( modName, "@" );
if ( !structKeyExists( modules, canonicalName ) ) {
modules[ canonicalName ] = modules[ modName ];
}
}

// Assert
expect( modules ).toHaveKey( "my-module@user@extra" );
expect( modules ).toHaveKey( "my-module", "Should extract name before first @" );
});
Comment on lines +141 to +160
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

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

This test also duplicates the production code logic (lines 150-155) instead of testing the actual ModuleService implementation. Consider consolidating these unit tests into integration tests that verify the actual behavior of the ModuleService.registerModule method.

Suggested change
it( "should handle multiple @ symbols correctly", function(){
// Arrange
var modules = {};
var mConfigCache = {};
// Act
var modName = "my-module@user@extra";
modules[ modName ] = { name : modName, settings : {} };
if ( find( "@", modName ) ) {
var canonicalName = listFirst( modName, "@" );
if ( !structKeyExists( modules, canonicalName ) ) {
modules[ canonicalName ] = modules[ modName ];
}
}
// Assert
expect( modules ).toHaveKey( "my-module@user@extra" );
expect( modules ).toHaveKey( "my-module", "Should extract name before first @" );
});

Copilot uses AI. Check for mistakes.

});
}

}
Loading