An extendable KeyError for s3dol and more

[thorwhalen]

I see the last change that is meant to catch an error in getitem that should be considered a key error.

There's something for that in dol.Store already, but not sure how well it's set up. Should have a look.

About the current local patch...

I’m slightly wary about the fact you are only catching one exception there. Of course, we don’t want to waste time looking at all s3 (boto?) exceptions to see which should be considered key errors. But as it is, (1) you could miss some actual key errors and (2) evolving to accumulate other key errors we’ll observe in the future.

I have a few ideas to propose here.

In s3dol base we define a subclass of KeyError, call it S3KeyError. It has an attribute (the class does) whose value is a set (or list--but not tuple) of all s3 errors that should be considered key errors. It all has a method "register" that will modify that set.

The use should be obvious from there.

Even better would be if we define the S3KeyError to be a KeyError, as well as all the other ones we want. Then you can just catch that one error and also raise it (and since it's also a key error, it will trigger the base behavior.

In fact, perhaps that last solution is better. Only thing I don't know how to do yet. Cleanly is make it extendable. We need to reload the module to redefine reset of registered error classes. But maybe we don't really need a dynamic way to register errors quite yet. It's cleaner if we can because that means that the module whose context races a certain type of error is the one registering it. But for the sake of pragmatism, let's just do the minimum modification that makes it a lot better.

[thorwhalen]

Some inspiration code.

class MyError(KeyError):
    caught_errors = tuple()

    @classmethod
    def register(cls, error):
        if error not in cls.caught_errors:
            cls.caught_errors = cls.caught_errors + (error,)
    

MyError.register(AttributeError)

try:
    list.foo
except MyError.caught_errors as e:
    print(f"caught: {e}")

[thorwhalen]

Worked through this as part of the v1 redesign. Full write-up in misc/docs/decisions/0004-error-taxonomy.md; the short version is yes to the intent, no to the registry mechanism, and the general part does belong in dol.

First — the instinct behind this issue is more right than it looks

I'm slightly wary about the fact you are only catching one exception there.

Measured, against botocore 1.40.53 and moto 5.2.2:

Operation	missing target	Error.Code	HTTP	caught by client.exceptions.NoSuchKey?
GetObject	key	NoSuchKey	404	yes
HeadObject	key	404	404	no
HeadBucket	bucket	404	404	no
DeleteObject	key	—	204	no error at all
DeleteObjects	keys	—	200	reported as Deleted, not an error

The reason is structural: HEAD has no response body, so there's no XML error document for botocore to model, so the modelled exception never fires. Any code that catches NoSuchKey around a head_object silently never matches. This is probably the most common S3 error-handling bug in the wild, and current s3dol has it.

Worse, and closer to the real risk here: _bucket_exists does a bare except ClientError: return False, so an expired token, a missing permission, or a wrong endpoint all report as "the bucket doesn't exist" — and __iter__ wraps its whole listing in that check, so list(store) returns [] instead of raising. A silent wrong answer where an error was available.

So the fix isn't just "catch more exception types" — it's that exception type is the wrong key to classify on.

Why not the class-attribute registry

The proposed S3KeyError.register(...) works mechanically (and yes, except cls.caught_errors re-reads the tuple at raise time, so late registration does take effect). Two objections:

1. It's global mutable state. Any import anywhere can change how every store in the process classifies errors. The effect is import-order-dependent and can't be tested in isolation.
2. It classifies on exception type, which for S3 carries almost no information — see the table. 13 error shapes are modelled in the whole S3 service model; everything else arrives as a generic ClientError, and the discriminating information is in response['Error']['Code'] and the HTTP status.

What replaces it

A translation table keyed on (operation, code, status), carried on the connection's preset:

• Extensible by third parties — same goal as register(), but per-connection, explicit, and inspectable rather than process-global.
• Provider quirks become table rows. Supabase returns 400 where AWS returns 404 on HeadBucket; several providers return 403 to hide 404. No code branch.
• Testable with no network — the classifier takes a synthesized ClientError, so most error tests are pure unit tests.

operation is in the key because of the HEAD ambiguity above: on real AWS, HeadObject against a missing key and against a missing bucket are both ('404', 404), indistinguishable. That can't be resolved by guessing, and moto won't warn you — moto returns an error body on HEAD where AWS returns none, so it yields NoSuchBucket locally and '404' in production. 'k' in store against a typo'd bucket is green in CI and silently False in prod.

And one seam, not a hierarchy: a single @translate_s3_errors decorator is the only place that catches botocore exceptions, so auth-vs-not-found stays auditable from one file. Auth, config and transport errors are never translated to KeyError — that's the rule that fixes _bucket_exists.

On "this may be better served in dol"

Agreed, and the evidence supports it: every *dol adapter has the same "which backend exception means no-such-key" problem, and each has solved it differently or not at all. azuredol has the cleanest version — a single @translate_azure_errors decorator plus an errors.py — and it's essentially this design with one backend.

The dol-level primitive would be a map_errors decorator plus a NotFound convention. It isn't a blocker: s3dol implements it locally in a drop-in shape with a linked issue, then deletes the local copy and raises the dol floor the day the upstream lands.

One trap to avoid while doing it — dol already has two different classes named KeyValidationError:

dol.errors.KeyValidationError    # bases: (NotValid, ValueError, TypeError)
dol.filesys.KeyValidationError   # bases: (KeyError, LookupError)
# same class? False

so s3dol names its own KeyNotValid rather than adding a third, and inherits from both KeyError and ValueError so either except works.

Two ergonomic details that cost real debugging time

KeyError.__str__ uses repr of its args. A carefully written multi-line error message comes out as one line full of \n escapes. Every "informative error" in a package is silently mangled by this unless the KeyError subclasses override __str__. Worth knowing before writing the messages, not after.

403 often means 404. AWS: "If you have s3:ListBucket … 404. If you don't have s3:ListBucket, Amazon S3 returns 403 Forbidden." The canonical least-privilege policy grants s3:GetObject/s3:PutObject on bucket/* and omits s3:ListBucket — so under the most common production IAM policy every miss is a 403, which is not a KeyError, which means store.get(k, default) raises and a cache-lookup service 500s instead of taking the miss branch. Default is to raise AccessDenied naming s3:ListBucket as the likely cause, with deny_means_absent=True as an explicit opt-in. Recording it as a decision rather than leaving it to be discovered.

---

The one place we deliberately let KeyError mean something other than "absent" is ObjectArchived (Glacier): the key does exist, so k in store must stay True, but store[k] has to fail — and as a KeyError, so .get(k, default) degrades. It carries .storage_class, .restore_status and .restore(days, tier). Two honest caveats that the obvious version of that argument gets wrong: dict(store), .items() and __eq__ still raise (only the defaulted accessors degrade), and setdefault would happily overwrite the archived object with the default — so it's overridden to re-raise.