From b7928637a0751b3d480b377bfdd510f92eb71bf1 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Thu, 7 May 2026 18:54:06 -0700 Subject: [PATCH] refactor(s3api): move Lifecycle XML structs to leaf package lifecycle_xml (#9360) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(s3api): move Lifecycle XML structs to leaf package lifecycle_xml The structs S3 PutBucketLifecycleConfiguration parses and the canonical conversion to s3lifecycle.Rule lived in package s3api, which transitively imports weed/server (which imports weed/shell). Any caller outside weed/s3api — the shell, the future lifecycle worker — that wanted to parse a bucket's lifecycle XML hit an import cycle. Moves: weed/s3api/s3api_policy.go -> lifecycle_xml/types.go weed/s3api/s3api_lifecycle_canonical.go -> lifecycle_xml/canonical.go s3api_lifecycle_canonical_test.go -> lifecycle_xml/canonical_test.go s3api_policy_test.go -> lifecycle_xml/round_trip_test.go Renames the public RuleStatus type (was unexported ruleStatus) and adds small accessor methods (Set/Val/AndSet/TagSet) for fields the s3api handler needs to read across the package boundary. Adds NewPrefix and NewExpirationDays constructors so the GET handler can build response rules without poking at unexported fields. Adds a Tag struct local to the package so it has zero internal seaweed deps. Adds a one-shot ParseCanonical(xml []byte) helper for non-server callers. s3api_policy.go was misnamed — its content is lifecycle XML, not S3 bucket policy. The new package name reflects the actual scope. * test(s3api/lifecycle_xml): exercise public API in tests - canonical_test.go's parseLifecycle helper went through xml.Unmarshal directly; route it through the package's exported Parse so tests validate the public entrypoint. - round_trip_test.go asserted internal flags (rule.Filter.tagSet, rule.Filter.andSet, Transition.set, NoncurrentVersionTransition.set); switch to TagSet(), AndSet(), Set() — exercises the public contract that downstream callers (s3api handler, future shell command) rely on. --- .../canonical.go} | 25 +++++++- .../canonical_test.go} | 15 +++-- .../round_trip_test.go} | 20 +++--- .../types.go} | 61 ++++++++++++++++--- weed/s3api/s3api_bucket_handlers.go | 33 +++++----- 5 files changed, 112 insertions(+), 42 deletions(-) rename weed/s3api/{s3api_lifecycle_canonical.go => lifecycle_xml/canonical.go} (78%) rename weed/s3api/{s3api_lifecycle_canonical_test.go => lifecycle_xml/canonical_test.go} (95%) rename weed/s3api/{s3api_policy_test.go => lifecycle_xml/round_trip_test.go} (94%) rename weed/s3api/{s3api_policy.go => lifecycle_xml/types.go} (81%) diff --git a/weed/s3api/s3api_lifecycle_canonical.go b/weed/s3api/lifecycle_xml/canonical.go similarity index 78% rename from weed/s3api/s3api_lifecycle_canonical.go rename to weed/s3api/lifecycle_xml/canonical.go index adf72fb2b..6c930b790 100644 --- a/weed/s3api/s3api_lifecycle_canonical.go +++ b/weed/s3api/lifecycle_xml/canonical.go @@ -1,9 +1,32 @@ -package s3api +package lifecycle_xml import ( + "bytes" + "encoding/xml" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3lifecycle" ) +// Parse decodes a BucketLifecycleConfiguration XML body into the wire-form +// Lifecycle struct. +func Parse(xmlBytes []byte) (*Lifecycle, error) { + var lc Lifecycle + if err := xml.NewDecoder(bytes.NewReader(xmlBytes)).Decode(&lc); err != nil { + return nil, err + } + return &lc, nil +} + +// ParseCanonical is the one-shot path most non-server callers want: +// raw XML in, []*s3lifecycle.Rule out. +func ParseCanonical(xmlBytes []byte) ([]*s3lifecycle.Rule, error) { + lc, err := Parse(xmlBytes) + if err != nil { + return nil, err + } + return LifecycleToCanonical(lc), nil +} + // LifecycleToCanonical flattens the XML-deserialized Lifecycle into the // engine's flat Rule shape. The optional element may contain // | | , or be absent (in which case the older top-level diff --git a/weed/s3api/s3api_lifecycle_canonical_test.go b/weed/s3api/lifecycle_xml/canonical_test.go similarity index 95% rename from weed/s3api/s3api_lifecycle_canonical_test.go rename to weed/s3api/lifecycle_xml/canonical_test.go index 543f72861..16f4b035c 100644 --- a/weed/s3api/s3api_lifecycle_canonical_test.go +++ b/weed/s3api/lifecycle_xml/canonical_test.go @@ -1,20 +1,19 @@ -package s3api +package lifecycle_xml import ( - "encoding/xml" "reflect" "testing" "time" ) -// parseLifecycle is a thin helper for tests; production code reads XML via the -// regular bucket-config decoder, this shortcut keeps the test focused on the -// canonical conversion. +// parseLifecycle is a thin helper for tests; production code reads XML via +// the regular bucket-config decoder, this shortcut goes through the +// package's public Parse so the tests exercise the exported entrypoint. func parseLifecycle(t *testing.T, xmlSrc string) *Lifecycle { t.Helper() - lc := &Lifecycle{} - if err := xml.Unmarshal([]byte(xmlSrc), lc); err != nil { - t.Fatalf("unmarshal: %v", err) + lc, err := Parse([]byte(xmlSrc)) + if err != nil { + t.Fatalf("parse: %v", err) } return lc } diff --git a/weed/s3api/s3api_policy_test.go b/weed/s3api/lifecycle_xml/round_trip_test.go similarity index 94% rename from weed/s3api/s3api_policy_test.go rename to weed/s3api/lifecycle_xml/round_trip_test.go index f6cc38f60..ca4d05306 100644 --- a/weed/s3api/s3api_policy_test.go +++ b/weed/s3api/lifecycle_xml/round_trip_test.go @@ -1,4 +1,4 @@ -package s3api +package lifecycle_xml import ( "encoding/xml" @@ -101,8 +101,8 @@ func TestLifecycleXMLRoundTrip_FilterWithTag(t *testing.T) { } rule := lc.Rules[0] - if !rule.Filter.tagSet { - t.Error("expected Filter.tagSet to be true") + if !rule.Filter.TagSet() { + t.Error("expected Filter.TagSet() to be true") } if rule.Filter.Tag.Key != "env" || rule.Filter.Tag.Value != "dev" { t.Errorf("expected Tag{env:dev}, got Tag{%s:%s}", rule.Filter.Tag.Key, rule.Filter.Tag.Value) @@ -133,8 +133,8 @@ func TestLifecycleXMLRoundTrip_FilterWithAnd(t *testing.T) { } rule := lc.Rules[0] - if !rule.Filter.andSet { - t.Error("expected Filter.andSet to be true") + if !rule.Filter.AndSet() { + t.Error("expected Filter.AndSet() to be true") } if rule.Filter.And.Prefix.String() != "logs/" { t.Errorf("expected And.Prefix='logs/', got %q", rule.Filter.And.Prefix.String()) @@ -178,7 +178,7 @@ func TestLifecycleXMLRoundTrip_FilterWithSizeOnly(t *testing.T) { } func TestLifecycleXML_TransitionSetFlag(t *testing.T) { - // Verify that Transition.set is true after unmarshaling. + // Verify that Transition.Set() is true after unmarshaling. input := ` transition @@ -195,8 +195,8 @@ func TestLifecycleXML_TransitionSetFlag(t *testing.T) { if err := xml.Unmarshal([]byte(input), &lc); err != nil { t.Fatalf("unmarshal: %v", err) } - if !lc.Rules[0].Transition.set { - t.Error("expected Transition.set=true after unmarshal") + if !lc.Rules[0].Transition.Set() { + t.Error("expected Transition.Set()=true after unmarshal") } } @@ -217,8 +217,8 @@ func TestLifecycleXML_NoncurrentVersionTransitionSetFlag(t *testing.T) { if err := xml.Unmarshal([]byte(input), &lc); err != nil { t.Fatalf("unmarshal: %v", err) } - if !lc.Rules[0].NoncurrentVersionTransition.set { - t.Error("expected NoncurrentVersionTransition.set=true after unmarshal") + if !lc.Rules[0].NoncurrentVersionTransition.Set() { + t.Error("expected NoncurrentVersionTransition.Set()=true after unmarshal") } } diff --git a/weed/s3api/s3api_policy.go b/weed/s3api/lifecycle_xml/types.go similarity index 81% rename from weed/s3api/s3api_policy.go rename to weed/s3api/lifecycle_xml/types.go index cb715cba9..35fd8fb4e 100644 --- a/weed/s3api/s3api_policy.go +++ b/weed/s3api/lifecycle_xml/types.go @@ -1,19 +1,36 @@ -package s3api +// Package lifecycle_xml is the XML wire-form for S3 BucketLifecycleConfiguration: +// the structs S3 PutBucketLifecycleConfiguration accepts and +// GetBucketLifecycleConfiguration returns. It lives outside weed/s3api so +// callers outside that package (shell, lifecycle worker) can parse and +// emit the same shape without pulling weed/s3api's full dependency graph +// — weed/s3api transitively imports weed/server, which imports weed/shell. +// +// Conversion to the engine-friendly canonical form +// (weed/s3api/s3lifecycle.Rule) lives next to the types here as +// LifecycleToCanonical. +package lifecycle_xml import ( "encoding/xml" "time" ) -// Status represents lifecycle configuration status -type ruleStatus string +// RuleStatus represents lifecycle rule status. +type RuleStatus string -// Supported status types +// Supported status types. const ( - Enabled ruleStatus = "Enabled" - Disabled ruleStatus = "Disabled" + Enabled RuleStatus = "Enabled" + Disabled RuleStatus = "Disabled" ) +// Tag is the lifecycle filter tag pair. Defined locally so this package +// has no dependency on weed/s3api. +type Tag struct { + Key string `xml:"Key"` + Value string `xml:"Value"` +} + // Lifecycle - Configuration for bucket lifecycle. type Lifecycle struct { XMLName xml.Name `xml:"LifecycleConfiguration"` @@ -24,7 +41,7 @@ type Lifecycle struct { type Rule struct { XMLName xml.Name `xml:"Rule"` ID string `xml:"ID,omitempty"` - Status ruleStatus `xml:"Status"` + Status RuleStatus `xml:"Status"` Filter Filter `xml:"Filter,omitempty"` Prefix Prefix `xml:"Prefix,omitempty"` Expiration Expiration `xml:"Expiration,omitempty"` @@ -63,6 +80,36 @@ func (p Prefix) String() string { return p.val } +// Set returns whether the Prefix carries a value (vs being absent). +func (p Prefix) Set() bool { return p.set } + +// Val returns the prefix string. Empty when !Set. +func (p Prefix) Val() string { return p.val } + +// NewPrefix builds a Prefix marked as set. +func NewPrefix(val string) Prefix { return Prefix{val: val, set: true} } + +// NewExpirationDays builds an Expiration with Days set. +func NewExpirationDays(days int) Expiration { return Expiration{Days: days, set: true} } + +// Set reports whether this Filter is present in the XML. +func (f Filter) Set() bool { return f.set } + +// AndSet reports whether the Filter contains an branch. +func (f Filter) AndSet() bool { return f.andSet } + +// TagSet reports whether the Filter contains a single branch. +func (f Filter) TagSet() bool { return f.tagSet } + +// Set reports whether the Transition element was present. +func (t Transition) Set() bool { return t.set } + +// Set reports whether the NoncurrentVersionTransition element was present. +func (n NoncurrentVersionTransition) Set() bool { return n.set } + +// Set reports whether the Expiration element was present. +func (e Expiration) Set() bool { return e.set } + // MarshalXML encodes Prefix field into an XML form. func (p Prefix) MarshalXML(e *xml.Encoder, startElement xml.StartElement) error { if !p.set { diff --git a/weed/s3api/s3api_bucket_handlers.go b/weed/s3api/s3api_bucket_handlers.go index bbabb0db5..301b385a8 100644 --- a/weed/s3api/s3api_bucket_handlers.go +++ b/weed/s3api/s3api_bucket_handlers.go @@ -20,6 +20,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/s3api/s3bucket" "github.com/seaweedfs/seaweedfs/weed/filer" + "github.com/seaweedfs/seaweedfs/weed/s3api/lifecycle_xml" "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" stats_collect "github.com/seaweedfs/seaweedfs/weed/stats" "github.com/seaweedfs/seaweedfs/weed/storage/needle" @@ -840,7 +841,7 @@ func (s3a *S3ApiServer) GetBucketLifecycleConfigurationHandler(w http.ResponseWr return } - response := Lifecycle{} + response := lifecycle_xml.Lifecycle{} // Sort locationPrefixes to ensure consistent ordering of lifecycle rules var locationPrefixes []string for locationPrefix := range ttls { @@ -859,11 +860,11 @@ func (s3a *S3ApiServer) GetBucketLifecycleConfigurationHandler(w http.ResponseWr if !found { continue } - response.Rules = append(response.Rules, Rule{ + response.Rules = append(response.Rules, lifecycle_xml.Rule{ ID: prefix, - Status: Enabled, - Prefix: Prefix{val: prefix, set: true}, - Expiration: Expiration{Days: days, set: true}, + Status: lifecycle_xml.Enabled, + Prefix: lifecycle_xml.NewPrefix(prefix), + Expiration: lifecycle_xml.NewExpirationDays(days), }) } @@ -920,7 +921,7 @@ func (s3a *S3ApiServer) PutBucketLifecycleConfigurationHandler(w http.ResponseWr return } - lifeCycleConfig := Lifecycle{} + lifeCycleConfig := lifecycle_xml.Lifecycle{} if err := xmlDecoder(bytes.NewReader(lifecycleXML), &lifeCycleConfig, int64(len(lifecycleXML))); err != nil { glog.Warningf("PutBucketLifecycleConfigurationHandler xml decode: %s", err) s3err.WriteErrorResponse(w, r, s3err.ErrMalformedXML) @@ -977,12 +978,12 @@ func (s3a *S3ApiServer) PutBucketLifecycleConfigurationHandler(w http.ResponseWr bucketVersioning == s3_constants.VersioningSuspended for _, rule := range lifeCycleConfig.Rules { - if rule.Status != Enabled { + if rule.Status != lifecycle_xml.Enabled { continue } // Reject Transition rules — they require storage class migration // infrastructure that does not exist yet. - if rule.Transition.set || rule.NoncurrentVersionTransition.set { + if rule.Transition.Set() || rule.NoncurrentVersionTransition.Set() { s3err.WriteErrorResponse(w, r, s3err.ErrNotImplemented) return } @@ -993,12 +994,12 @@ func (s3a *S3ApiServer) PutBucketLifecycleConfigurationHandler(w http.ResponseWr var rulePrefix string switch { - case rule.Filter.andSet: - rulePrefix = rule.Filter.And.Prefix.val - case rule.Filter.Prefix.set: - rulePrefix = rule.Filter.Prefix.val - case rule.Prefix.set: - rulePrefix = rule.Prefix.val + case rule.Filter.AndSet(): + rulePrefix = rule.Filter.And.Prefix.Val() + case rule.Filter.Prefix.Set(): + rulePrefix = rule.Filter.Prefix.Val() + case rule.Prefix.Set(): + rulePrefix = rule.Prefix.Val() } // Only create filer.conf TTL entries for simple Expiration.Days rules @@ -1009,9 +1010,9 @@ func (s3a *S3ApiServer) PutBucketLifecycleConfigurationHandler(w http.ResponseWr if rule.Expiration.Days == 0 { continue } - hasTagOrSizeFilter := rule.Filter.tagSet || + hasTagOrSizeFilter := rule.Filter.TagSet() || rule.Filter.ObjectSizeGreaterThan > 0 || rule.Filter.ObjectSizeLessThan > 0 || - (rule.Filter.andSet && (len(rule.Filter.And.Tags) > 0 || + (rule.Filter.AndSet() && (len(rule.Filter.And.Tags) > 0 || rule.Filter.And.ObjectSizeGreaterThan > 0 || rule.Filter.And.ObjectSizeLessThan > 0)) if hasTagOrSizeFilter { continue // evaluated by lifecycle worker at scan time