NO-JIRA | fix: source InfraImage cannot be updated#1042
NO-JIRA | fix: source InfraImage cannot be updated#1042tupyy wants to merge 1 commit intokubev2v:mainfrom
Conversation
This commit fixes the bug when source InfraImage cannot be updated once it has been set. Signed-off-by: Cosmin Tupangiu <cosmin@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe pull request refactors the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This is on-hold until we decide together with UI how to move forward. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/mappers/inbound.go (1)
122-159:⚠️ Potential issue | 🔴 CriticalCritical:
ImageTokenKeywill be zeroed on every update, breaking download URL validation.The refactored
ToImageInfracreates a freshmodel.ImageInfrawithout preserving the existingImageTokenKey. Since theUpdateoperation would use GORM'sSave()(full row overwrite), this will setImageTokenKeyto an empty string in the database on every source update.The
ImageTokenKeyis the HMAC signing key generated duringCreateSourceand used byimage.GenerateDownloadURLByToken()to create JWT tokens for image downloads. Zeroing it will break all existing download URLs and prevent generating valid new ones.Either:
- Pass the existing
ImageInfrato preserve its values (the original approach), or- Fetch the existing
ImageTokenKeyand include it in the new struct, or- Change
Updateto use targeted column updates instead ofSave()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/mappers/inbound.go` around lines 122 - 159, The ToImageInfra method currently builds a new model.ImageInfra and omits ImageTokenKey, which will be zeroed when the row is saved; update ToImageInfra (or its caller) to preserve the existing ImageTokenKey by either: (a) change ToImageInfra signature to accept the existing model.ImageInfra (or the existing ImageTokenKey string) and copy ImageTokenKey into the returned struct, or (b) before constructing the new model.ImageInfra fetch the current ImageInfra (or its ImageTokenKey) and set imageInfra.ImageTokenKey = existing.ImageTokenKey; ensure this change is applied where SourceUpdateForm.ToImageInfra is used so that Update/Save does not overwrite ImageTokenKey generated during CreateSource or used by image.GenerateDownloadURLByToken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/source.go`:
- Around line 150-154: The update currently overwrites and zeroes existing
ImageInfra fields (including ImageTokenKey) because form.ToImageInfra(source.ID)
creates a new struct; instead preserve existing values by merging with the
fetched source.ImageInfra before calling s.store.ImageInfra().Update. Fix by
either mutating the existing source.ImageInfra (update fields on
source.ImageInfra then pass it to s.store.ImageInfra().Update) or change
SourceUpdateForm.ToImageInfra to accept the existing model.ImageInfra (e.g.,
ToImageInfra(existing model.ImageInfra) model.ImageInfra) and call it with
source.ImageInfra so only intended fields are overwritten while ImageTokenKey
and other existing values remain intact when invoking
s.store.ImageInfra().Update.
---
Outside diff comments:
In `@internal/service/mappers/inbound.go`:
- Around line 122-159: The ToImageInfra method currently builds a new
model.ImageInfra and omits ImageTokenKey, which will be zeroed when the row is
saved; update ToImageInfra (or its caller) to preserve the existing
ImageTokenKey by either: (a) change ToImageInfra signature to accept the
existing model.ImageInfra (or the existing ImageTokenKey string) and copy
ImageTokenKey into the returned struct, or (b) before constructing the new
model.ImageInfra fetch the current ImageInfra (or its ImageTokenKey) and set
imageInfra.ImageTokenKey = existing.ImageTokenKey; ensure this change is applied
where SourceUpdateForm.ToImageInfra is used so that Update/Save does not
overwrite ImageTokenKey generated during CreateSource or used by
image.GenerateDownloadURLByToken.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0efbcce5-0a2b-4c79-8bb6-8a2a620248df
📒 Files selected for processing (3)
internal/service/mappers/inbound.gointernal/service/source.gointernal/store/image.go
| // Update ImageInfra | ||
| form.ToImageInfra(&source.ImageInfra) | ||
| if _, err := s.store.ImageInfra().Update(ctx, source.ImageInfra); err != nil { | ||
| imageInfra := form.ToImageInfra(source.ID) | ||
| if _, err := s.store.ImageInfra().Update(ctx, imageInfra); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This discards the existing ImageInfra values including ImageTokenKey.
The source fetched at line 136 contains source.ImageInfra with all existing values (including ImageTokenKey). However, form.ToImageInfra(source.ID) creates a new struct without those values, which are then overwritten to zero in the database.
To fix the bug while preserving existing data, consider one of these approaches:
Option 1: Revert to mutation pattern (preserves all values)
// Update ImageInfra
- imageInfra := form.ToImageInfra(source.ID)
- if _, err := s.store.ImageInfra().Update(ctx, imageInfra); err != nil {
+ form.ToImageInfra(&source.ImageInfra)
+ if _, err := s.store.ImageInfra().Update(ctx, source.ImageInfra); err != nil {
return nil, err
}Option 2: Pass existing ImageInfra to new mapper (if you prefer value semantics)
Change the mapper to accept and merge with the existing struct:
func (f *SourceUpdateForm) ToImageInfra(existing model.ImageInfra) model.ImageInfra {
// Start with existing values, then override with non-nil form fields
if f.SshPublicKey != nil {
existing.SshPublicKey = *f.SshPublicKey
}
// ... etc
return existing
}Then call: imageInfra := form.ToImageInfra(source.ImageInfra)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/source.go` around lines 150 - 154, The update currently
overwrites and zeroes existing ImageInfra fields (including ImageTokenKey)
because form.ToImageInfra(source.ID) creates a new struct; instead preserve
existing values by merging with the fetched source.ImageInfra before calling
s.store.ImageInfra().Update. Fix by either mutating the existing
source.ImageInfra (update fields on source.ImageInfra then pass it to
s.store.ImageInfra().Update) or change SourceUpdateForm.ToImageInfra to accept
the existing model.ImageInfra (e.g., ToImageInfra(existing model.ImageInfra)
model.ImageInfra) and call it with source.ImageInfra so only intended fields are
overwritten while ImageTokenKey and other existing values remain intact when
invoking s.store.ImageInfra().Update.
1 participant
This commit fixes the bug when source InfraImage cannot be updated once it has been set.
Signed-off-by: Cosmin Tupangiu cosmin@redhat.com
Summary by CodeRabbit