Lucee's released a new "stable" version and we're being encouraged to use it. Today I upgraded my dev environment to Lucee 5.3.10.120 (from 5.3.9.166 which seems quite stable and gave us no issues), ran our tests to see if anything obvious went wrong, and a whole lot of them went splat. All of them were variations of this (this is from my repro case, not our codebase):
Lucee 5.3.10.120 Error (expression)
Message
Cannot update key [MAORI_NUMBERS] in static scope from component [cfml.vendor.lucee.staticFinal.C], that member is set to final
Notice how I am not extending anything there? This is significant.
What's in Base.cfc? I'm kind of embarrassed to show you this.
component {
include "include.cfm";
}
"WTF?" you might legitimately ask. Well: quite. The thing is I found this issue in a CFWheels application, and it's down to CFWheels's "architecture" that this bug in Lucee surfaces.
Look at this… stuff:
component output="false" displayName="Model" {
include "model/functions.cfm";
include "global/functions.cfm";
include "plugins/standalone/injection.cfm";
}
That's how CFWheels implements its base model class. They are pretending CFML has mixins (it doesn't) by using includes. All their classes seem to be defined as a series of include files. I just… just… I just… … aaaaaaaah!!! Just do me a favour and don't use CFWheels. Trust me.
Anyway, so this is why I'm writing shit code to reproduce this Lucee bug.
In include.cfm I have this:
public function build() {
return this
}
(This is also how CFWheels initialises its model class objects: using a sort of factory method. So I'm replicating a pared-down version of that).
Yeah. <cfinvoke>. Ain't seen one of those in about a decade (and it was old code then…).
And if I run that code, I get the error concerned.
I've tried to pare it back further, but it seems I need the sub class, base class, include combo. Lucee is - it seems - doing something dodgy when it instantiates an object like this. No surprise they didn't catch it during their own tests, because this is a very - um - "edge case" approach to designing code.
Another weird thing is that if I restart my Docker container: the problem goes away. However if I then do something like change the name of include.cfm to includex.cfm or something, the problem comes back. Adding code to either CFC does not bring the issue back (if you see what I mean). It's def down to something about how a base class that has an include in it is first "created" (I hesitate to say "compiled" here, cos then I don't think the issue would magically go away between container restarts: the compiled code seems fine. Just the in-memory code after the compilation the first time round is crocked. Anyhow, I won't try to guess what's wrong, I'll leave that to the Lucee bods. They can have my repro case to help them (here on GitHub: /vendor/lucee/staticFinal).
I encountered a ColdFusion bug during assessing this: time to write that one up. (update: this one has already been reported, as it turns out: CF-4213214).
I'm going to pass this on to the CFWheels Dev Team, but it's applicable in anyone's code, so posting it here first.
A lot of the code in the CFWheels codebase hasn't really been "designed". It's been written as if the developer concerned just went "OK, I have this file open… I'll start typing code in this file…", rather than giving thought to the notions of design patterns or the SOLID principles and stuff like that that exist to keep codebases… usable.
It's legacy code, and we have all written code like this in the past, so let's not dwell too much on how the codebase got to where it is now. Let's just accept that it could be better than it is.
But It's 2022 now, and there's no real excuse for perpetuating coding practices like this.
An example in front of me ATM is the onApplicationStart method. It is 1000 lines long. That is ludicrous.
The design problem in this particular method (and this is the same for a lot of code in the CFWheels codebase) is that it confuses "the things I need to get done", with "how to do those things". It's onApplicationStart's job to… start the application. There are numerous steps to this, but instead of it just calling all the steps it needs to call to define "starting the application", it also has all the implementation of the steps inline in it as well. This is an anti-pattern.
One can even see what each of the steps are. There are comments identifying them. In fact the first two are done "right":
// Abort if called from incorrect file.
$abortInvalidRequest();
// Setup the CFWheels storage struct for the current request.
$initializeRequestScope();
(Except the comments here are pointless as they simply repeat what the method names already say clearly).
But then the wheels (sic) fall off:
if (StructKeyExists(application, "wheels")) {
// Set or reset all settings but make sure to pass along the reload password between forced reloads with "reload=x".
if (StructKeyExists(application.wheels, "reloadPassword")) {
local.oldReloadPassword = application.wheels.reloadPassword;
}
// Check old environment for environment switch
if (StructKeyExists(application.wheels, "allowEnvironmentSwitchViaUrl")) {
local.allowEnvironmentSwitchViaUrl = application.wheels.allowEnvironmentSwitchViaUrl;
local.oldEnvironment = application.wheels.environment;
}
}
application.$wheels = {};
if (StructKeyExists(local, "oldReloadPassword")) {
application.$wheels.reloadPassword = local.oldReloadPassword;
}
That could should not be inline. There should be a method call inline, called something like configureReloadOptions (or whatever this is doing… a case in point is that I can't really tell).
The thing is onApplicationStart should not be defining the implementation of this, something else should be doing that, and onApplicationStart should just know how to call it.
Why is this an issue?
Because for us, one of the sections of processing in onApplicationStart is wrong, and sufficiently wrong it's been flagged up as a pen vector.
If each part of the application-start-up sequence was in its own functions, then we could override the function with our own implementation to address the issue. And the change would be local to that one small function, so the testing surface area would be small. But instead I now have to hack a third-party codebase to solve this. It's 100s of lines into this method, and accordingly its test surface area is huge (and largely unreachable, I think).
My recommendation / rule of thumb is that whenever one is tempted to put a comment in that explains a block of code (or delimit it from other blocks), then that block of code should be in its own function. If one follows this rule of thumb, one seldom gets into the situation that one has Open/Closed Principle issues. It's not foolproof, but it's a good start.
Template Method design pattern
2023-01-31
As Ben points out in the comments, this is an example of the Template Method design pattern at work. So it's not like I am advocating some esoteric approach that I made up myself, it's an established design pattern we ought to be using.
Cheers for the heads-up Ben.
Also having a code design rule that functions should start to raise a flag if they are over 20 lines long. They are probably doing too much, or aren't factored well. If they are more than 50 lines? Stop. Recode them.
If a function has unrelated flow-control blocks in them? Also a red flag. Each block is likely more appropriate to be in their own functions.
A function like onApplicationStart that needs to carry-out a _lot_ of steps to consider the app to be "started" should look something like this:
It really shouldn't be implementing any stuff or things or this or that itself.
Lastly, someone drew my attention to this statement on the CFWheels home page:
Good Organization
Stop thinking about how to organize your code and deal with your business specific problems instead.
This possibly explains a lot of the way the CFWheels codebase got to be the way it is. It is really bad advice. One should always be thinking about how one's application ought to be designed. It's important.
NB: this is not just related to onApplicationStart. That's just what currently has me sunk. It relates to really a lot of the code in the codebase.
This is also something that can be tackled in a piecemeal fashion. If one has to maintain some code in a really long method: extract the block of code in question into its own function, and maintain (and test that).
This exercise came about from a bug in CFWheels we encountered today.
We got a notification that a user had received a 500-error, and I was looking into it. It was happening in /wheels/global/internal.cfm, in this bit of code:
for (local.key in application.wheels.cache[arguments.category]) {
if (Now() > application.wheels.cache[arguments.category][local.key].expiresAt) {
On the second line we are getting an error: key [81AB90EB428410D6222B18AAE12EC014] doesn't exist.
It should be obvious to even a journeyman dev what the problem is here.
When the first line of code runs, there is a key 81AB90EB428410D6222B18AAE12EC014 in application.wheels.cache. By the time the second line of code runs, some other request has come along and removed 81AB90EB428410D6222B18AAE12EC014 from application.wheels.cache, and hence we get the error. This is why everyone (?) knows that one must synchronise access to shared scopes like the application scope, so that if a process is writing to it, then other processes can't read from it. Or competing writes can't be happening simultaneously. IE: Chuck a lock block around this sort of thing. This is CFML 101 sort of stuff, CFWheels Team.
On the face of it, this was just super bad timing. What are the odds that one request will run the first line, and then before that request gets to the second line, another request has blitzed what the first line "knew" was there to use.
But it's not really super bad timing. It's just bad coding. This code is a loop. When does a for (key in struct) loop decide which keys exist in the struct, and accordingly which keys to loop over? At the first time the loop is entered. And only then. So if there are 10000 items in that struct when our request first ran, any other process has from the first iteration until the 9999th iteration to delete the 10000th key, and we'll get this error. That's a lot of opportunity for failure. Also, due to the very nature of this code, there's a good chance that a bunch of requests are gonna be running it simultaneously: the key deletions come after a determination that a shared cache is full and needs culling (let's not get into matter of why a function called addToCache is deleting stuff anyhow. This is not the worst thing about this cluster🦆, I assure you), and at the moment in time the cache seems to be full, then every call to addToCache (sic) will try to delete stuff. Ugh.
To demonstrate what's going on here, I've knocked a quick example together (code on github):
All very straight forward. In secondRequest.cfm I only add a key so there's a visual cue in the output of firstRequest.cfm when the second request ran.
The test is to run firstRequest.cfm, then whist that's running, run secondRequest.cfm. The output from a typical run of firstRequest.cfm would be like:
keys @ top of loop: one,two,three,four
current key: one
current value: tahi
keys after sleep: one,two,three,four
keys @ top of loop: one,two,three,four
current key: two
current value: rua
keys after sleep: one,two,three,five <--- here is where I ran secondRequest.cfm
keys @ top of loop: one,two,three,five
current key: three
current value: toru
keys after sleep: one,two,three,five
keys @ top of loop: one,two,three,five
current key: four
[...]
Error Occurred While Processing Request
Element FOUR is undefined in a CFML structure referenced as part of an expression.
You can see that the keys the loop iterates over is set at the beginning, given it's unaffected by changes to the struct's keys during the loop.
What should the CFWheels codebase do to mitigate this. Well: here's a list of things that are… erm… "less good than they could be "… in that code.
Framework functionality should be in classes, not in include files. In this case cache-handing code should be in a CachingService or somesuch.
internal.cfm is an awful name for a library. A filename should describe what the code does (like, say "CachingService"), not how it's used ("internally", apparently. Sounds uncomfortable, not to mention a bit unhygienic).
Related to this, that file is a hotchpotch mess of all sorts of shite that should be homed somewhere else. This describes most files in the CFWheels codebase, unfortunately.
The only code that touches the application scope should be in an ApplicationScopeProxy object. BTW this makes controlling the scope-locking way easier to manage. See also: SessionScopeProxy / ServerScopeProxy / etc
Within that proxy, access to any shared scope should be appropriately synchronised (with a lock). This should also be extended to mitigating race conditions like we have here.
If your function is called addToCache then stick to doing that. Code that removes stuff from the cache does not belong there.
All the code in that method is conditional. All of it. There is no "mainline" or happy path. Simplify the conditions and bail out if they're not met. Don't put the code you actually expect to run inside a conditional block.
Connected to the two preceding points: most of the code in that method is not actually anything to do with adding something to a cache.
For the love of god stop prefixing functions with $ to indicate they're private. Just make them private.
I don't think anything there is particularly controversial or complicated. And whilst it's beyond "CFML 101", I think it's within the low bar of "Writing Frameworks 101".
Anyway. There you got. A coupla hours in the life of someone who has to use CFWheels.
This follows on from CFML: implementing dependency injection in a CFWheels web site. In that article I got the DI working, but only with a test scenario. For the sake of completeness, I'm gonna continue with the whole point of the exercise, which is getting a logger service into my model objects, via DI.
0.6 adding LogBox
I ran into some "issues" yesterday whilst trying to write this article. Documented here: "A day in the life of trying to write a blog article in the CFML ecosystem", and there's some file changes committed as 0.5.1. So that killed my motivation to continue this work once I'd waded through that. I appear to be back on track now though.
Right so I'll add a quick test to check LogBox is installed. It's a bit of a contrived no-brainer, but this is playing to my strengths. So be it:
If LogBox is where it's supposed to be: it'll pass. Initially I had a new LogBox() in there, but it needs some config to work, and that requires a bit of horsing around, so I'll deal with that next. For now: is it installed? Test sez "no":
Test sez… yes.
OK. That was unexpected. Why did that pass? I have checked that LogBox is not installed, so WTH??
After a coupla hours of horsing about looking at TestBox code, I worked out there's a logic… um… shortfall (shall I say) in its implementation of that regex param, which is a bit wayward. The code in question is this (from /system/Assertion.cfc):
if (
len( arguments.regex ) AND
(
!arrayLen( reMatchNoCase( arguments.regex, e.message ) )
OR
!arrayLen( reMatchNoCase( arguments.regex, e.detail ) )
)
) {
return this;
}
Basically this requires both of message and detail to not match the regex for it to be considered "the same" exception. This is a bit rigorous as it's really unlikely for this to be the case in the real world. I've raised it with Ortus (TESTBOX-349), but for now I'll just work around it. Oh yeah, there's a Lucee bug interfering with this too. When an exception does have the same message and details, Lucee ignores the details. I've not raised a bug for this yet: I'm waiting for them to fee-back as to whether I'm missing something. When there's a ticket, I'll cross-post it here.
Anyway, moving on, I'll just check for any exception, and that'll do:
0.7 wiring LogBox into the DependencyInjectionService
One of the reasons the previous step really didn't push the boat out with testing if LogBox was working, is that to actually create a working LogBox logger takes some messing about; and I wanted to separate that from the installation. And also to give me some time to come up with the next test case. I want to avoid this sort of thing:
I don't want to skip to a test that is "it can log stuff that happens in the Test model object". I guess it is part ofthe requiremnt that the logger is handled via dependency injection into the model, so we can first get it set up and ready to go in the DependencyInjectionService. I mean the whole thing here is about DI: the logger is just an example usage. I think the next step is legit.
I've never used LogBox before, so I am RTFMing all this as I type (docs: "Configuring LogBox"). It seems I need to pass a Config object to my LogBox object, then get the root logger from said object… and that's my logger. Al of that can go in a factory method in configureDependencies, adn I'll just put the resultant logger into the IoC container.
it("loads a logger", () => {
diService = new DependencyInjectionService()
logger = diService.getBean("Logger")
expect(logger).toBeInstanceOf("logbox.system.logging.Logger")
expect(() => logger.info("TEST")).notToThrow()
})
I'm grabbing a logger and logging a message with it. The expectation is simply that the act of logging doesn't error. For now.
First here's the most minimal config I seem to be able to get away with:
The docs ("LogBox DSL
") seemed to indicate I only needed the logBox struct, but it errored when I used it unless I had at least one appender. I'm just using a dummy one for now because I'm testing config, not operations. And there's nothing to test there: it's all implementation, so I think it's fine to create that in the "green" phase of "red-green-refactor" from that test above (currently red). With TDD the red phase is just to do the minimum code to make the test pass. That doesn't mean it needs to be one line of code, or one function or whatever. If my code needed to call a method on this Config object: then I'd test that separately. But I'm happy that this is - well - config. It's just data.
Once we have that I can write my factory method on DependencyInjectionService:
private function configureDependencies() {
variables.container.declareBean("DependencyInjectionService", "services.DependencyInjectionService")
variables.container.declareBean("TestDependency", "services.TestDependency")
variables.container.factoryBean("Logger", () => {
config = new Config()
logboxConfig = new LogBoxConfig(config)
logbox = new LogBox(logboxConfig)
logger = logbox.getRootLogger()
return logger
})
}
I got all that from the docs, and I have nothing to add: it's pretty straight forward. Let's see if the test passes:
Cool.
Now I need to get my Test model to inject the logger into itself, and verify I can use it:
Here I am mocking the logger's debug method, just so I can check it's being called, and with what. Having done this, I am now wondering about "don't mock what you don't own", but I suspect in this case I'm OK because whilst the nomenclature is all "mock", I'm actually just spying on the method that "I don't own". IE: it's LogBox's method, not my application's method. I'll have to think about that a bit.
And the implementation for this is way easier than the test:
// models/Test.cfc
private function setDependencies() {
variables.dependency = variables.diService.getBean("TestDependency")
variables.logger = variables.diService.getBean("Logger")
}
public function getMessage() {
variables.logger.debug("getMessage was called")
return variables.dependency.getMessage()
}
Just for the hell of it, I also wrote a functional test to check the append was getting the expected info:
it("logs via the correct appender", () => {
test = model("Test").new()
prepareMock(test)
logger = test.$getProperty("logger")
appenders = logger.getAppenders()
expect(appenders).toHaveKey("DummyAppender", "Logger is not configured with the correct appender. Test aborted.")
appender = logger.getAppenders().DummyAppender
prepareMock(appender)
appender.$("logMessage").$results(appender)
test.getMessage()
appenderCallLog = appender.$callLog()
expect(appenderCallLog).toHaveKey("logMessage")
expect(appenderCallLog.logMessage).toHaveLength(1)
expect(appenderCallLog.logMessage[1]).toSatisfy((actual) => {
expect(actual[1].getMessage()).toBe("getMessage was called")
expect(actual[1].getSeverity()).toBe(logger.logLevels.DEBUG)
expect(actual[1].getTimestamp()).toBeCloseTo(now(), 2, "s")
return true
}, "Log entry is not correct")
})
It's largely the same as the unit test, except it spies on the appender instead of the logger. There's no good reason for doing this, I was just messing around.
This is not the article I intended to write today. That article was gonna be titled "CFML: Adding a LogBox logger to a CFWheels app via dependency injection", but I'll need to get to that another day now.
Here's how far that article got before the wheels fell off:
And that was it.
Why? Well I started by writing an integration test just to check that box install logbox did what I expected:
Simple enough. It'll throws an exception if LogBox ain't there, and I'm expecting that. It's a dumb test but it's a reasonable first step to build on.
I run the test:
Err… come again? I ain't installed it yet. I lifted the code from the expect callback out and run it "raw" in the body ofthe test case: predictable exception. I put it back in the callback. Test passes. I change the matcher to be toThrow. Test still passed. So this code both throws and exception and doesn't throw an exception. This is pleasingly Schrödingeresque, but not helpful.
The weird thing is I know this is not a bug in TestBox, cos we use notToThrow in our tests at work. I port the test over to my work codebase: test fails (remember: this is what I want ATM, we're still at the "red" of "red-green-refactor").
I noticed that we were running a slightly different version of Testbox in the work codebase: 4.4.0-snapshot compared to my 4.5.0+5. Maybe there's been a regression. I changed my TestBox version in box.json and - without thinking things through - went box install again (not just box install testbox which is all I really needed to do), and was greeted with this:
That's reasonably bemusing as I had just used box install fw1 to install it in the first place, and that went fine. And I have not touched it since. I checked what version I already had installed (in framework/box.json), and it claims 4.3.0. So… ForgeBox… I beg to differ pal. You found this version y/day, why can't you find it today? I check on ForgeBox, and for 4.x I see versions 4.0.0, 4.1.0, 4.2.0, 4.5.0-SNAPSHOT. OK, so granted: no 4.3.0. Except that's what it installed for me yesterday. Maybe 4.3.0 has issues and got taken down in the last 24h (doubtful, but hey), so I blow away my /framework directory, and remove the entry from box.json, and box install fw1 again. This is interesting:
4.2.0. But its entry in its own box.json is 4.3.0, and the constraint it put in my box.json is ^4.3.0.
I do not have time or inclination for any of this, so I just stick a constraint of ~4.2.0 in my box.json, and that seems to have solved it. I mean the error went away: it's still installing 4.3.0. Even with a hard-coded 4.2.0 it's still installing 4.3.0.
Brad Wood from Ortus/CommandBox had a look at this, nutted-out that there was something wrong with the way the FW/1 package on ForgeBox was configured, and he in turn pinged Steve Neiland who looks after FW/1 these days, and he got this sorted. I'm now on 4.3.0, and it says it's 4.2.0. And box install no longer complains at me. Cheers fellas.
Then I noticed that because of the stupid way CFWheels "organises" itself in the file system, I have inadvertantly overwritten a bunch of my own CFWheels files. Sigh. CFWheels doesn't bother to package itself up as "app" (its stuff) and "implementation" (my code that uses their app), it just has "here's some files: some you should change (anything outside the wheels subdirectory), some you probably shouldn't (the stuff in the wheels subdirectory)", but there's no differentiation when it comes to installation: all the files are deployed. Overwriting all the user-space files with their original defaults. Sorry but this is just dumbarsey. Hurrah for source control and small commit iterations is all I can say, as I could just revert some files and I was all good.
Right so now I have the same version of TestBox installed here as in our app at work (remember how this was all I was tring to do? Update testbox. Nothing to do with FW/1, and nothing to do with CFWheels. But there's an hour gone cocking around with that lot).
And the test still doesn't work. Ballocks.
I notice the Lucee version is also different. We're locked into an older version of Lucee at work due to bugs and incompats in newer versions that we're still waiting on to be fixed, so the work app is running 5.3.7.47, and I am on 5.3.8.206. Surely it's not that? I rolled my app's Lucee version back to 5.3.7.47 and the test started failing (correctly). OK, so it's a Lucee issue.
I spent about an hour messing around doing a binary search of Lucee container versions until I identified the last version that wasn't broken (5.3.8.3) and the next version - a big jump here - 5.3.8.42 that was broken. I looked at a diff of the code but nothing leapt out at me. This was slightly daft as I had no idea what I was looking for, so that was probably half an hour of time looking at Lucee's source code in an aimless fashion. I actually saw the change that was the problem, but didn't clock that that is what caused it at the time.
Having drawn a blank, I slapped my forehead, called myself a dick, and went back to the code in TestBox that was behaving differently. That would obviously tell me where to look for the issue.
There are some Java method calls there to act as controls, but on Lucee's current version, we get this:
And on earlier versions it's this:
(Full disclosure: I'm using Lucee 4.5 on trycf.com for that second dump, but it's the same results in earlier versions of Lucee 5, up to the point where it starts going wrong)
Note how previously a regex match of .* matches an empty string? This is correct. It does. In all regex engines I know of. Yet in Lucee's current versions, it returns a completely empty array. This indicates no match, and it's wrong. Simple as that. So there's the bug.
I was pointed in the direction of an existing issue for this: LDEV-3703. Depsite being a regression they know they caused, Lucee have decided to only fix it in 6.x. Not the version they actually broke. Less than ideal, but so be it.
There were a coupla of Regex issues dealt with between those Lucee versions I mentioned before. Here's a Jira search for "fixversion >= 5.3.8.4 and fixversion < 5.3.8.42 and text ~ regex". I couldn't be arsed tracking back through the code, but I did find something in LDEV-3009 mentioning a new Application.cfc setting:
this.useJavaAsRegexEngine = true
This is documented for ColdFusion in Application variables, and… absolutely frickin' nowhere in the Lucee docs, as far as I can see.
On a whim I stuck that setting in my Application.cfc and re-ran the test. If the setting was false: the test doesn't work. If it was true: the test does work. That's something, but Lucee is not off the hook here. The behaviour of that regex match does not change between the old and new regex engines! .* always matches an empty string! So there's still a bug.
However, being pragmatic, I figured "problem solved" (for now), and moved on. For some reason I restarted my container, and re-hit my tests:
I switched the setting to this.useJavaAsRegexEngine = false and the tests ran again (failed incorrectly, but ran). So… let me get this straight. For TestBox to work, I need to set that setting to true. To get CFWheels to work, I need to set it to false.
For pete's sake.
As I said on the Lucee subchannel on the CFML Slack:
Do ppl recall how I've always said these stupid flags to change behaviour of the language were a fuckin dumb idea, and are basically unusable in a day and age where we all rely on third-party libs to do our jobs?
Exhibit. Fucking. A.
Every single one of these stupid, toxic, setting doubles the overhead for library providers to make their code work. I do not fault TestBox or CFWheels one bit here. They can't be expected to support the exponential number of variations each one of those settings accumulates. I can firmly say that no CFML code should ever be written that depends on any of these settings. And no library or third-party code should ever be tested with the setting variation on. Just ignore them. The settings should not exist. Anyway: this is an editorial digression. "We are where we are" as the over-used saying goes.
Screw all this. Seriously. All I wanted to do is to do a blog article about perhaps 50-odd new lines of code in my example app. Instead I spent four hours untangling this shite. And my blog article has not progressed.
Here's what I needed to do to my app to work around these various issues:
Recently I wanted to abstract some logic out of one of our CFWheels model classes, into its own representation. Code had grown organically over time, with logic being inlined in functions, making a bunch of methods a bit weighty, and had drifted well away from the notion of following the "Single Responsibility Principle". Looking at the code in question, even if I separated it out into a bunch of private methods (it was a chunk of code, and refactoring into a single private method would not have worked), it was clear that this was just shifting the SRP violation out of the method, and into the class. This code did not belong in this class at all. Not least of all because we also needed to use some of it in another class. This is a pretty common refactoring exercise in OOP land.
I'm going to be a bit woolly in my descriptions of the functionality I'm talking about here, because the code in question is pretty business-domain-specific, and would not make a lot of sense to people outside our team. Let's just say it was around logging requirements. It wasn't, but that's a general notion that everyone gets. We need to log stuff in one of our classes, and currently it's all baked directly into that class. It's not. But let's pretend.
I could see what I needed to do: I'll just rip this lot out into another service class, and then use DI to… bugger. CFWheels does not have any notion of dependency injection. I mean... I'm fairly certain it doesn't even use the notion of constructors when creating objects. If one wants to use some code in a CFWheels model, one writes the code in the model. This is pretty much why we got to where we are. If one wants to use code from another class in one's model... one uses the model factory method to get a different model (eg: myOtherModel = model("Other")). Inline in one's code. There's a few drawbacks here:
In CFWheels parlance, "model" means "an ORM representation of a DB row". The hard-coupling between models and a DB table is innate to CFWheels. It didn't seem to occur to the architects of CFWheels that not all domain model objects map to underlying DB storage. One can have a "tableless model", but it still betrays the inappropriate coupling between domain model and storage representation. A logging service is a prime example of this. It is part of the domain model. It does not represent persisted objects. In complex applications, the storage considerations around the logic is just a tier underneath the domain model. It's not baked right into the model. I've just found a telling quote on the CFWheels website:
Model: Just another name for the representation of data, usually a database table.
Secondly, it's still a bit of a fail of separation of concerns / IoC if the "calling code" hard-codes which implementation of a concern it is going to use.
If one is writing testable code, one doesn't want that second-point tight-coupling going on. If I'm testing features of my model, I want to stub out the logger it's using, for example. This is awkward to do if the decision as to which logger implementation we're using is baked-into the code I'm testing.
Anyway, you probably get it. Dependency injection exists for a reason. And this is something CFWheels appears to have overlooked.
0.1 - Baseline container
I have worked through the various bits and pieces I'm going to discuss already, to make sure it all works. But as I write this I am starting out with a bare-bones Lucee container, and a bare-bones MariaDB container (from my lucee_and_mariadb repo. I've also gone ahead and installed TestBox, and my baseline tests are passing:
Yes. I have a test that TestBox is installed and running correctly.
We have a green light, so that's a baseline to start with. I've tagged that in GitHub as 0.1.
0.2 - CFWheels operational
OK, I'll install CFWheels. But first my requirement here is "CFWheels is working". I will take this to mean that it displays its homepage after install, so I can test for that pretty easily:
I'm using TDD for even mundane stuff like this so I don't get ahead of myself, and miss bits I need to do to get things working.
This test fails as one would expect. Installing CFWheels is easy: just box install cfwheels. This installs everything in the public web root which is not great, but it's the way CFWheels works. I've written another series about how to get a CFWheels-driven web app working whilst also putting the code in a sensible place, summarised here: Short version: getting CFWheels working outside the context of a web-browsable directory, but life's too short to do all that horsing around today, so we'll just use the default install pattern. Note: I do not consider this approach to be appropriate for production, but it'll do for this demonstration.
After the CFWheels installation I do still have to fix-up a few things:
It steamrolled my existing Application.cfc, so I had to merge the CFWheels bit with my bit again.
Anything in CFWheels will only work properly if it's called in the context of a CFWheels app, so I need to tweak my test config slightly to accommodate that.
And that CFWheels "context" only works if it's called from index.cfm. So I need to rename my test/runTests.cfm to be index.cfm.
Having done that:
A passing test is all good, but I also made sure the thing did work. By actually looking at it:
I want to mess around with models, so I need to create one. I have a stub DB configured with this app, and it has a table test with a coupla test rows in it. I'll create a CFWheels model that maps to that. CFWheels expects plural table names, but mine's singular so I need a config tweak there. I will test that I can retrieve test records from it.
it("can find test records from the DB", () => {
tests = model("Test").findAll(returnAs="object")
expect(tests).notToBeEmpty()
tests.each((test) => {
expect(test).toBeInstanceOf("models.Test")
expect(test.properties().keyArray().sort("text")).toBe(["id", "value"])
})
})
Good. Next I wanted to check when CFWheels calls my Test class's constructor. Given one needs to use that factory method (eg: model("Test").etc) to do anything relating to model objects / collections . etc, I was not sure whether the constructor comes into play. Why do i care? Because when using dependency injection, one generally passes the dependencies in as constructor arguments. This is not the only way of doing it, but it's the most obvious ("KISS" / "Principle of Least Astonishment") approach. So let's at least check.
it("has its constructor called when it is instantiated by CFWheels", () => {
test = model("Test").new()
expect(test.getFindMe()).toBe("FOUND")
})
Implementation:
public function init() {
variables.findMe = "FOUND"
}
public string function getFindMe() {
return variables.findMe
}
Result:
OK so scratch that idea. CFWheels does not call the model class's constructor. Initially I was annoyed about this as it seems bloody stupid. But then I recalled that when one is using a factory method to create objects, it's not unusual to not use the public constructor to do so. OK fair enough.
I asked around, and (sorry I forget who told me, or where they told me) found out that CFWheels does provide an event hook I can leverage for when an model object is created: model.afterInitialization. I already have my test set up to manage my expectations, so I can just change my implementation:
function config() {
table(name="test")
afterInitialization("setFindMe")
}
public function setFindMe() {
variables.findMe = "FOUND"
}
And that passed this time. Oh I changed the test label from "has its constructor called…" to be "has its afterInitialization handler called…". But the rest of the test stays the same. This is an example of how with TDD we are testing the desired outcome rather than the implementation. It doesn't matter whether the value is set by a constructor or by an event handler: it's the end result of being able to use the value that matters.
At the moment I have found my "way in" to each object as they are created. I reckon from here I can have a DependencyInjectionService that I can call upon from the afterInitialization handler so the model can get the dependencies it needs. This is not exactly "dependency injection", it's more "dependency self-medication", but it should work.
My DI requirements ATM are fairly minimal, but I am not going to reinvent the wheel. I'm gonna use DI/1 to handle the dependencies. I've had a look at it before, and it's straight forward enough, and is solid.
My tests are pretty basic to start with: I just want to know it's installed properly and operations:
it("can be instantiated", () => {
container = new framework.ioc("/services")
expect(container).toBeInstanceOf("framework.ioc")
})
And now to install it: box install fw1
And we have a passing test (NB: I'm not showing you the failures necessarily, but I do always actually not proceed with anything until I see the test failing):
It's not much use unless it loads up some stuff, so I'll test that it can:
it("loads services with dependencies", () => {
container = new framework.ioc("/services")
testService = container.getBean("TestService")
expect(testService.getDependency()).toBeInstanceOf("services.TestDependency")
})
I'm gonna show the failures this time. First up:
This is reasonable because the TestService class isn't there yet, so we'd expect DI/1 to complain. The good news is it's complaining in the way we'd want it to. TestService is simple:
component {
public function init(required TestDependency testDependency) {
variables.dependency = arguments.testDependency
}
public TestDependency function getDependency() {
return variables.dependency
}
}
Now the failure changes:
This is still a good sign: DI/1 is doing what it's supposed to. Well: trying to. And reporting back with exactly what's wrong. Let's put it (and, I imagine: you) out of its misery and give it the code it wants. TestDependency:
component {
}
And now DI/1 has wired everything together properly:
As well as creating a DI/1 instance and pointing it at a directory (well: actually I won't be doing that), I need to hand-crank some dependency creation as they are not just a matter of something DI/1 can autowire. So I'm gonna wrap-up all that in a service too, so the app can just use a DependencyInjectionService, and not need to know what its internal workings are.
To start with, I'll just make sure the wrapper can do the same thing we just did with the raw IoC object from the previous tests:
describe("Tests for DependencyInjectionService", () => {
it("loads the DI/1 IoC container and its configuration", () => {
diService = new DependencyInjectionService()
testService = diService.getBean("DependencyInjectionService")
expect(testService).toBeInstanceOf("services.DependencyInjectionService")
})
})
Instead of testing the TestService here, I decided to use DependencyInjectionService to test it can… load itself
There's a bit more code this time for the implementation, but not much.
import framework.ioc
component {
public function init() {
variables.container = new ioc("")
configureDependencies()
}
private function configureDependencies() {
variables.container.declareBean("DependencyInjectionService", "services.DependencyInjectionService")
}
public function onMissingMethod(required string missingMethodName, required struct missingMethodArguments) {
return variables.container[missingMethodName](argumentCollection=missingMethodArguments)
}
}
It creates an IOC container object, but doesn't scan any directories for autowiring opportunities this time.
It hand-cranks the loading of the DependencyInjectionService object.
It also acts as a decorator for the underlying IOC instance, so calling code just calls getBean (for example) on a DependencyInjectionService instance, and this is passed straight on to the IOC object to do the work.
And we have a passing test:
Now we can call our DI service in our model, and the model can use it to configure its dependencies. First we need to configure the DependencyInjectionService with another bean:
private function configureDependencies() {
variables.container.declareBean("DependencyInjectionService", "services.DependencyInjectionService")
variables.container.declareBean("TestDependency", "services.TestDependency")
}
describe("Tests for TestDependency", () => {
describe("Tests for getMessage method")
it("returns SET_BY_DEPENDENCY", () => {
testDependency = new TestDependency()
expect(testDependency.getMessage()).toBe("SET_BY_DEPENDENCY")
})
})
})
// TestDependency.cfc
component {
public string function getMessage() {
return "SET_BY_DEPENDENCY"
}
}
That's not quite the progression of the code there. I had to create TestDependency first, so I did its test and it first; then wired it into DependencyInjectionService.
Now we need to wire that into the model class. But first a test to show it's worked:
describe("Tests for Test model", () => {
describe("Tests of getMessage method", () => {
it("uses an injected dependency to provide a message", () => {
test = model("Test").new()
expect(test.getMessage()).toBe("SET_BY_DEPENDENCY")
})
})
})
Hopefully that speaks for itself: we're gonna get that getMessage method in Test to call the equivalent method from TestDependency. And to do that, we need to wire an instance of TestDependency into our instance of the Test model. I should have thought of better names for these classes, eh?
// /models/Test.cfc
import services.DependencyInjectionService
import wheels.Model
component extends=Model {
function config() {
table(name="test")
afterInitialization("setFindMe,loadIocContainer")
}
public function setFindMe() {
variables.findMe = "FOUND"
}
public string function getFindMe() {
return variables.findMe
}
private function loadIocContainer() {
variables.diService = new DependencyInjectionService()
setDependencies()
}
private function setDependencies() {
variables.dependency = variables.diService.getBean("TestDependency")
}
public function getMessage() {
return variables.dependency.getMessage()
}
}
That works…
…but it needs some adjustment.
Firstly I want the dependency injection stuff being done for all models, not just this one. So I'm going to shove some of that code up into the Model base class:
// /models/Model.cfc
/**
* This is the parent model file that all your models should extend.
* You can add functions to this file to make them available in all your models.
* Do not delete this file.
*/
import services.DependencyInjectionService
component extends=wheels.Model {
function config() {
afterInitialization("loadIocContainer")
}
private function loadIocContainer() {
variables.diService = new DependencyInjectionService()
setDependencies()
}
private function setDependencies() {
// OVERRIDE IN SUBCLASS
}
}
Now the base model handles the loading of the DependencyInjectionService, and calls a setDependencies method. Its own method does nothing, but if a subclass has an override of it, then that will run instead.
I will quickly tag that lot before I continue. 0.4.
But…
0.5 Dealing with the hard-coded DependencyInjectionService initialisation
The second problem is way more significant. Model is creating and initialising that DependencyInjectionService object every time a model object is created. That's not great. All that stuff only needs to be done once for the life of the application. I need to do that bit onApplicationStart (or whatever approximation of that CFWheels supports), and then I need to somehow expose the resultant object in Model.cfc. A crap way of doing it would be to just stick it in application.dependencyInjectionService and have Model look for that. But that's a bit "global variable" for my liking. I wonder if CFWheels has an object cache that it intrinsically passes around the place, and exposes to its inner workings. I sound vague because I had pre-baked all the code up to where I am now a week or two ago, and it was not until I was writing this article I went "oh well that is shit, I can't be having that stuff in there". And I don't currently know the answer.
Let's take the red-green-refactor route, and at least get the initialisation out of Model, and into the application lifecycle.
…
…
…
Ugh. Looking through the CFWheels codebase is not for the faint-hearted. Unfortunately the "architecture" of CFWheels is such that it's about one million (give or take) individual functions, and no real sense of cohesion to anything other than a set of functions might be in the same .cfm (yes: .cfm file :-| ), which then gets arbitrarily included all over the place. If I dump out the variables scope of my Test model class, it has 291 functions. Sigh.
There's a bunch of functions possibly relating to caching, but there's no Cache class or CacheService or anything like that... there's just some functions that act upon a bunch of application-scoped variable that are not connected in any way other than having the word "cache" in them. I feel like I have fallen back through time to the days of CF4.5. Ah well.
I'll chance my arm creating my DependencyInjectionService object in my onApplicationStart handler, use the $addToCache function to maybe put it into a cache… and then pull it back out in Model. Please hold.
[about an hour passes. It was mostly swearing]
Okey doke, so first things first: obviously there's a new test:
describe("Tests for onApplicationStart", () => {
it("puts an instance of DependencyInjectionService into cache", () => {
diService = $getFromCache("diService")
expect(diService).toBeInstanceOf("services.DependencyInjectionService")
})
})
The implementation for this was annoying. I could not use the onApplicationStart handler in my own Application.cfc because CFWheels steamrolls it with its own one. Rather than using the CFML lifecycle event handlers the way they were intended, and also using inheritance when an application and an application framework might have their own work to do, CFWheels just makes you write its handler methods into your Application.cfc. This sounds ridiculous, but this is what CFWheels does in the application's own Application.cfc. I'm going to follow-up on this stupidity in a separate article, perhaps. But suffice it to say that instead of using my onApplicationStart method, I had to do it the CFWheels way. which is … wait for it… to put the code in events/onapplicationstart.cfm. Yuh. Another .cfm file. Oh well. Anyway, here it is:
<cfscript>
// Place code here that should be executed on the "onApplicationStart" event.
import services.DependencyInjectionService
setDependencyInjectionService()
private void function setDependencyInjectionService() {
diService = new DependencyInjectionService()
$addToCache("diService", diService)
}
</cfscript>
And then in models/Model.cfc I make this adjustment:
private function loadIocContainer() {
variables.diService = new DependencyInjectionService()variables.diService = $getFromCache("diService")
setDependencies()
}
And then…
I consider that a qualified sucessful exercise in "implementing dependency injection in a CFWheels web site". I mean I shouldn't have to hand-crank stuff like this. This article should not need to be written. This is something that any framework still in use in 2022 should do out of the box. But… well… here we are. It's all a wee bit Heath Robinson, but I don't think it's so awful that it's something one oughtn't do.
And now I'm gonna push the code up to github (0.5), press "send" on this, and go pretend none of it ever happened.
although as I type this paragraph, I think I have worked out a better way than what I'm about to show you
Well. In my never-ending quest to provide quantity rather than quality, as soon as I pressed "send" on that, I tried the idea I had. And it worked. An it's so bloody simple.
Go read that other article for full context, but the object of the exercise here is to write CFWheels plug-in implementations so they:
don't pollute every object in the app with all its public functions;
and don't actually require the plugin to be stateless and have only public functions.
I dicked around using closure to solve this. But it's more easily solved by just leveraging… the file system.
I've created another plugin with file structure thus:
// src/plugins/colourPlugin/ColourPlugin.cfc
import cfmlInDocker.plugins.colourPlugin.implementation.*
component {
function init() {
this.version = "2.2"
return this
}
function ColourPlugin() {
return new implementation.ColourPlugin()
}
}
// src/plugins/colourPlugin/implementation/ColourPlugin.cfc
import cfmlInDocker.dao.ColourDao
import cfmlInDocker.models.Colour
component {
function init() {
variables.dao = new ColourDao()
return this
}
public function getColourById(id) {
record = variables.dao.selectColourById(id)
if (record.recordCount) {
return new Colour(record.id, record.en, record.mi)
}
throw(type="ColourNotFoundException", message="Colour not found", detail="Data for Colour with ID #id# not found")
}
}
CFWheels only loads the outer CFC as the plugin, so only finds the one public method, so only pollutes everything with that. That method returns an object that actually implements the plugin. Using it in a controller method looks like this:
I've tested (see test/functional/plugins/ColourPluginTest.cfc) that CFWheels doesn't load everything recursively, and there's no sign of implementation.ColourPlugin's getColourById method in the pollution space:
import testbox.system.BaseSpec
component extends=BaseSpec {
function run() {
describe("Tests for ColourPlugin", () => {
it("does not pollute the application with implementation methods", () => {
expect(application.wheels.dispatch).toHaveKey("ColourPlugin")
expect(application.wheels.dispatch).notToHaveKey("getColourById")
})
})
}
}
This is still more messing around than one ought to have to do in a framework whose job it is to make coding easier, but it's less complicated than the other approaches I took in the previous article. I'd even be happy (-ish) doing CFWheels plugin development this way.
That's perhaps a slightly strange article title. I was trying to be diplomatic, and anyone who's read anything I write here will know: this is not my strong suit. Hopefully it'll make sense by the time I finish.
I recently did that series on mixins in CFML: "CFML: messing around with mixins (part 1)" (and parts 2 & 3). This all stemmed from trying to make head or tail of an architectural choice CFWheels has made, which has resulted in a lot of mixed-in functions polluting everything (view functions in models, controller functions in views, helper functions all over the place), and a necessity to eschew private functions in favour of using a "special code" that when a function is prefixed with $, then despite it being public yer supposed to pretend it's private and not use it. Umm… so be it.
I had a need to look at what CFWheels did with its "plugins" feature the other day, and was unsurprised these have taken the same route as all its other function libraries: they can't really be nicely encapsulated classes, because all their methods need to be public and the objects can have no state because of how CFWheels has been architected. I'm not going to play the game by these particular rules, so I've looked at how I can write a plugin, but still actually implement it using proper coding practices at the same time.
It turns out it's pretty easy, just leveraging a bit of closure. Here's a simple example:
import cfmlInDocker.dao.MyDao
import cfmlInDocker.models.Thing
component {
function init() {
this.version = "2.2"
variables.dao = new MyDao()
return this
}
this.selectThingById = function (id) {
record = variables.dao.selectThingById(id)
if (record.recordCount) {
return new Thing(record.id, record.name)
}
throw(type="ThingNotFoundException", message="Thing not found", detail="Data for Thing with ID #id# not found")
}this.getVersion = function () {
return this.version}this.getSecret = function () {
return variables.getSecret()
}
private function selectThingById(id) {
return variables.dao.selectThingById(id)
}
private function getSecret() {
return "sssshhhh"
}
}
It all comes down to how I'm publicly exposing the private functions via a this-scoped wrapper function expression, and that uses closure to preserve variable references to the context of this object.
I'm not going to dwell on it here because I cover all this in those other three articles (specifically the second one), but the way CFWheels is injecting plugin functions into everything, the context of the functions is not preserved, so references to variables and this refer to where they've been injected into, not the object they started in. Using closure ensures the context is coupled to the function, and this persists when the function reference is injected into some other object.
I have tests for all this in /test/functional/plugins/MyPluginTest.cfc. I won't repeat them here cos all they're doing is verifying the plugin methods do what I expect them to do. Have a look though.
But I'm still not happy about this.
It might be fine in CFWheels's mind that polluting every object with seemingly every function in the entire application, but I'm a bit more conserative when it comes to the single responsibility principle, and I really don't like the fact that my plugin is polluting everything with three unnamespaced functions: selectThingById, getVersion and getSecret. A sensible application of the concept of a plugin would be to inject an object perhaps. And the object then exposes its methods. All crazy and OOP I know. I didn't quite nail this (although as I type this paragraph, I think I have worked out a better way than what I'm about to show you), but I think this is an improvement of sorts:
In this example, just one unnamespaced function is exposed, and that returns a struct that then exposes methods. I'm just borrowing the actual code from MyPlugin for the functionality here. Note that the references to the actual functions are still wrapped in a function expressions so that closure binds the super references to this object's super. Same as with the earlier example.
MyOtherPlugin is the only thing that is exposed from this plugin. After that, its return value behaves like an object. Like, you know, how the rest of us have been writing our CFML code for the last decade and a half.
