Standardize v1-objects data decoding with unified abstraction#41
Standardize v1-objects data decoding with unified abstraction#41
Conversation
- Add getV1ObjectData() abstraction for consistent dual JSON/msgpack decoding - Include exists return value to handle tombstones, deletions, and not-found cases - Update lookupV1User, getAlternateEmailDetails, and all meetings handlers - Remove redundant deletion/tombstone checks from individual functions - Ensure all v1-objects bucket reads use consistent format detection - Preserve existing handler.go message decoding logic as required - Update dependencies with go get -u and go mod tidy Fixes LFXV2-1115 🤖 Generated with GitHub Copilot (via Zed) Signed-off-by: Eric Searcy <eric@linuxfoundation.org>
![github-license-compliance[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
this isn't picking up my force-push, so I'm closing and re-opening |
There was a problem hiding this comment.
Pull request overview
This PR standardizes decoding of records read from the v1-objects NATS KV bucket by introducing a single helper that handles JSON + msgpack decoding and normalizes tombstone/deletion handling, then updates v1 user/email lookups and meeting handlers to use it.
Changes:
- Add
getV1ObjectData()abstraction for dual JSON/msgpack KV reads with unified “exists” semantics (not-found, deleted, tombstoned, soft-deleted). - Update v1 user lookup, alternate email lookup, and meetings handlers to use the unified abstraction and remove duplicated checks.
- Bump Go dependencies (go get -u / go mod tidy), plus minor formatting adjustments.
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/lfx-v1-sync-helper/lfx_v1_client.go |
Adds getV1ObjectData() and migrates user + alternate email lookups to unified KV decoding/deletion handling. |
cmd/lfx-v1-sync-helper/handlers_meetings.go |
Switches meeting-related KV reads to getV1ObjectData() to standardize decoding and deletion semantics. |
cmd/lfx-v1-sync-helper/config.go |
Formatting-only alignment changes in config loading. |
cmd/dynamodb-stream-consumer/publisher.go |
Formatting-only alignment changes for the published event struct fields. |
go.mod |
Dependency version bumps. |
go.sum |
Updated checksums for bumped dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| userData, exists, err := getV1ObjectData(ctx, userKey) | ||
| if err != nil { | ||
| if err == jetstream.ErrKeyNotFound { | ||
| return nil, fmt.Errorf("user %s not found in v1-objects KV bucket", platformID) | ||
| } | ||
| return nil, fmt.Errorf("failed to get user from v1-objects KV bucket: %w", err) | ||
| } | ||
|
|
||
| // Parse the merged_user data | ||
| var userData map[string]any | ||
| if err := json.Unmarshal(entry.Value(), &userData); err != nil { | ||
| // Try msgpack if JSON fails. | ||
| if msgpackErr := msgpack.Unmarshal(entry.Value(), &userData); msgpackErr != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal user data (json: %w, msgpack: %w)", err, msgpackErr) | ||
| } | ||
| return nil, fmt.Errorf("failed to get user data: %w", err) | ||
| } | ||
|
|
||
| // Check if the user record is deleted | ||
| if isDeleted, ok := userData["isdeleted"].(bool); ok && isDeleted { | ||
| return nil, fmt.Errorf("user %s is deleted (isdeleted)", platformID) | ||
| } | ||
|
|
||
| // Also treat WAL-based soft deletes (indicated by _sdc_deleted_at) as deleted. | ||
| if deletedAt, ok := userData["_sdc_deleted_at"]; ok { | ||
| if s, okStr := deletedAt.(string); (okStr && strings.TrimSpace(s) != "") || (!okStr && deletedAt != nil) { | ||
| return nil, fmt.Errorf("user %s is deleted (_sdc_deleted_at))", platformID) | ||
| } | ||
| if !exists { | ||
| return nil, fmt.Errorf("user %s not found or is deleted in v1-objects KV bucket", platformID) | ||
| } |
There was a problem hiding this comment.
The error returned here loses useful context (platformID/userKey and the bucket). Since getV1ObjectData already abstracts the fetch, consider including the key or platformID in the wrapped error so production logs can be tied back to a specific record more easily.
| if msgpackErr := msgpack.Unmarshal(entry.Value(), &emailData); msgpackErr != nil { | ||
| return "", false, false, fmt.Errorf("failed to unmarshal email data (json: %w, msgpack: %w)", err, msgpackErr) | ||
| } | ||
| return "", false, false, fmt.Errorf("failed to get email data: %w", err) |
There was a problem hiding this comment.
This wrapped error is very generic (no email SFID / key). Including emailSfid or emailKey in the error message would make troubleshooting KV/data issues much easier.
| return "", false, false, fmt.Errorf("failed to get email data: %w", err) | |
| return "", false, false, fmt.Errorf("failed to get email data for emailSfid %s (key %s): %w", emailSfid, emailKey, err) |
| if err == jetstream.ErrKeyNotFound || err == jetstream.ErrKeyDeleted { | ||
| return nil, false, nil | ||
| } | ||
| return nil, false, fmt.Errorf("failed to get data from v1-objects KV bucket: %w", err) |
There was a problem hiding this comment.
When KV.Get fails with a non-notfound error, this message doesn’t include the KV key being fetched. Including the key in the returned error would significantly improve debuggability (especially since many call sites only wrap with a generic "failed to get ... data").
| return nil, false, fmt.Errorf("failed to get data from v1-objects KV bucket: %w", err) | |
| return nil, false, fmt.Errorf("failed to get data from v1-objects KV bucket for key %q: %w", key, err) |
| meetingData, exists, err := getV1ObjectData(ctx, meetingKey) | ||
| if err != nil { | ||
| funcLogger.With(errKey, err, "meeting_id", meetingID).WarnContext(ctx, "failed to fetch meeting from KV bucket, cannot trigger re-index") | ||
| funcLogger.With(errKey, err, "meeting_id", meetingID).ErrorContext(ctx, "failed to get meeting data from KV bucket") |
There was a problem hiding this comment.
This failure is handled by logging and returning (i.e., it’s a recoverable condition that just prevents re-index). Logging it at Error level may create noisy alerts; consider using Warn level here to match other recoverable KV/mapping misses in this service.
| funcLogger.With(errKey, err, "meeting_id", meetingID).ErrorContext(ctx, "failed to get meeting data from KV bucket") | |
| funcLogger.With(errKey, err, "meeting_id", meetingID).WarnContext(ctx, "failed to get meeting data from KV bucket") |
| pastMeetingData, exists, err := getV1ObjectData(ctx, pastMeetingKey) | ||
| if err != nil { | ||
| funcLogger.With(errKey, err).WarnContext(ctx, "failed to fetch past meeting from KV bucket, cannot trigger re-index") | ||
| funcLogger.With(errKey, err).ErrorContext(ctx, "failed to get past meeting data from KV bucket") |
There was a problem hiding this comment.
Similar to the meeting mapping handler, this KV read failure is recovered by returning early. Consider logging at Warn instead of Error to avoid treating transient KV issues/out-of-order events as an application error.
| funcLogger.With(errKey, err).ErrorContext(ctx, "failed to get past meeting data from KV bucket") | |
| funcLogger.With(errKey, err).WarnContext(ctx, "failed to get past meeting data from KV bucket") |
1 participant
Fixes LFXV2-1115
🤖 Generated with GitHub Copilot (via Zed)