Skip to content

Commit ac1690a

Browse files
steipetesteipete
authored
refactor: attachment output flow + timezone helpers (openclaw#252)
* refactor(gmail): simplify attachment output + cache flow * refactor(calendar): extract timezone helpers * refactor(gmail): simplify attachment output + cache flow
1 parent eefca47 commit ac1690a

3 files changed

Lines changed: 163 additions & 84 deletions

File tree

‎internal/cmd/calendar_build.go‎

Lines changed: 34 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -25,22 +25,19 @@ func buildEventDateTime(value string, allDay bool) *calendar.EventDateTime {
2525
return edt
2626
}
2727

28-
// extractTimezone attempts to determine a timezone from an RFC3339 datetime string.
29-
// Returns an IANA timezone name if determinable, empty string otherwise.
30-
func extractTimezone(value string) string {
31-
t, err := time.Parse(time.RFC3339, value)
32-
if err != nil {
33-
return ""
28+
func etcGMTForOffsetSeconds(offset int) (string, bool) {
29+
if offset == 0 || offset%3600 != 0 {
30+
return "", false
3431
}
35-
36-
_, offset := t.Zone()
37-
if offset == 0 {
38-
return tzUTC
32+
hours := offset / 3600
33+
if hours > 0 {
34+
// NOTE: IANA "Etc/GMT" names use reversed signs (e.g. +02:00 => Etc/GMT-2).
35+
return fmt.Sprintf("Etc/GMT-%d", hours), true
3936
}
37+
return fmt.Sprintf("Etc/GMT+%d", -hours), true
38+
}
4039

41-
// RFC3339 values have a fixed offset, but Google Calendar requires an IANA timezone
42-
// name for recurring events. We guess by checking which common zones match the
43-
// offset at this instant.
40+
func usIANAForOffsetAt(t time.Time, offset int) string {
4441
switch offset {
4542
case -4 * 3600, -5 * 3600, -6 * 3600, -7 * 3600, -8 * 3600:
4643
for _, candidate := range []string{
@@ -60,15 +57,32 @@ func extractTimezone(value string) string {
6057
}
6158
}
6259
}
60+
return ""
61+
}
62+
63+
// extractTimezone attempts to determine a timezone from an RFC3339 datetime string.
64+
// Returns an IANA timezone name if determinable, empty string otherwise.
65+
func extractTimezone(value string) string {
66+
t, err := time.Parse(time.RFC3339, value)
67+
if err != nil {
68+
return ""
69+
}
70+
71+
_, offset := t.Zone()
72+
if offset == 0 {
73+
return tzUTC
74+
}
75+
76+
// RFC3339 values have a fixed offset, but Google Calendar requires an IANA timezone
77+
// name for recurring events. We guess by checking which common zones match the
78+
// offset at this instant.
79+
if tz := usIANAForOffsetAt(t, offset); tz != "" {
80+
return tz
81+
}
6382

6483
// Fallback for fixed whole-hour offsets when no regional timezone match is found.
65-
// NOTE: IANA "Etc/GMT" names use reversed signs (e.g. +02:00 => Etc/GMT-2).
66-
if offset%3600 == 0 {
67-
hours := offset / 3600
68-
if hours > 0 {
69-
return fmt.Sprintf("Etc/GMT-%d", hours)
70-
}
71-
return fmt.Sprintf("Etc/GMT+%d", -hours)
84+
if tz, ok := etcGMTForOffsetSeconds(offset); ok {
85+
return tz
7286
}
7387
return ""
7488
}

‎internal/cmd/gmail_attachment.go‎

Lines changed: 111 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ func (c *GmailAttachmentCmd) Run(ctx context.Context, flags *RootFlags) error {
4343
return usage("messageId/attachmentId required")
4444
}
4545

46-
destPath, err := resolveAttachmentOutputPath(messageID, attachmentID, c.Output.Path, c.Name, false)
46+
dest, err := resolveAttachmentDest(messageID, attachmentID, c.Output.Path, c.Name, false)
4747
if err != nil {
4848
return err
4949
}
@@ -52,7 +52,7 @@ func (c *GmailAttachmentCmd) Run(ctx context.Context, flags *RootFlags) error {
5252
if dryRunErr := dryRunExit(ctx, flags, "gmail.attachment.download", map[string]any{
5353
"message_id": messageID,
5454
"attachment_id": attachmentID,
55-
"path": destPath,
55+
"path": dest.Path,
5656
}); dryRunErr != nil {
5757
return dryRunErr
5858
}
@@ -67,69 +67,83 @@ func (c *GmailAttachmentCmd) Run(ctx context.Context, flags *RootFlags) error {
6767
return err
6868
}
6969

70-
destPath, err = resolveAttachmentOutputPath(messageID, attachmentID, c.Output.Path, c.Name, true)
70+
dest, err = resolveAttachmentDest(messageID, attachmentID, c.Output.Path, c.Name, true)
7171
if err != nil {
7272
return err
7373
}
74+
if dest.EnsureDefaultDir {
75+
// Ensure the config dir exists (so permissions are correct) before we write under it.
76+
if _, ensureErr := config.EnsureGmailAttachmentsDir(); ensureErr != nil {
77+
return ensureErr
78+
}
79+
}
7480

7581
expectedSize := int64(-1)
76-
if st, statErr := os.Stat(destPath); statErr == nil && st.Mode().IsRegular() {
82+
if st, statErr := os.Stat(dest.Path); statErr == nil && st.Mode().IsRegular() {
7783
// Only hit messages.get when we might have a cache-hit candidate.
7884
expectedSize = lookupAttachmentSizeEstimate(ctx, svc, messageID, attachmentID)
7985
}
80-
path, cached, bytes, err := downloadAttachmentToPath(ctx, svc, messageID, attachmentID, destPath, expectedSize)
86+
path, cached, bytes, err := downloadAttachmentToPath(ctx, svc, messageID, attachmentID, dest.Path, expectedSize)
8187
if err != nil {
8288
return err
8389
}
8490
return printAttachmentDownloadResult(ctx, u, path, cached, bytes)
8591
}
8692

87-
func resolveAttachmentOutputPath(messageID, attachmentID, outPathFlag, name string, ensureDefaultDir bool) (string, error) {
93+
type attachmentDest struct {
94+
Path string
95+
EnsureDefaultDir bool
96+
}
97+
98+
func resolveAttachmentDest(messageID, attachmentID, outPathFlag, name string, allowEnsureDefaultDir bool) (attachmentDest, error) {
8899
shortID := attachmentID
89100
if len(shortID) > 8 {
90101
shortID = shortID[:8]
91102
}
92103
safeFilename := sanitizeAttachmentFilename(name, defaultGmailAttachmentFilename)
93104

94105
if strings.TrimSpace(outPathFlag) == "" {
95-
var dir string
96-
var err error
97-
if ensureDefaultDir {
98-
dir, err = config.EnsureGmailAttachmentsDir()
99-
} else {
100-
dir, err = config.GmailAttachmentsDir()
101-
}
106+
dir, err := config.GmailAttachmentsDir()
102107
if err != nil {
103-
return "", err
108+
return attachmentDest{}, err
104109
}
105-
return filepath.Join(dir, fmt.Sprintf("%s_%s_%s", messageID, shortID, safeFilename)), nil
110+
return attachmentDest{
111+
Path: filepath.Join(dir, fmt.Sprintf("%s_%s_%s", messageID, shortID, safeFilename)),
112+
EnsureDefaultDir: allowEnsureDefaultDir,
113+
}, nil
106114
}
107115

108116
outPath, err := config.ExpandPath(outPathFlag)
109117
if err != nil {
110-
return "", err
118+
return attachmentDest{}, err
111119
}
112120

121+
isDir := isDirIntent(outPathFlag, outPath)
122+
if !isDir {
123+
// file path; keep as-is
124+
return attachmentDest{Path: outPath}, nil
125+
}
126+
127+
filename := safeFilename
128+
if strings.TrimSpace(name) == "" {
129+
filename = fmt.Sprintf("%s_%s_attachment.bin", messageID, shortID)
130+
}
131+
132+
return attachmentDest{Path: filepath.Join(outPath, filename)}, nil
133+
}
134+
135+
func isDirIntent(outPathFlag, expandedOutPath string) bool {
113136
// Directory intent:
114137
// - existing directory path
115138
// - or explicit trailing slash for a (possibly non-existent) directory
116-
isDir := strings.HasSuffix(strings.TrimSpace(outPathFlag), string(os.PathSeparator)) ||
117-
strings.HasSuffix(outPathFlag, "/") ||
118-
strings.HasSuffix(outPathFlag, "\\")
119-
if !isDir {
120-
if st, statErr := os.Stat(outPath); statErr == nil && st.IsDir() {
121-
isDir = true
122-
}
139+
flag := strings.TrimSpace(outPathFlag)
140+
if strings.HasSuffix(flag, string(os.PathSeparator)) || strings.HasSuffix(flag, "/") || strings.HasSuffix(flag, "\\") {
141+
return true
123142
}
124-
if isDir {
125-
filename := safeFilename
126-
if strings.TrimSpace(name) == "" {
127-
filename = fmt.Sprintf("%s_%s_attachment.bin", messageID, shortID)
128-
}
129-
return filepath.Join(outPath, filename), nil
143+
if st, statErr := os.Stat(expandedOutPath); statErr == nil && st.IsDir() {
144+
return true
130145
}
131-
132-
return outPath, nil
146+
return false
133147
}
134148

135149
func sanitizeAttachmentFilename(name, fallback string) string {
@@ -170,40 +184,91 @@ func downloadAttachmentToPath(
170184
return "", false, 0, errors.New("missing outPath")
171185
}
172186

173-
if st, err := os.Stat(outPath); err == nil {
174-
if st.IsDir() {
175-
return "", false, 0, fmt.Errorf("outPath is a directory: %s", outPath)
176-
}
177-
if st.Mode().IsRegular() && expectedSize > 0 && st.Size() == expectedSize {
178-
return outPath, true, st.Size(), nil
187+
cached, cachedSize, err := cachedRegularFile(outPath, expectedSize)
188+
if err != nil {
189+
return "", false, 0, err
190+
}
191+
if cached {
192+
return outPath, true, cachedSize, nil
193+
}
194+
195+
data, err := fetchAttachmentBytes(ctx, svc, messageID, attachmentID)
196+
if err != nil {
197+
return "", false, 0, err
198+
}
199+
if err := writeFileAtomic(outPath, data); err != nil {
200+
return "", false, 0, err
201+
}
202+
return outPath, false, int64(len(data)), nil
203+
}
204+
205+
func cachedRegularFile(outPath string, expectedSize int64) (cached bool, size int64, err error) {
206+
if expectedSize <= 0 {
207+
return false, 0, nil
208+
}
209+
st, statErr := os.Stat(outPath)
210+
if statErr != nil {
211+
if os.IsNotExist(statErr) {
212+
return false, 0, nil
179213
}
214+
return false, 0, statErr
215+
}
216+
if st.IsDir() {
217+
return false, 0, fmt.Errorf("outPath is a directory: %s", outPath)
218+
}
219+
if st.Mode().IsRegular() && st.Size() == expectedSize {
220+
return true, st.Size(), nil
180221
}
222+
return false, 0, nil
223+
}
181224

225+
func fetchAttachmentBytes(ctx context.Context, svc *gmail.Service, messageID, attachmentID string) ([]byte, error) {
182226
if svc == nil {
183-
return "", false, 0, errors.New("missing gmail service")
227+
return nil, errors.New("missing gmail service")
184228
}
185229

186230
body, err := svc.Users.Messages.Attachments.Get("me", messageID, attachmentID).Context(ctx).Do()
187231
if err != nil {
188-
return "", false, 0, err
232+
return nil, err
189233
}
190234
if body == nil || body.Data == "" {
191-
return "", false, 0, errors.New("empty attachment data")
235+
return nil, errors.New("empty attachment data")
192236
}
237+
193238
data, err := base64.RawURLEncoding.DecodeString(body.Data)
194239
if err != nil {
195240
// Gmail can return padded base64url; accept both.
196241
data, err = base64.URLEncoding.DecodeString(body.Data)
197242
if err != nil {
198-
return "", false, 0, err
243+
return nil, err
199244
}
200245
}
246+
return data, nil
247+
}
201248

202-
if err := os.MkdirAll(filepath.Dir(outPath), 0o700); err != nil {
203-
return "", false, 0, err
249+
func writeFileAtomic(outPath string, data []byte) error {
250+
dir := filepath.Dir(outPath)
251+
if err := os.MkdirAll(dir, 0o700); err != nil {
252+
return err
204253
}
205-
if err := os.WriteFile(outPath, data, 0o600); err != nil {
206-
return "", false, 0, err
254+
255+
f, err := os.CreateTemp(dir, ".gog-attachment-*")
256+
if err != nil {
257+
return err
207258
}
208-
return outPath, false, int64(len(data)), nil
259+
tmp := f.Name()
260+
defer func() { _ = os.Remove(tmp) }()
261+
262+
if err := f.Chmod(0o600); err != nil {
263+
_ = f.Close()
264+
return err
265+
}
266+
if _, err := f.Write(data); err != nil {
267+
_ = f.Close()
268+
return err
269+
}
270+
if err := f.Close(); err != nil {
271+
return err
272+
}
273+
return os.Rename(tmp, outPath)
209274
}

‎internal/cmd/gmail_attachment_more_test.go‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -215,70 +215,70 @@ func TestSanitizeAttachmentFilename(t *testing.T) {
215215

216216
func TestResolveAttachmentOutputPath(t *testing.T) {
217217
t.Run("explicit file path", func(t *testing.T) {
218-
path, err := resolveAttachmentOutputPath("m1", "a1", "/tmp/out.bin", "", false)
218+
dest, err := resolveAttachmentDest("m1", "a1", "/tmp/out.bin", "", false)
219219
if err != nil {
220220
t.Fatalf("unexpected error: %v", err)
221221
}
222-
if path != "/tmp/out.bin" {
223-
t.Fatalf("got %q, want /tmp/out.bin", path)
222+
if dest.Path != "/tmp/out.bin" {
223+
t.Fatalf("got %q, want /tmp/out.bin", dest.Path)
224224
}
225225
})
226226

227227
t.Run("directory target appends filename", func(t *testing.T) {
228228
dir := t.TempDir()
229-
path, err := resolveAttachmentOutputPath("m1", "abcdefghij", dir, "", false)
229+
dest, err := resolveAttachmentDest("m1", "abcdefghij", dir, "", false)
230230
if err != nil {
231231
t.Fatalf("unexpected error: %v", err)
232232
}
233233
want := filepath.Join(dir, "m1_abcdefgh_attachment.bin")
234-
if path != want {
235-
t.Fatalf("got %q, want %q", path, want)
234+
if dest.Path != want {
235+
t.Fatalf("got %q, want %q", dest.Path, want)
236236
}
237237
})
238238

239239
t.Run("directory target with custom name", func(t *testing.T) {
240240
dir := t.TempDir()
241-
path, err := resolveAttachmentOutputPath("m1", "abcdefghij", dir, "report.pdf", false)
241+
dest, err := resolveAttachmentDest("m1", "abcdefghij", dir, "report.pdf", false)
242242
if err != nil {
243243
t.Fatalf("unexpected error: %v", err)
244244
}
245245
want := filepath.Join(dir, "report.pdf")
246-
if path != want {
247-
t.Fatalf("got %q, want %q", path, want)
246+
if dest.Path != want {
247+
t.Fatalf("got %q, want %q", dest.Path, want)
248248
}
249249
})
250250

251251
t.Run("traversal in name is stripped", func(t *testing.T) {
252252
dir := t.TempDir()
253-
path, err := resolveAttachmentOutputPath("m1", "abcdefghij", dir, "../../etc/passwd", false)
253+
dest, err := resolveAttachmentDest("m1", "abcdefghij", dir, "../../etc/passwd", false)
254254
if err != nil {
255255
t.Fatalf("unexpected error: %v", err)
256256
}
257257
want := filepath.Join(dir, "passwd")
258-
if path != want {
259-
t.Fatalf("got %q, want %q", path, want)
258+
if dest.Path != want {
259+
t.Fatalf("got %q, want %q", dest.Path, want)
260260
}
261261
})
262262

263263
t.Run("trailing separator treated as directory", func(t *testing.T) {
264-
path, err := resolveAttachmentOutputPath("m1", "abcdefghij", "/tmp/newdir/", "report.pdf", false)
264+
dest, err := resolveAttachmentDest("m1", "abcdefghij", "/tmp/newdir/", "report.pdf", false)
265265
if err != nil {
266266
t.Fatalf("unexpected error: %v", err)
267267
}
268268
want := filepath.Join("/tmp/newdir", "report.pdf")
269-
if path != want {
270-
t.Fatalf("got %q, want %q", path, want)
269+
if dest.Path != want {
270+
t.Fatalf("got %q, want %q", dest.Path, want)
271271
}
272272
})
273273

274274
t.Run("trailing separator with no name uses stable default", func(t *testing.T) {
275-
path, err := resolveAttachmentOutputPath("m1", "abcdefghij", "/tmp/newdir/", "", false)
275+
dest, err := resolveAttachmentDest("m1", "abcdefghij", "/tmp/newdir/", "", false)
276276
if err != nil {
277277
t.Fatalf("unexpected error: %v", err)
278278
}
279279
want := filepath.Join("/tmp/newdir", "m1_abcdefgh_attachment.bin")
280-
if path != want {
281-
t.Fatalf("got %q, want %q", path, want)
280+
if dest.Path != want {
281+
t.Fatalf("got %q, want %q", dest.Path, want)
282282
}
283283
})
284284
}

0 commit comments

Comments
 (0)