CFML: pseudo-constructor polymorphic inheritance expectations management

G'day:

Well there's a sequence of words I never expected to write down.

I just ran across something that I was reasonably surprised by when I first saw it. But having looked at it some more, I'm not sure. So I thought I'd ask the 3-4 people who actually read this blog to offer their insight.

Consider this traditional example of polymorphism in play:

// Base.cfc
component {

    function runMe(){
        doThings()
    }

    function doThings(){
        writeOutput("BaseApp doThings called")
    }
}

// Sub.cfc
component extends=Base {

    public function doThings(){
        super.doThings()
        writeOutput("SubApp doThings called")
    }
}

// from StandardInheritanceTest.cfc
it("A subclass will override a base class method", () => {
    o = new Sub()
    o.runMe()

    expect(o.stack).toBe([
        "Base doThings called",
        "Sub doThings called"
    ])
})

The test (which passes) confirms what we'd expect: when runMe calls doThings, Because it's being called on a Sub object, the reference to doThings is referring to Sub.doThings even though the call is in Base. Hopefully no surprises there.

But what about this example:

// Base.cfc
component {

    this.stack = []

    doThings()

    function doThings(){
        this.stack.append("Base doThings called")
    }
}

// Sub.cfc
component extends=Base {

    function doThings(){
        super.doThings()
        this.stack.append("Sub doThings called")
    }
}

// from PseudoConstructorInheritanceTest.cfc
it("A subclass will override a base class method", () => {
    o = new Sub()

    expect(o.stack).toBe([
        "Base doThings called",
        "Sub doThings called"
    ])
})

The difference here is the call to doThings is not done by the test, it's done within the pseudo-constructor of the Base component.

And in this case the test fails:

Expected [[Base doThings called, Sub doThings called]] but received [[Base doThings called]]

It would seem the pseudo-constructor code of a base-class is not aware it's being called from a sub-class. This doesn't seem right to me?

I'm running this code on Lucee, but I ran equivalent code on ColdFusion and the results were the same (so I guess that's something). And given the behaviour is the same on both I'm thinking this is more me not understanding something, rather than a bug. What do you think?

BTW whilst testing this I found out I can get the behaviour I actually want with a slight tweak to Sub.cfc. I changed from this:

component extends=Base {

    function doThings(){
        super.doThings()
        this.stack.append("Sub doThings called")
    }
}

To this:

component extends=Base {

    doThings()

    function doThings(){
        this.stack.append"Sub doThings called")
    }
}

IE: I replicate the way doThings is called in Base.cfc: from the pseudo-constructor. Both calls are made. For my purposes this will work fine. But I still do find it curious.

Or am I being daft?

Righto.

--
Adam

How TDD and automated testing helped me solve an Nginx config problem I had created for myself

G'day:

I have a "website" I'm building on as part of a series of articles I'm writing about Lucee / CFWheels / Docker. I have a Docker container running Nginx which proxies requests for CFML code to a Docker container running Lucee.

Due to the nature of web applications, I have my web-accessible assets - JS, CSS, image and "entry point" index.cfm and Application.cfc files - in a public directory off my app root; and adjacent to that I have a src directory for my code, and vendor directory for third-party code (like stuff I install from ForgeBox via CommandBox).

This /public abstraction needs to be hidden from the end user: they're going to want to be browsing to - for example - http://example.com/, not http://example.com/public. So this means the proxy from Nginx to Lucee also needs to deal with that. It is imperative that no CFML files are publicly exposed other than the aforementioned index.cfm and Application.cfc

I'm terrible at configuring Nginx, and mak a lot of mistakes. Some mistakes are obvious because Nginx flat-out refuses to start. Others are less obvious, and bleed out as "unexpected behaviour" later in the piece. Knowing this, I approached the exercise in a TDD fashion; identifying what cases need addressing and writing tests for them, and then doing the config work to make each pass. Note: my wording is ambiguous there: I did not write more than one test at a time. I wrote a test, then got the config to make that test pass. Then I wrote the next test and reconfigured to make that pass (whilst also keeping the earlier ones passing too). I've already detailed some of this in my earleir article "Adding TestBox, some tests and CFConfig into my Lucee container".

As a result of these efforts to get Nginx working how I expected it to, I ended up with these green tests:

(Ignore how I'm skipping some of these tests. This is down to a bug in CFWheels that doesn't report 404 situations with an actual 404 status code when in dev mode. This does demonstrate though how I identified a shortcoming in behaviour whilst TDDing the work, that said).

And that's great. At every step as I was configuring more and more of Nginx's handling of requests, I could see that any change I made had a) addressed the new requirement; b) not broken any previous requirement. And it was indeed the reality at times that something I tried fixed one issue, but broke something else.

Back to those tests I'm skipping. This showed to me that I was short some cases: CFWheels handles things differently from how I expect it to in some situations, so I figured I had better have two flavours of test: one set for proxying to non-CFWheel-handled URLs; another set for when the URLs are within CFWheels domain. And I'm glad I did make this call, because it showed-up a bug in my Nginx config:

And when I checked those URLs, I saw the problem:

expectedParamValue = "expectedValue"
// ... rest of test not relevant here
expect(response.fileContent).toInclude(
    "Expected query param value: [#expectedParamValue#]",
    "Query parameter value was incorrect (URL: #testUrl#)"
)

But what I was seeing at that URL was this:

Expected query param value: [expectedValue?testParam=expectedValue]

The query string part (including the ?) was being appended to the URL sent to Lucee twice (same problem for both those failing tests). I was pretty puzzled how my previous non-Wheels test was passing, and it still seemed legit. Bemusing. However I was actively appending the query string in my proxy_pass URL, so that was "clearly wrong":

proxy_pass  http://cfml-in-docker.lucee:8888/public$fastcgi_script_name$is_args$args;

I got rid of that, and figured "I had better go and check why those other tests are passing after I re-run the tests here, to check that change:

Dammit. Now the CFWheels-specific tests are passing, but the non-Wheels ones are failing. Time for me to RTFM, cos I'm clearly doing something wrong here.

The first thing I'm going wrong is here:

proxy_pass  http://cfml-in-docker.lucee:8888/public$fastcgi_script_name;

$fastcgi_script_name is a PHP thing, and whilst it coincidentally holds the URI I want, it's the wrong thing to have here. So I put $request_uri back in there.

Right and that broke all the path_info CFWheels tests, so wasn't right. I decided to read more closely. It turns out that request_uri is the whole original URI (including the path_info and query string), and thus is ignoring my rewrites, and it was the reason that the query params were getting doubled up. In my rewrite I had this:

location @rewrite {
    rewrite ^/(.*)? /index.cfm$request_uri last;
    rewrite ^ /index.cfm last;
    return 404;
}

I just wanted $uri, which is just the document URI part of the requested URI, and it also reflects any changes made to that URI during rewrites and what-have-you. So once I used that in my rewrite and for my proxy_pass URL, the tests now look better:

I've abbreviated how long it took me to work this out, and how many cycles of trial end error it too me. Having those automated tests in place were gold because after each iteration I knew how wrong I was - for all cases - in half a second. I didn't need to manually go "OK, did it work for this?" "did it break this other one?" etc, for 20-odd tests.

It was also a big help to me to take the TDD path here, and stop and think & reason about exactly what my expectations ought to be for each of the cases I had. It also lead me to add more cases, such as the combinations of "it has both path_info and query parameters", as well as realising the path through the Nginx config was different for URLs aimed at Wheels (which are completely rewritten), and the ones directly to files in the public directory. I could easily cover both cases by duplicating the tests and changing the URLs sligthly.

Things seem to be working now, but if I find something else wrong, I will first work out what my expectations of it to be right are, and write a quick test for it. Then I'll fix it (without breaking anything else).

For now though: I'm fed-up with Nginx & CFML & CFWheel and I'm gonna do something els for a while. But I'll be back to it later this afternoon: I'm wll behind wehre I want to be with this stuff, and using the bank-holiday weekend to catch up a bit.

The "final" state of my Nginx site config is (docker/nginx/sites/default.conf):

server {
    listen 80;
    listen [::]:80;

    #rewrite_log on;

    server_name cfml-in-docker.frontend;
    root /usr/share/nginx/html;
    index index.html index.cfm;

    resolver 127.0.0.11;

    location / {
        try_files $uri $uri/ @rewrite;
    }

    location @rewrite {
        rewrite ^/(.*)? /index.cfm$uri last;
        rewrite ^ /index.cfm last;
        return 404;
    }

    location ~ \.(?:cfm|cfc)\b {
        proxy_http_version  1.1;
        proxy_set_header    Connection "";
        proxy_set_header    Host                $host;
        proxy_set_header    X-Forwarded-Host    $host;
        proxy_set_header    X-Forwarded-Server  $host;
        proxy_set_header    X-Forwarded-For     $proxy_add_x_forwarded_for;     ## CGI.REMOTE_ADDR
        proxy_set_header    X-Forwarded-Proto   $scheme;                        ## CGI.SERVER_PORT_SECURE
        proxy_set_header    X-Real-IP           $remote_addr;
        expires             epoch;

        proxy_pass  http://cfml-in-docker.lucee:8888/public$uri$is_args$args;
    }

    location ~ /\.ht {
        deny all;
    }
}

Righto, where's me shooting game?

--
Adam

Definitions of ~

G'day

I'm just building on some thoughts here. Some of the thoughts are from learning from people who know a lot more than me, and from watching my teams through their successes and… "less than successes".

When we work, whether we realise it or not, the intent is to deliver value to some client. The client could be an external user who we would like to hand money over to us as a result of our efforts. Or the client might be the boss and their latest revenue-making idea. Or the client might be our own application that is need of some love because the codebase is not exactly as good as we'd like it to be. But there is always a client, and the end result of our work needs to benefit the client. Sometimes it's hard to get a handle on what the client wants, and what exactly we need to do to solve their problems and to add value.

But all our work needs to add value. And that value need to be able to be both measured, and to be realised.

To do this, when we set-out to do & deliver some work, we need a coupla benchmark points along the way. Most notably before we even agree to start, a "Definition of Ready"; and before we agree it's finished: a "Definition of Done".

My notes below are from an engineer's perspective, and there's no-doubt some nuance I'm missing, or the points are specific to situations I've been in. It's not really a generic template, but a theoretical example of what might go into these documents. But to get our work done, I figure we ought to be considering some super/sub -set of these kind of things. These points represent a statement of obvious, and an aide-mémoire for stuff that might be less obvious.

Terminology

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC-2119. I like including this, because it makes sure these words that might be read as being fairly loaded are just used as a reference vocab. I'm not looking at you when I say this.

Definition of Ready

• A story should represent effort that SHOULD fit in one sprint (ie: from "Ready" to "Done" in one sprint).
• Within the above constraint, some estimation of complexity, risk and effort SHOULD be indicated.
• A Behaviour Driven Development (BDD) phraseology SHOULD be used for User Stories. This is so we actively and deliberately make clear each case that need to be addressed to fulfil the requirement, and to reduce ambiguity.
• Inbound and outbound parameters (eg for a new web page or API end point) incl. route slug, query string params, request method, POST body, headers, cookies, anything else) MUST be clearly defined, where the definition MUST include any validation constraints (eg: "[some date] must be in the past").
• Where relevant, outbound status codes SHOULD also be detailed (200-OK, 201-CREATED, 400-BAD-REQUEST etc). For non-2xx responses, error information (or lack thereof when there are TMI concerns) SHOULD be defined if appropriate.
• Logging requirements of non-2xx statuses SHOULD be defined. EG: 404s probably nothing. Some 400s: maybe? 401 & 403: probably? 5xx: definitely.
• Interactions with external systems - eg DBs - SHOULD be considered.
• If the story represents partial work, consideration MUST be made as to how it will be deployed in an "unavailable state" to external uses (eg: what feature toggles are in place, and the criteria for toggle on/off).
• Similarly, consideration SHOULD be made as to how the partial work can be internally QAed (eg: feature toggle rules include the environment, so that features behave "live" in a QA environment).
• For stories that are investigative rather than development-oriented, BDD phraseology SHOULD still be used.
• For investigative stories, there MUST still be a deliverable ("we have a clear idea how to integrate Varnish on the front end to remove load from the application servers on seldom-changing content"). The deliverable MUST be tangible (eg: written down and filed), not just "oh just google some stuff and get an idea how it works".
• For stories related to bugs: steps to reproduce, expected behaviour and actual behaviour, any error messages and log entries SHOULD be included in the ticket detail, if possible.
• For stories relating to UI changes, an example of how it will look SHOULD be included on the ticket. This MAY NOT be pixel-perfect, just indicative.
• Engineering MUST have an idea of how any legacy code changes will have automated testing. This is because we accept legacy code has not been written with testing in mind, but we still MUST test our work.

One thing to note here is that all this stuff requires collaboration from the client and the squad. Some stuff is business need, some stuff is technical fulfilment. For example one would not expect the client to know about DB rollback minutiae, but it's our job to know that it might need to be done, and how to do it. We are their technical advocates and advisors. And gatekeepers. And backstops. And there might be a toner cartridge needing to be changed. Shrug. You know the deal. But it's an important deal.

But anyway… if some work meets a "Definition of Ready", then a squad MAY consider it to be added into the workflow for an upcoming release.

For some work to be considered completed, then we need to consider if it's been "done".

Definition of Done

• There SHOULD be development guidelines (whatever they are) and they MUST be followed.
• There SHOULD be automated code quality checks, and they MUST pass.
• All new code SHOULD have automated tests, and test coverage MUST pass.
• Code relating to other processes such as DB updates and rollbacks SHOULD be in source control and/or readily to hand.
• In feature-toggled situations, both toggle-on and toggle-off states MUST be tested.
• Work MUST have been QAed and actively signed-off as good-to-go.
• Work MUST be actively accepted by the client (whoever the client is, and that could well be a colleague).
• Work that is under feature toggle MAY be considered "done" even if toggled off, provided it's passed QA in the "off" state.
• Work MUST be in production to be considered "done".

The bottom line here is particularly relevant for devs: you've not finished your work until it's live. It's not done when you've finished typing. It's not done when it's in code review. It's not done when it's in QA. It's not done when yer bored of it and would rather be doing something else. It's only done when the client sees the work and accepts it. You are on-the-hook until then, and it's your responsibility until then. You should be actively chasing whoever is necessary to get your work into production and earning value, because if it's not: there's no point in having done it.

For the squad as a whole, every person has a part in seeing the squad's work seeing the light of day. Make sure you're doing what you can to expedite work getting in front of the client and to help them either sign it off, or kick it back for revision.

There will be more factors on both sides of this table, measuring whether work ought to be done, or is done. My point here is more that it needs to be a series of active checks along the way. We don't undertake work if we can't see the light at the end of the tunnel, and we don't actually know it will benefit our client. And we need to think about this all the way.

Let me know what other considerations there might be in this. I'm very web-app-centric in my programming / business / leadership (such as it is) exposure, and there's probably more - of fewer! - things to think about.

Righto.

--
Adam

Change in look...

G'day:

I see my change in look has been leaked to the public:

I hope Adam Tuttle doesn't mind me copying his work.

This was taken from Top 30 Coldfusion RSS Feeds. I will touch base with them and ask them "WTF?"

Righto.

--
Adam

On code review

G'day:

I'm pretty big on code review; I see it as a critical part of the process of developing solution for our clients, and making our own and our colleagues' lives easier. It's a useful communications exercise, and it's a useful technical exercise.

I've used a variation of these notes with a variety of teams in the past, but I've never - until recently - got around to writing a decent (semi-) generic document about it. I've polished things a bit here, and thought I'd get it down in a public-facing fashion. There are references in here to systems I am familiar working with like Bitbucket and Jira and PHP. But I think the guidance carries across, irrespective of what combination of tooling one uses; if not in the precise mechanics, then in the processes the mechanics facilitate.

The CSS for the <h4> through <h6> is not as good as it could be. It's annoying that Blogspot gets me down as far as an <h3> even before we get to the title of the article (the date gets the <h2> for some reason), but: so be it. Sorry about that (you'll see what I mean as you get further down).

Introduction

Before code can be considered ready for release, the dev team needs to give the work a once-over or a "second set of eyes" to make sure it meets code quality guidelines, is appropriately tested, and that it fulfils the requirements of the user story that the code addresses, using the domain design / architecture / approach originally agreed before coding started.

What code review is

It is part of the collaborative team approach to delivering value to the client, just like the initial planning stage is before any code is even written, and any pair-programming that takes place during the initial development cycle. Given a lot of the physical coding work is done in separately from the rest of the team, code review is a "re-union" exercise where the devs get back together to see how their solution has ended up. It's important to remember that any given code is our code, not an individual developer's code.

Code review gives the coding dev a stage to show how they've implemented the solution, draw attention to anything noteworthy or needs flagging for any other reason to the rest of the team. And it gives the reviewers a chance to check things out, and potentially spot anything in the work that might need addressing still, or tweaking, or done slightly differently maybe, or whatever.

Another thing code review is is a learning exercise for everyone concerned. Some nuance of the code, or a suggestion to amend the code could be new knowledge to one or all people on the review. Or it might just clarify how some less-often-trod part of the codebase works. And if nothing else, it'll add to everyone's familiarity with the code in question, and it might be you working on it next. Win.

What code review is not

I have experienced some code reviews that have taken an adversarial approach: pitting the coding developer against the reviewing developers, with the reviewers going "you got this wrong", "this doesn't work", "nup", etc, and trying to seem more clever than their colleagues, with arguments going back and forth in the review comments. This is not what code review is about.

It's also not about people checking up on each other. Nor is it about more senior people pronouncing to less senior people.

We are on the same team, working on the same stories, with the same end-goal in mind.

Where the disconnect happens

Still. If I've done some work and someone else adds a comment saying "this could be done differently because [reasons]", it's easy to get a wee bit defensive / protective of our work (for a moment you're claiming the team's work as your own because it happened to be you who typed it in). This is pretty normal. I mean when I'm doing my own hobby-work and something goes unexpected wrong, I'm usually pretty quick to blame the language or the framework or some library, or something other than just myself for getting something slightly wrong, and forgetting that it's OK to be wrong. I like to think that the blame-casting phase of this is a pretty short cycle these days, and I'm quickly back to going "OK, so what do I need to do to sort this out?". Code review is the same.

I have a good quote here:

An amateur is defensive. The professional finds learning (and even, occasionally, being shown up) to be enjoyable; they like being challenged and humbled, and engage in education as an ongoing and endless process.

Ryan Holiday, Ego is the Enemy

Anyway, bottom line: code review is participants on the same team working together.

Code review mindset

A pull request is a way to communicate application changes to the team. The comments that are added to the code in the pull request have good intentions with the intent of improving our work, and the application. Everyone should be open for a discussion like that. However because written communication can lack some nuance of a spoken in-person conversation, remember to over-compensate with the text and make sure it reads as polite, constructive, and respectful. I've found that sometimes even being too terse in a comment ("needs a test") can be seen as being abrupt, rather than just being to-the-point. Also remember to address the code, not the person. If something's not clear: perhaps ask about it rather than telling about. There's no harm in asking for clarification in something. Code is a communications mechanism - it's designed to articulate concepts between people - so if something needs clarification, this could indeed be something that needs to be addressed in the code. Or you might just've not clocked what the code was doing initially and just need a quick pointer from the dev, before going "duh, of course".

By the same token it's important for the recipient of a comment to frame things the same way. If you read a comment as being adversarial or confrontational, you are probably just be using the wrong internal voice to read it. Read it again with a neutral or collaborative voice. It's hardly ever the case that the comment writer was committing a personal attack to writing, and in a code review comment. It's more likely the person chose the wrong wording or didn't take time to polish-up the comment before pressing send. Devs are notoriously bad at articulating themselves in writing, after all, right?

Comments

If you're reviewing and making a comment, read it back to yourself before pressing submit. Would you like to be on the receiving end of it?

If you're replying to a comment, act likewise.

Before adding a reply to a reply (… to a reply… to a reply…), remember it's a code review not a discussion thread. Consider getting on a call and actually talking to the person. This can really quickly solve the misunderstanding, but it is a really good way of defusing any tensions that could possibly be building.

That said, if some call to action or resolution arose from the voice-conversation, make sure to follow up in the code review with a summary comment. That information is important.

Compromise

Some back-and-forth discussions are really useful and interesting! Some aren't. Either way, they're probably not what you all ought to be doing right at that moment. Give some thought as to whether a) the discussion is actually moving us closer to getting the work approved; b) needs to happen right then and there; c) via a text-based discussion. Possibly just go "yeah actually let's just run with it" (whether "it" is how the code already is, or if it's the fix being suggested). Or possibly go "OK, but let's consider revisiting this in another story later… it might be worth it".

Code review logistics

• All work going into the codebase must be reviewed and approved by at least one other person (not the dev who wrote the code!). Bitbucket can be configured to disabled the Merge button until there are n approvals.
• If a review yields any rework, this should be undertaken before any other new work (ie: a different new story) is commenced. One should focus on completing existing story before starting the next story. "Stop starting and start finishing".
• From a reviewer's perspective: reviewing our colleagues existing work ought to generally take priority over continuing to develop our own new code. At any given moment work that is closer to be completed should be the focus, over starting or continuing newer work. This is not to say that one must drop everything when a code review comes in. What it does mean is that when re-commencing from a break (arriving at work, after or lunch or some other intermediary break), and before refocusing on new code: check what code review needs to be done, and deal with that first.
• That said, it's important to minimise task-switching, so be pragmatic about this. But bear in mind that it is also part of our job to work as a team as well as work as an individual. This is not a situatin of "my work" and "their work": it doesn't matter happened to type-in the code, it is all our work.

Handling code review tasks

Tasks are a feature in BitBucket that allows a code review comment to have a "task" attached to it. This indicates work that needs to be done before the code review can be completed.

(Inadvertently an example of a comment that is probably a bit abrupt-sounding!)

Bitbucket can be configured to disable the Merge button whilst there are outstanding tasks.

As a reviewer, if the nature of an observation you make seems to require action, add a task to the comment. This signifies that you're not just making the observation, you mean it needs action.

As the developer, when addressing a task, mark it "done" in one of two situations:

• whatever was requested has been done;
• after further discussion, it's concluded there's no action to take. But add a comment to that effect in reply to the task comment.

Do not mark a task done unless one of those two resolutions have been made. IE: don't simply decide to not address a task because [reasons]. Get agreement from the reviewer as to the legitimacy of those reasons first.

Code review guidelines

Before the code gets there

• The branch name, commit comments (and they should all have comments!), and pull request should be prefixed with the ticket number for the story the work is for. This is just to make it easier to tie everything together later. The important one of these is the pull request name should have the ticket reference in it so the reviewers can check what they're reviewing. In the past I've used a rule that the pull request title should be "JRA-1234 The Title Of The Ticket" (ie: exactly the title of the ticket). Hopefully Bitbucket has integrations with the ticketing system to it can automatically like through to the ticket just from the ID being there.
• If there's no ticket, the work should not have been even started, let alone submitted for code review.
• The ticket needs to have a story in it. The reviewers need to be able to to know what the work is setting out to do, otherwise how can they know it's fulfilled all its cases? For that matter, how did the dev who did the work?
• Consider squashing your commits when doing a pull request (or configure Bitbucket to do it automatically). By the time the code is going into master, it's no longer useful to log the sequence the work was done it. And we still have the commit log in your own repo anyhow.

Before the review starts

• There ought to be an automated build that ran code quality checks and unit tests. Hopefully there's integration between the CI tool and Bitbucket so the pull request automatically indicates a green status, or the dev should include a link to the green status report.
• If that's not green: don't review anything. However the dev possibly did not notice the build is broken, so give them a (gentle) nudge.
• As the developer, go through the pull request and put your own comments in, where necessary. Sometimes the code can legitimately not be obvious, or it just helps to contextualise which part of the story the code is addressing.

What to look for in a review

General

• Does the work fulfil all of the requirements of the story on the ticket. This might seem obvious, but sometimes a dev can forget something. Or perhaps their reading of the requirement is different from yours, so what's been delivered might not meet actual expectations.
• Does the work go off-piste anywhere? It should only address the requirement of the ticket. Not "oh, whilst I was in this file I saw something I could fix in this other bit of code". That can be done later on another ticket. Or: not at all. So check that the work has maintained focus.
• As a reviewer do you understand the code. This is vital. Remember code is a tool for communicating between developers, and it's not doing its job if you don't understand it. Code can be as fancy as you like, but if no-one can understand it, it should get flagged in code review.
• All code needs to adhere to any prescribed guidelines:
  • In-house guidelines (whatever they are, but all teams should have them).
  • Probably by implication, external community-recognised guidelines such as PHP's PSR-12. People who are not currently in the team will need to understand this code later on, so let's write code that newcomers have more of a chance of understanding.

Specific

• Are all the test cases in place and implemented?
• Some code-sizing boundaries in coding guidelines docs won't literally be checked by the automated quality tests, it's up to the reviewer to think about it. A method will be rejected by the automated tests if it's - for example - over 100 lines long. But if the target is 20, warning bells should be going off at 40. If it's 80 lines long, this probably warrants a closer look. The size restrictions are not there to be gamed (101 bad; 99 good), they're just something we can get code analysis tooling (eg: PHPMD) to flag up. It's down to the humans to agree whether the method is A-OK at 80 lines.
• Equally if new work takes a method that is 50 lines long and takes it up to 60 lines… it was already too big so the dev ought to have extracted the piece of logic they needed to work on into a separate method, and done the work there. Or if they new work definitely belongs in the mainline of the method, then extract a different section of the method (so employing the boy scout rule). There is seldom any good reason for a method that is already oversized to get even bigger.
• Do class / method / variable names make sense? Are they spelt right? Yeah, it matters.
• Are classes / methods doing just one thing?
  • A controller should not be getting stuff from the database or implementing business logic.
  • A method called getAccount should be getting the account. Not creating it if it's missing and then returning it.
  • A repository class should not be implementing logging, etc.
  • A class that hits the DB and also sets constants for other classes to use is doing two things. One or other of them does not belong in that class.
  • Any method that takes a Boolean parameter is almost certainly doing two things: one if the flag is true; another if it's false. This is definitely a warning sign.
  • Look at the levels of indentation. Every level suggests that the method is doing too much.

The value of code review

• The codebase will be better.
• You might have learned some new coding techniques. You have improved as a developer.
• You know more about the codebase, which reduces the bus factor for the company.
• You help your colleagues stay on track with their work.
• You get to practise your written communication skills.
• We get a better historical record of why work was done.
• Team cohesion. It's a team-building exercise.
• Reading code. It's good practise reading other people's code.
• Writing code. It's good practise writing code other people can read.

---

Let me know what you think about code review. Any considerations I've missed?

Righto.

--
Adam

Misc changes to environment for my ongoing Docker / Lucee / CFWheels series

G'day

This will be a bit of a scrappy article just summarising some changes to my project environment since the last article in this series on Lucee / CFWheels / Docker; "Adding TestBox, some tests and CFConfig into my Lucee container". By the end of that article I'd got Nginx proxying calls to Lucee, and some tests to verify its integrity and my expectations of how it ought to be working. I'm about to continue with an article about getting CFWheels to work (URL TBC), but before that - and for the ake of full disclosure - I'll detail these wee changes I've made.

It can connect Lucee to a MariaDB database and fetch records

The test summarises the aim here. /test/integration/TestDatabaseConnection.cfc:

component extends=testbox.system.BaseSpec {

    function run() {
        describe("Tests we can connect to the database", () => {
            it("can retrieve test records", () => {
                expectedRecords = queryNew("id,value", "int,varchar", [
                    [101, "Test row 1"],
                    [102, "Test row 2"]
                ])

                actualRecords = queryExecute("SELECT id, value FROM test ORDER BY id")
                
                expect(actualRecords).toBe(expectedRecords)
            })
        })
    }
}

Note that this filed under test/integration because it's testing the integration between Lucee and the DB, rather than any business logic.

I've aded some config to the test suite's Application.cfc too:

component {

    this.mappings = {
        "/cfmlInDocker/test" = expandPath("/test"),
        "/testbox" = expandPath("/vendor/testbox")
    }

    this.localmode = "modern"

    this.datasources["cfmlInDocker"] = {
        type = "mysql",
        host = "database.backend",
        port = 3306,
        database = "cfmlindocker",
        username = "cfmlindocker",
        password = server.system.environment.MYSQL_PASSWORD,
        custom = {
            useUnicode = true,
            characterEncoding = "UTF-8"
        }
    }
    this.datasource = "cfmlInDocker"
}

One key thing to note here is that I am setting this.localmode in here. Previous I was setting this in Lucee's global config via CFConfig, but Zac Spitzer dropped me a line and pointed out it could be set at runtime in Application.cfc. This is a much more elegant approach, so I'm running with it.

Other than that I'm setting a data source. Note I'm picking up the password from the environment, not hard-coding it. This is passed by the docker-compose.yml file:

lucee:
    build:
        context: ./lucee
        args:
            - LUCEE_PASSWORD=${LUCEE_PASSWORD}
    environment:
        - MYSQL_PASSWORD=${MYSQL_PASSWORD}

For the implementation of this requirement I've added a Docker container for MariaDB, added a test table into it and tested that Lucee can read data from it. This was all straight forward. Here are the file changes:

/docker/mariadb/Dockerfile:

FROM mariadb:latest

COPY ./docker-entrypoint-initdb.d/ /docker-entrypoint-initdb.d/
COPY ./conf/logging.cnf /etc/mysql/conf.d/logging.cnf
RUN chmod -R 644 /etc/mysql/conf.d/logging.cnf

CMD ["mysqld"]

EXPOSE 3306

Nothing mysterious there. I'm using the entrypoint to create the DB table and populate it (docker-entrypoint-initdb.d/1.createAndPopulateTestTable.sql):

USE cfmlindocker;

CREATE TABLE test (
    id INT NOT NULL,
    value VARCHAR(50) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL,

    PRIMARY KEY (id)
) ENGINE=InnoDB;

INSERT INTO test (id, value)
VALUES
    (101, 'Test row 1'),
    (102, 'Test row 2')
;

ALTER TABLE test MODIFY COLUMN id INT auto_increment;

I'm also moving logging to a different directory so I can see them on my host machine (via conf/logging.cnf):

[mysqld]
log_error = /var/log/mariadb/error.log

This is all wired-together in docker-compose.yml

mariadb:
    build:
        context: ./mariadb
    environment:
        - MYSQL_ROOT_PASSWORD=${DATABASE_ROOT_PASSWORD}
        - MYSQL_DATABASE=${MYSQL_DATABASE}
        - MYSQL_USER=${MYSQL_USER}
        - MYSQL_PASSWORD=${MYSQL_PASSWORD}
    ports:
        - "3306:3306"
    volumes:
        - mysqlData:/var/lib/mariadb
        - ./mariadb/root_home:/root
        - ../var/log:/var/log
    stdin_open: true
    tty: true
    networks:
        backend:
            aliases:
                - database.backend

volumes:
    mysqlData:

Note that I am sticking the DB data into a Docker volume instead of in a volume from my host machine. This means I need to take some care if I ever get around to adding non-test data into it, but for the time being it saves cluttering up my host machine with DB files, plus it's easier during initial configuration to completely reset the DB. It's easy enough to change later on when I need to.

I'm setting some of those magic environment variable in .env:

COMPOSE_PROJECT_NAME=cfml-in-docker
MYSQL_DATABASE=cfmlindocker
MYSQL_USER=cfmlindocker

# the following are to be provided to `docker-compose up`
LUCEE_PASSWORD=
DATABASE_ROOT_PASSWORD=
MYSQL_PASSWORD=

And the passwords when I build the containers:

adam@DESKTOP-QV1A45U:/mnt/c/src/cfml-in-docker/docker$ DATABASE_ROOT_PASSWORD=123 MYSQL_PASSWORD=1234 LUCEE_PASSWORD=12345 docker-compose up --build --detach --force-recreate

It got rid of CFConfig

Both the Lucee settings I needed to change with CFConfig before hand can be done natively with Lucee, so I didn't need CFConfig any more. I might need it again later, in which case I will re-install it. But for now it's dead-weight.

RUN box install commandbox-cfconfig
RUN box cfconfig set localScopeMode=modern to=/opt/lucee/web
RUN box cfconfig set adminPassword=${LUCEE_PASSWORD} to=/opt/lucee/web
RUN echo ${LUCEE_PASSWORD} > /opt/lucee/server/lucee-server/context/password.txt # this handles the passwords for both server and web admins

It can run the tests from the shell

Running TestBox's tests in a browser is all very pretty, but not very practical. Fortunately I read the TestBox docs some more and found out how to run them from the shell. They show how to run it from within CommandBox's own special shell here in "TestBox integration › Test runner", but that's weird and no help to me. However I finally twigged that it seems that whatever one might do within the special shell, one can also call from the normal shell via the box command. All I needed to do to enable this was to tell CommandBox how to run the tests in docker/lucee/box.json, which is used by CommandBox in the docker/lucee/Dockerfile:

{
    "devDependencies":{
        "testbox":"^4.2.1+400"
    },
    "installPaths":{
        "testbox":"vendor/testbox/"
    },
    "testbox":{
        "runner":"http://localhost:8888/test/runTests.cfm"
    }
}

COPY ./box.json /var/www/box.json
RUN mkdir -p /var/www/vendor
RUN box install

This has the benefit that the test run doesn't simply return a 200-OK all the time whether tests all passed or not; it exits with a 1 if there's any test failures. So it's usable in a CI/CD situation.

It resolves the slowness with CommandBox

In the previous article I observed that running stuff with CommandBox seemed to have about a 45sec overhead for any action. I tracked this down to the fact that I have my /root/home directory as a volume from my host machine so my various shell histories persist across container rebuilds. And I then realised that CommandBox dumps a whole lot of shite in that directory which it needs to load every time it runs. Because of the shenanigans Docker needs to do when bridging from its file system across to WSL across to the native Windows file systems, these operations are S-L-O-W. OK for a few files. Not OK for stacks of them.

Fortunately CommandBox can be configured to put its temp files elsewhere, so I have configured it to put them in /var/temp instead. As they regenerate if they are missing, this seems like the best place for them. It also prevents clutter leaking out of my container and onto my host machine. This is done via a commandbox.properties file:

commandbox_home=/var/tmp/commandbox

Which I copy into place in the Dockerfile. CommandBox picks it up automatically when I place it there:

COPY ./commandbox.properties /usr/local/bin/commandbox.properties

Good stuff. Now it only takes about 5sec for box to start doing anything, which is fine.

It no longer has the problem with path_info

I covered the shortfall in how Lucee handles path_info in "Repro for Lucee weirdness". I've managed to work around this. Kind of. In a way that solves the problem for this project anyhow.

Well I guess really it is just "learning to live with it". I've done some other experimentation with CFWheels, and all it uses path_info for is indeed to implement semi-user-friendly URLs tacked on to index.cfm. It has no need for any other .cfm file to use its path_info, so the default mappings are actually fine as they are.

However it occurred to me when I was configuring Nginx to do its part of the user-friendly URLs that all requests coming into Lucee from the web server will land in /public, so I could just put in a servlet mapping for the index.cfm in that directory (from web.xml):

<servlet-mapping>
    <servlet-name>CFMLServlet</servlet-name>
    <url-pattern>*.cfm</url-pattern>
    <url-pattern>*.cfml</url-pattern>
    <url-pattern>*.cfc</url-pattern>
    <url-pattern>/index.cfm/*</url-pattern>
    <url-pattern>/index.cfc/*</url-pattern>
    <url-pattern>/index.cfml/*</url-pattern>

    <url-pattern>/public/index.cfm/*</url-pattern>
</servlet-mapping>

One might think that instead of using <url-pattern>/public/index.cfm/*</url-pattern>, I might be able to just specify a match for the entire directory, like this: <url-pattern>/public/*</url-pattern>. From a POV of Tomcat's expectations this ought to be good enough, but from Lucee's perspective it doesn't see that as a "anything in that directory", it's expecting that pattern to be a file that matches a CFML file, so when I tried that I just got an error along the lines of "/public is a directory". Ah well. FWIW, ColdFusion said pretty much the same thing.

One downside to this is that I cannot work out how to add a servlet mapping just for this Lucee application, so I need to replace the entire Tomcat web.xml file, with another one with just one additional line (the original file is 4655 lines long). This is less than ideal, and I've followed it up on the Lucee Slack channel. I just copy the file over in the Dockerfile:

COPY ./root_home/.bashrc /root/.bashrc
COPY ./root_home/.vimrc /root/.vimrc
COPY ./web.xml /usr/local/tomcat/conf/web.xml

I had to rename my test file to index.cfm (so this means the test will need to just go once I install CFWheels which needs that file), but for now I was able to test the change:

it("passes URL path_info to Lucee correctly", () => {
    testPathInfo = "/additional/path/info/"

    http url="http://cfml-in-docker.frontend/index.cfm#testPathInfo#" result="response";

    expect(response.status_code).toBe(200, "HTTP status code incorrect")
    expect(response.fileContent.trim()).toBe(testPathInfo, "PATH_INFO value was incorrect")
})

<cfoutput>#CGI.path_info#</cfoutput>

Oh! And the Nginx changes! docker/nginx/sites/default.conf:

location / {
    try_files $uri $uri/ =404;
    try_files $uri $uri/ @rewrite;
}

location @rewrite {
    rewrite ^/(.*)? /index.cfm$request_uri last;
    rewrite ^ /index.cfm last;
}

(Thanks to the ColdBox docs for those)

It no longer needs PHP to test things

I'm happy that TestBox is working well enough now that I don't need to test things with PHPUnit, and that's all the PHP container was for, so I've removed all that stuff.

---

That's it. In the next article I shall continue from here, and get CFWheels set up in a waythat doesn't require the entire application being a) messed in with my own code; b) in a web browsable directory. Stay tuned…

Righto.

--
Adam

Heads-up: Disqus is being flaky ATM

G'day:

A quick note to advise that I am not getting email notifications of all comments people are making to my blog articles ATM. This is really annoying, and I think makes me seem a bit disrespectful in that it looks like I'm ignoring stuff. I really do value everyone's efforts to read and comment on the shite I push out here. If it seems like I'm ignoring you, ping me on Twitter @adam_cameron. I've downgraded my interactions on that platform, but I still to pay attn to it. If it's CFML-related, I'm also back on the CFML Slack channel.

Righto.

--
Adam