Fix confused config keys

[ulidtko]

Hi again @jappeace 👋

I'm updating a vendor-fork of keter onto version 2; this piece of feedback bubbled up from testing:

psMissingHost <- ... "unknown-host-response-file"
psUnknownHost <- ... "missing-host-response-file"

(in Keter/Proxy.hs)

Far from a huge bug, it doesn't make any difference for us (setting them both to the same .html stub anyway) — yet, I'm guessing it's probably unintentional.

Most interesting commit to review is the 3rd one, 2f89797, let me quote the commit message:

use a speck of typelevel spice to showcase "how not to" fall victim to this class of mistakes again, if desired. The Tagged helper is:

• zero-cost;
• simple;
• easy to use;
• attaches a constant string to a type, and transparently tracks it for as long as you allow it to — e.g. in this case, from KeterConfig data-decl all the way until readFile calls.

Please review & merge.

[ulidtko]

Root cause of the mismatch is here... 👀 wrong order:

            <*> o .:? "missing-host-response-file"
            <*> o .:? "unknown-host-response-file"

this is flipped cf. the fields of KeterConfig:

    , kconfigUnknownHostResponse  :: !(Maybe F.FilePath)
    , kconfigMissingHostResponse  :: !(Maybe F.FilePath)

Tagged allows to avoid this pitfall completely.

[jappeace]

Hi 👋
thanks!

could you fix the bug without introducing a dependency?
optionally write a test to assert correctness (although I'd merge the bugfix by itself)

[ulidtko]

could you fix the bug without introducing a dependency?

tagged is very small, and nearly 0-deps itself (it does pull in a few deps optionally with some older GHC versions and non-default flags; even GHC 7 is supported).

Reimplementing it is a matter of a 1-page module; sure can do... but do you really want this code in keter?

[ulidtko]

optionally write a test to assert correctness

With Tagged, correctness is verified by typechecker; a good enough test won't compile 😃

E.g. you'd get errors like this:

    • Couldn't match type ‘"unknown-host-response-file"’
                     with ‘"missing-host-response-file"’
      Expected type: Tagged "missing-host-response-file" (Maybe FilePath)
        Actual type: Tagged "unknown-host-response-file" (Maybe FilePath)
    • In the ‘kconfigMissingHostResponse’ field of a record
      In the expression:

        ...

[jappeace]

is there a way you can think of implementing this without using symbolVal?

[ulidtko]

is there a way you can think of implementing this without using symbolVal?

no. it's how you get type-reflected values back to value level. why?

[ulidtko]

If you dislike the "typelevel spice" — there's ofcourse much simpler fix: just flip back the constants into correct order. six lines of diff if I'm counting correctly

Just showcasing how in principle, it's possible to do better than that 🤷

[jappeace]

please do the simpel fix, I've seen enough of typelevel spice in haskell to know I don't want any of it in production code, it's fine for art 😀
Agda is a much better language for this anyway.

your change from error to warning is justified I suppose. I wasn't sure what the right behavior was, erroring or warning.

[jappeace]

please add a note to the changelog as well

[ulidtko]

Agda forces you into the "correct by construction" cage; while Haskell allows to put (or not!) just one finger into it 😆

See the commit just pushed — reimplemented the dependency in 2 lines; it's really that simple.

your change from error to warning is justified I suppose

Yes, it'll save deployment roundtrips with, said politely, "messed up" infrastructure, one that keeps hundreds of copies of those configs (per every running keter instance). The fallback is already there, so why not use it, right?

[jappeace]

Please drop the tagged idea. 😅

I understand you put effort into figuring out how to do that,
however, I don't want to introduce any type level shenanigans to Keter because it'll increase the contribution barrier.
One nice aspect of this code base is that it's relatively simple.

[ulidtko]

Okay; another problem which Tagged has helped with, now resurfaces: not only the ordering in instance ParseYamlFile KeterConfig is brittle and just has to be right, but also now the string constants must be repeated, accurately, in makeSettings, here:

keter/src/Keter/Proxy.hs

Lines 102 to 107 in 709bbc3

	makeSettings hostman = do
	KeterConfig{..} <- ask
	psManager <- liftIO $ HTTP.newManager HTTP.tlsManagerSettings
	psMissingHost <- taggedReadFile "unknown-host-response-file" kconfigMissingHostResponse defaultMissingHostBody id
	psUnknownHost <- taggedReadFile "missing-host-response-file" kconfigUnknownHostResponse defaultUnknownHostBody const
	psProxyException <- taggedReadFile "proxy-exception-response-file" kconfigProxyException defaultProxyException id

which obviously is error-prone (another "just has to be right"); also inflates line lengths.

Tagged obviated that by carrying key names implicitly in the type. The keys in logs were guaranteed to match those in FromJSON instance.

@jappeace that looks good to you? I disagree, but doesn't matter, you're the one requesting changes here 🥲 I'm happy to oblige.

[ulidtko]

OMG wait... I've done the same mistake myself 🤣

That's what I'm saying — it's way too easy to mess up!..

I know to trust myself, but humans get tired, distracted, lose concentration, etc... IMO there's no harm in buying a little transparent insurance from a good type-system.

[jappeace]

Thanks for your changes. I'm sorry but I don't think this code changes often enough to justify an abstraction like that. We'll keep it simple, even though it's a little brittle.
If you care a lot about correct config loading I think the best thing to do is write a test (which I'd love to see! This project has far too few tests)

don't forget to update the changelog

[ulidtko]

Done. Changelog opened for 2.1.3

[jappeace]

thanks for your contribution