DEV Community

MinSoo Kim
MinSoo Kim

Posted on

The comparison operator that returns true when there is nothing to compare

I've been trying to add a "block the checkout if this rule matches" feature to a Shopify app I run, and I nearly shipped something that would have blocked every single cart in a store. Not some carts. All of them. The bug was one line long and it was already in production, doing something perfectly reasonable, and you'd have to squint to see anything wrong with it. Let's go through it, because I think the shape of it is a lot more common than the specific code, and you probably have one of these somewhere too.

Quick context so you can follow. The app has a small rules engine: 15 kinds of conditions (shipping country, cart total, product, customer tag, whether the address looks like a PO box, that sort of thing), written once in TypeScript and shared between the checkout function that runs as wasm and the rule tester inside the app. The two existing targets use those rules to hide a payment method or to rename a delivery option. The new work was supposed to be small: same condition vocabulary, one more output, "block". Easy, right? I thought so.

Where the function runs

So this is the part I didn't know. Shopify's Cart and Checkout Validation Function is not a checkout-only thing. It runs at 3 points in the buyer's journey: CART_INTERACTION, CHECKOUT_INTERACTION and CHECKOUT_COMPLETION.

The existing two targets only ever ran inside checkout, and the worst they could do to a buyer was hide a payment method a bit early or rename a delivery option. So for as long as that engine has been running, a wrong answer on a cart with no country had never cost anyone a sale. But at the cart stage there is no address yet. Nobody has typed one. And now the wrong answer would stop the sale. Obvious in hindsight? Completely. Did I think of it? I did not.

That is the whole bug, really. The rest is just working out which line it will land on.

The line

Our comparison operators don't fail closed. The not_in branch explicitly returns true when the value is missing. This isn't me guessing after the fact; it's what the code says:

// shared/rules-engine/evaluate.ts
case "not_in":
  return a === undefined || !list.includes(a);
Enter fullscreen mode Exit fullscreen mode

Read that with a merchant's rule in mind: "block if the shipping country is not in [US]". On a cart with no address, a is undefined, so the operator says yes, the country is not in the list, then the rule matches, then the cart gets blocked. And at the cart stage every cart looks like that. From the moment a merchant turns that rule on, nobody will be able to get past the cart. What's the logic in that? There isn't any; it's just what the code does when you ask it to answer a question it wasn't built to answer.

I want to be precise about what actually happened, because "blocked every order" is the kind of sentence that gets repeated. This lived on a branch (feat/validation-rules). It never reached a merchant. The live app can't block anything and still has its 15 conditions. But if I had shipped the branch as it stood, that is what it would have gone on to do.

4 of the 15 conditions read the address: country, province, zip and po_box. The branch adds 4 more address-shaped ones (length, whether there's a street number, contains-string, non-Latin characters), so on the branch it's 8 of 19 that depend on something a cart doesn't have yet.

Why the obvious fixes don't work

So my first thought was the obvious one: make not_in return false when the value is missing. Fail closed, done. Right?

But that operator is not mine to change any more. It's shared with the two targets that are already live, and merchants have built their rules on top of what it does today. Flip the operator and I'd be changing the behaviour of software that merchants have configured and rely on, to fix a feature nobody is using yet. I'm not going to do that.

Second thought: use buyerJourney.step and just skip address rules at CART_INTERACTION. That's what the API is giving you the step for, surely? Surely?

It's not enough. The address can also be empty during checkout, before the buyer has got round to filling in the address form. So a rule can be evaluated at CHECKOUT_INTERACTION with the same missing value and the same wrong true. Filtering by step fixes the cart case and then leaves the checkout case exactly as broken as before.

What I did instead

So the engine now refuses to answer instead of guessing, which I'm pretty happy with. Any rule that needs an address is held until the cart actually has one, and the function decides that by looking at the cart, not at which step it is on.

export const ADDRESS_CONDITION_FIELDS: ConditionField[] = [
  "country", "province", "zip", "po_box",
  // …plus the 4 address-shape conditions (length, street number, contains, non-Latin)
];

export function ruleNeedsAddress(rule: Rule): boolean {
  return rule.conditions.some((c) => ADDRESS_CONDITION_FIELDS.includes(c.field));
}
Enter fullscreen mode Exit fullscreen mode
// Function side. Not buyerJourney.step: does the cart have an address or not.
const addressKnown = (input.cart.deliveryGroups ?? []).some(
  (group) => !!group.deliveryAddress?.countryCode,
);
const { errors } = evaluateValidation(config, ctx, { addressKnown });
Enter fullscreen mode Exit fullscreen mode

The rule tester inside the app doesn't take a default. It works out addressKnown from what the merchant actually typed: if they filled in a country, the rule is evaluated; if they left it blank, it shows "waiting for address" rather than silently matching, which is the same thing the checkout does, just made visible to them.

And there are regression tests pinning both directions now: cart stage with no address lets the cart through, checkout stage with an address blocks it. The original 4 (country, province, zip, po_box) never get evaluated on an address-less cart at all, because ruleNeedsAddress holds the whole rule first. The 4 new address-shape conditions have their own test, and one sentence I made it say out loud, because it's the sentence I got wrong: on a cart with no address, all 4 of them answer "no", including the negative forms (not_in, not_set). 173 tests pass on the branch.

The bit that generalises

The same operator is safe when the worst it can do is hide a payment button, and dangerous when it can stop a sale. Nothing about the operator changed between those two sentences. The thing that changed is how the caller reacts to a true. Is the operator wrong, then? I don't think it is. Is it dangerous? Obviously, in the right place.

So "fail open or fail closed?" isn't a property of the comparison. It's a property of the comparison plus whoever is holding the result. An engine that returns true for "I don't know" has a blast radius decided entirely by its callers, and when you reuse it somewhere new you're not just importing the vocabulary of conditions. You're importing the execution context that vocabulary was written in, and here that context was "the worst a wrong answer can do is hide a button". It wasn't written down anywhere. It was just true, until it wasn't.

I don't have a clever rule for catching this in general, sorry. The honest version is: when a shared function gets a new caller, go and read every branch that handles missing input, one by one, and ask yourself what the new caller will do with each answer. Then write that down. It's boring, but it would have been a lot more expensive to learn after launch.

Anyway, the branch is still a branch, and if you've got a shared rules engine of your own, I'd really go and read its missing-value branches this week. It gets the last checks (typegen, wasm build, a dev store run-through) before it goes anywhere near a real merchant, and now I'm slightly more nervous about the other 11 conditions than I was last week, which is probably the correct amount of nervous.

Top comments (0)