feat(s3): implement object tagging (VersionState second tenant) - #81
Merged
Merged
Conversation
frrist
requested review from
alanshaw and
hannahhoward
and removed request for
hannahhoward
August 13, 2026 18:15
hannahhoward
approved these changes
Aug 13, 2026
| // mapCommitError); key existence outranks lock-enabled, which outranks the | ||
| // versionId grammar, matching posix and the pinning conformance cases. | ||
| func (b *Backend) mutateVersionState(ctx context.Context, bucketName, key, versionID string, mutate func(*msbucket.VersionState)) error { | ||
| func (b *Backend) mutateVersionState(ctx context.Context, bucketName, key, versionID string, requireLock bool, mutate func(*msbucket.VersionState)) error { |
Contributor
There was a problem hiding this comment.
might be worth two convenience functions mutateVersionLock and mutateVersionTags that set the boolean automatically, just cause it'd be more clear.
Member
Author
There was a problem hiding this comment.
Done in c0fd7c5, using your names. The read-side helper had the same bare boolean, so gave it the same treatment ( resolveLockTarget / resolveTagTarget ). No call site passes a raw true/false anymore.
| // withTags merges a creation-time tag set into a version's initial state, | ||
| // allocating the state when only tags are present (docs/s3-object-tagging.md | ||
| // §4). Returns vs unchanged when there are no tags. | ||
| func withTags(vs *msbucket.VersionState, tags map[string]string) *msbucket.VersionState { |
Contributor
There was a problem hiding this comment.
I may be being pedantic, but I don't like withTags as a name for this function. Usually I'd consider a "with x" function to return a value without modifying the input. This one modifys vs is tags are present and its not nil. My suggestion, more wordy, is "applyTagsIfPresent" -- the actual behavior is catpured in the function name then.
Member
Author
There was a problem hiding this comment.
renamed to applyTagsIfPresent in c0fd7c5 and updated the code comment
frrist
added a commit
that referenced
this pull request
Aug 14, 2026
…Present Per review on #81: the require-lock boolean disappears from call sites behind four thin wrappers — mutateVersionLock / mutateVersionTags and resolveLockTarget / resolveTagTarget — so each operation family sets its own gate and a bare true/false never appears at a call site. withTags is renamed applyTagsIfPresent: the old name implied a non-mutating derive (the Go WithX convention) while the function mutates its argument in place; the new name and its comment state the mutation outright. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implements docs/s3-object-tagging.md. Tags live in the per-key version-state tree the object-lock design built: the three per-version methods run the shared check order with no bucket gate (the state-target and state-write helpers gain a require-lock flag), the merge rule carries lock fields across tag writes and tags across lock writes, and DeleteObjectTagging exercises the empty-block elision rule for real. A version without tags answers the empty set, never a sentinel. Creation-time stamping covers the x-amz-tagging header on PutObject (parsed via backend.ParseObjectTags before ingest, so an invalid header uploads nothing), the copy tagging directive (COPY inherits the resolved source version's tags, REPLACE takes the request header), and the multipart carry (the raw header validated at create, stored on the session, stamped at Complete). GET/HEAD gain the x-amz-tagging-count echo from the same state-block fetch as the lock headers. Tags on unversioned buckets make the write-rule discard cleanup live: a null-replacing PUT prunes the old version's tag entry in the same commit. itest gains the three tagging categories and the eight Versioning_* tagging rows, and promotes PutObject_tagging, CreateMultipartUpload_with_tagging, CopyObject_should_copy_tagging, CopyObject_should_replace_tagging, GetObject_success, and HeadObject_success (the last two XFailed on the TagCount echo). DeleteObjectTagging_expected_bucket_owner is XFail: the hilt flow substitutes the root access key as the ACL owner, so the tenant's correct-owner request 403s — the existing *_expected_owner surface. Per review, the require-lock boolean never appears at a call site: the named wrappers mutateVersionLock / mutateVersionTags and resolveLockTarget / resolveTagTarget set the gate for their operation family, and applyTagsIfPresent (previously withTags) states its in-place mutation outright. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
frrist
force-pushed
the
frrist/feat/object-tagging
branch
from
August 14, 2026 00:56
c0fd7c5 to
62258fc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements docs/s3-object-tagging.md — stacked on the design PR. Full
TestForgeVersityconformance partition green against the smelt stack.s3frontend/objecttag.go):Get/Put/DeleteObjectTaggingover the shared per-version state machinery — the state-target resolution and state-write helpers gain a require-lock flag (lock passes true, tagging false), so nothing is duplicated. Untagged versions answer the empty set; markers 405; the versionId grammar is the versioning design's.DeleteObjectTaggingexercises empty-block elision (the block is removed, the emptied tree drops off the leaf).x-amz-taggingon PutObject parsed viabackend.ParseObjectTagsbefore ingest (an invalid header uploads nothing); copy directives (COPY inherits the resolved source version's tags, REPLACE takes the header); multipart carries the raw header on the session (validated at create, migration00012) and stamps at Complete. All stamping rides the same commit that installs the version.x-amz-tagging-countfrom the same single state-block fetch as the lock headers (stateHeaderFields).Conformance: three new categories (
PutObjectTagging,GetObjectTagging,DeleteObjectTagging), the eightVersioning_*tagging rows, and promotions forPutObject_tagging,CreateMultipartUpload_with_tagging,CopyObject_should_copy_tagging,CopyObject_should_replace_tagging,GetObject_success, andHeadObject_success(the last two had XFailed on the missing TagCount echo). One new XFail with its reason recorded:DeleteObjectTagging_expected_bucket_owner— the hilt flow substitutes the root access key as ACL owner, so the tenant's correct-owner request 403s (the existing*_expected_ownersurface, not a tagging defect).🤖 Generated with Claude Code