Create canonical module alias if module name contains forgebox username #637
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for canonical module aliases when ForgeBox modules are installed with a username (e.g., modulename@username). The feature allows DSL injection to work with or without the username by creating an alias that strips the @username portion. This addresses COLDBOX-1374 where private ForgeBox modules couldn't be injected without the username qualifier.
Key Changes:
- Added logic to detect module names containing
@and create canonical aliases - Includes conflict detection to prevent overwriting existing modules
- Added comprehensive unit tests for the ForgeBox alias creation feature
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| system/web/services/ModuleService.cfc | Added ForgeBox username detection and canonical alias creation with logging; however, this replaces the existing module alias handling code |
| tests/specs/web/services/ModuleServiceTest.cfc | Added new test suite with multiple test cases covering ForgeBox alias creation scenarios |
| /// 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" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
This pull request has been mentioned on Ortus Solutions Community. There might be relevant details there: |
Co-authored-by: Copilot <[email protected]>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
| // 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" ); |
There was a problem hiding this comment.
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.
| // 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" | |
| ); |
| 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" ); | ||
| }); |
There was a problem hiding this comment.
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.
| 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" ); | ||
| }); |
There was a problem hiding this comment.
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.
| 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 @" ); | ||
| }); |
There was a problem hiding this comment.
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.
| 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 @" ); | |
| }); |
|
Closing this PR as I think I was overthinking the problem. |
2 participants
Description
When a Forgebox module is installed with a username (private module), Wirebox now canonicalizes the mapping so it can be injected with or without the username in the DSL. Fixes COLDBOX-1374
Jira Issues
https://ortussolutions.atlassian.net/browse/COLDBOX-1374
Type of change
Please delete options that are not relevant.