✨ Add binding methods for analysis. #980
Conversation
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
📝 WalkthroughWalkthroughThis PR introduces a new Analysis API endpoint with POST creation support, adds thin resource variants for efficient data transfer, establishes standalone and application-scoped binding clients for Analysis operations, and provides comprehensive test infrastructure including comparison helpers and fixtures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@internal/api/resource/analysis.go`:
- Around line 23-31: The ThinModel method on Analysis omits copying the Archived
field back into the model, causing asymmetry with ThinWith; update
Analysis.ThinModel to set m.Archived = r.Archived (add the Archived assignment
alongside Effort, Commit, and Insights) so the resource-to-model conversion
preserves the Archived state.
In `@shared/binding/application.go`:
- Around line 334-337: The Get method on AppAnalysis currently passes a nil
pointer-to-pointer (&r) to h.client.Get; initialize the response first by
creating r as &api.Analysis{} and pass r (not &r) into h.client.Get so
json.Unmarshal has a valid target (i.e., in AppAnalysis.Get set r =
&api.Analysis{} before calling h.client.Get with path and r).
In `@test/api/analysis/cmp.go`:
- Around line 93-102: The comparison of map values using `v != bv` in the Facts
comparison block can panic for non-comparable types; update the logic in the
function that compares `a.Facts` and `b.Facts` to use `reflect.DeepEqual(v, bv)`
instead of `v != bv` (and import reflect if needed), and apply the same change
to the other identical block referenced at lines ~130-139 so all value
comparisons on `Facts` use `reflect.DeepEqual` to safely handle slices, maps,
and other non-comparable types.
In `@test/api/analysis/pkg_test.go`:
- Around line 25-27: The test currently ignores errors from json.Marshal(Sample)
and json.Unmarshal(b, &r); update the code to check those errors with
assert.Must() so failures fail the test immediately: call assert.Must on the
result of json.Marshal(Sample) (or on the error returned) when creating b and
call assert.Must on json.Unmarshal(b, &r) before using r, referencing the
variables and functions in this snippet (json.Marshal, json.Unmarshal, Sample,
b, r, and assert.Must) to locate and fix the assertions.
In `@test/api/application/analysis_test.go`:
- Around line 51-69: The test currently ignores API errors and returns silently
on mismatches; update the assertions in the GetInsights and GetDependencies
blocks to fail the test on error and mismatch instead of returning: check the
returned err from anAPI.GetInsights and anAPI.GetDependencies and call
t.Fatalf/t.Errorf with a descriptive message if err != nil, assert lengths of
r.Insights vs gotInsights and r.Dependencies vs gotDeps using t.Fatalf/t.Errorf
when they differ, and use t.Fatalf/t.Errorf when analysis.EqInsight or
analysis.EqDependency finds a mismatch (include index and values in the
message); this involves editing the code around anAPI.GetInsights, gotInsights,
anAPI.GetDependencies, and gotDeps checks.
🧹 Nitpick comments (3)
internal/api/resource/analysis.go (2)
35-37: Redundant Application assignment.Line 37 sets
r.Applicationbut this is already set byThinWith(m)on line 36 (see line 17 in ThinWith). This duplicate assignment can be removed.♻️ Suggested fix
func (r *Analysis) With(m *model.Analysis) { r.ThinWith(m) - r.Application = ref(m.ApplicationID, m.Application) r.Insights = make([]api.Insight, 0)
25-31: Consider initializingDependenciesfor consistency.
m.Insightsis initialized to an empty slice, butm.Dependenciesis left as nil. While Go allows appending to nil slices, this could lead to inconsistent JSON serialization (nullvs[]) if no dependencies exist.♻️ Suggested fix
func (r *Analysis) ThinModel() (m *model.Analysis) { m = &model.Analysis{} m.Effort = r.Effort m.Commit = r.Commit m.Insights = []model.Insight{} + m.Dependencies = []model.TechDependency{} return }internal/api/analysis.go (1)
301-318: Consider enforcing the “thin-only” contract in Create.
r.Model()will include nested slices if present, and GORM may try to persist associations. That undermines “must NOT be used for large resources.” Recommend usingThinModel()(and optionally rejecting non-empty insights/deps) and responding withThinWith()for consistency.♻️ Proposed adjustment
- m := r.Model() + m := r.ThinModel() m.CreateUser = h.CurrentUser(ctx) db := h.DB(ctx) err = db.Create(m).Error @@ - r.With(m) + r.ThinWith(m) h.Respond(ctx, http.StatusCreated, r)
| // ThinModel builds a model. | ||
| // This must be used in with manifest-based or collection endpoints. | ||
| func (r *Analysis) ThinModel() (m *model.Analysis) { | ||
| m = &model.Analysis{} | ||
| m.Effort = r.Effort | ||
| m.Commit = r.Commit | ||
| m.Insights = []model.Insight{} | ||
| return | ||
| } |
There was a problem hiding this comment.
Missing Archived field in ThinModel.
ThinWith sets r.Archived = m.Archived (line 20), but ThinModel doesn't set m.Archived = r.Archived. This asymmetry could cause data loss when converting the resource back to a model.
🐛 Proposed fix
func (r *Analysis) ThinModel() (m *model.Analysis) {
m = &model.Analysis{}
m.Effort = r.Effort
m.Commit = r.Commit
+ m.Archived = r.Archived
m.Insights = []model.Insight{}
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ThinModel builds a model. | |
| // This must be used in with manifest-based or collection endpoints. | |
| func (r *Analysis) ThinModel() (m *model.Analysis) { | |
| m = &model.Analysis{} | |
| m.Effort = r.Effort | |
| m.Commit = r.Commit | |
| m.Insights = []model.Insight{} | |
| return | |
| } | |
| func (r *Analysis) ThinModel() (m *model.Analysis) { | |
| m = &model.Analysis{} | |
| m.Effort = r.Effort | |
| m.Commit = r.Commit | |
| m.Archived = r.Archived | |
| m.Insights = []model.Insight{} | |
| return | |
| } |
🤖 Prompt for AI Agents
In `@internal/api/resource/analysis.go` around lines 23 - 31, The ThinModel method
on Analysis omits copying the Archived field back into the model, causing
asymmetry with ThinWith; update Analysis.ThinModel to set m.Archived =
r.Archived (add the Archived assignment alongside Effort, Commit, and Insights)
so the resource-to-model conversion preserves the Archived state.
| func (h *AppAnalysis) Get() (r *api.Analysis, err error) { | ||
| path := Path(api.AppAnalysisRoute).Inject(Params{api.ID: h.appId}) | ||
| err = h.client.Get(path, &r) | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find client.go in shared/binding
fd -a "client.go" shared/binding
# Check Client.Get signature
rg -n "func .* Get\(" -g 'client.go' shared/binding -A 3Repository: konveyor/tackle2-hub
Length of output: 456
🏁 Script executed:
# Read the full Client.Get implementation
sed -n '99,150p' shared/binding/client.goRepository: konveyor/tackle2-hub
Length of output: 1084
🏁 Script executed:
# Check how other similar Get methods are implemented in other binding files
fd -a "\.go" shared/binding -type f | head -20 | xargs grep -l "func.*Get.*\*.*error" | head -5
# Look for similar patterns in other binding files
rg "err = h\.client\.Get\(" shared/binding -B 5 -A 2 | head -50Repository: konveyor/tackle2-hub
Length of output: 2774
🏁 Script executed:
# Get the exact context around line 334-337
sed -n '330,340p' shared/binding/application.goRepository: konveyor/tackle2-hub
Length of output: 342
Initialize the response object before Get.
Passing &r (where r is nil) violates the pattern used in other methods and will cause json.Unmarshal to fail since it expects a pointer to a valid object, not a nil pointer-to-pointer. Initialize r to &api.Analysis{} and pass r directly instead of &r.
🐛 Proposed fix
func (h *AppAnalysis) Get() (r *api.Analysis, err error) {
path := Path(api.AppAnalysisRoute).Inject(Params{api.ID: h.appId})
- err = h.client.Get(path, &r)
+ r = &api.Analysis{}
+ err = h.client.Get(path, r)
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (h *AppAnalysis) Get() (r *api.Analysis, err error) { | |
| path := Path(api.AppAnalysisRoute).Inject(Params{api.ID: h.appId}) | |
| err = h.client.Get(path, &r) | |
| return | |
| func (h *AppAnalysis) Get() (r *api.Analysis, err error) { | |
| path := Path(api.AppAnalysisRoute).Inject(Params{api.ID: h.appId}) | |
| r = &api.Analysis{} | |
| err = h.client.Get(path, r) | |
| return | |
| } |
🤖 Prompt for AI Agents
In `@shared/binding/application.go` around lines 334 - 337, The Get method on
AppAnalysis currently passes a nil pointer-to-pointer (&r) to h.client.Get;
initialize the response first by creating r as &api.Analysis{} and pass r (not
&r) into h.client.Get so json.Unmarshal has a valid target (i.e., in
AppAnalysis.Get set r = &api.Analysis{} before calling h.client.Get with path
and r).
| // Compare Facts (map[string]interface{} or similar) | ||
| if len(a.Facts) != len(b.Facts) { | ||
| return | ||
| } | ||
| for k, v := range a.Facts { | ||
| bv, ok := b.Facts[k] | ||
| if !ok || v != bv { | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "cmp.go" -path "*/api/analysis/*"Repository: konveyor/tackle2-hub
Length of output: 90
🏁 Script executed:
cat -n ./test/api/analysis/cmp.go | head -150Repository: konveyor/tackle2-hub
Length of output: 3995
🏁 Script executed:
find . -path "*/shared/api/*" -name "*.go" | head -20Repository: konveyor/tackle2-hub
Length of output: 431
🏁 Script executed:
rg "type Insight|type Incident" ./shared/api/ -A 20Repository: konveyor/tackle2-hub
Length of output: 4711
🏁 Script executed:
rg "type Map " ./shared/api/ -A 5Repository: konveyor/tackle2-hub
Length of output: 360
🏁 Script executed:
cat -n ./shared/api/core.go | grep -A 2 "type Map"Repository: konveyor/tackle2-hub
Length of output: 152
🏁 Script executed:
rg "Facts\s*=" ./shared/api/ -B 3 -A 3 | head -50Repository: konveyor/tackle2-hub
Length of output: 46
🏁 Script executed:
rg "Facts" ./shared/api/analysis.go -B 2 -A 2Repository: konveyor/tackle2-hub
Length of output: 433
🏁 Script executed:
rg "\.Facts\[" . --type go | head -30Repository: konveyor/tackle2-hub
Length of output: 342
🏁 Script executed:
rg "Facts\[" ./internal/api/ -B 5 -A 5Repository: konveyor/tackle2-hub
Length of output: 1629
🏁 Script executed:
rg "Facts.*:=" . --type go -B 3 -A 3 | head -80Repository: konveyor/tackle2-hub
Length of output: 447
🏁 Script executed:
rg "Facts\s*\{" . --type go -B 2 -A 10 | head -80Repository: konveyor/tackle2-hub
Length of output: 3766
🏁 Script executed:
rg "api.Map\|make.*Map" . --type go -B 2 -A 5 | head -100Repository: konveyor/tackle2-hub
Length of output: 46
🏁 Script executed:
rg "api.Insight|api.Incident" ./test/api/analysis/ --type go -B 2 -A 8 | head -150Repository: konveyor/tackle2-hub
Length of output: 3964
🏁 Script executed:
cat -n ./test/api/analysis/sample.go | sed -n '1,120p'Repository: konveyor/tackle2-hub
Length of output: 4226
🏁 Script executed:
rg "json.Unmarshal|json.Marshal" ./shared/api/core.go -B 2 -A 5Repository: konveyor/tackle2-hub
Length of output: 286
Unsafe map value comparison can panic with non-comparable types.
Since Facts is of type Map (defined as map[string]any), it can theoretically contain non-comparable types like slices or maps. The comparison v != bv will panic at runtime if such values are present. Use reflect.DeepEqual to safely compare maps with arbitrary value types.
Proposed fix
import (
"github.com/konveyor/tackle2-hub/shared/api"
+ "reflect"
)
@@
- // Compare Facts (map[string]interface{} or similar)
- if len(a.Facts) != len(b.Facts) {
- return
- }
- for k, v := range a.Facts {
- bv, ok := b.Facts[k]
- if !ok || v != bv {
- return
- }
- }
+ // Compare Facts safely (handles non-comparable values)
+ if !reflect.DeepEqual(a.Facts, b.Facts) {
+ return
+ }
@@
- // Compare Facts
- if len(a.Facts) != len(b.Facts) {
- return
- }
- for k, v := range a.Facts {
- bv, ok := b.Facts[k]
- if !ok || v != bv {
- return
- }
- }
+ // Compare Facts safely (handles non-comparable values)
+ if !reflect.DeepEqual(a.Facts, b.Facts) {
+ return
+ }Also applies to: lines 130-139
🤖 Prompt for AI Agents
In `@test/api/analysis/cmp.go` around lines 93 - 102, The comparison of map values
using `v != bv` in the Facts comparison block can panic for non-comparable
types; update the logic in the function that compares `a.Facts` and `b.Facts` to
use `reflect.DeepEqual(v, bv)` instead of `v != bv` (and import reflect if
needed), and apply the same change to the other identical block referenced at
lines ~130-139 so all value comparisons on `Facts` use `reflect.DeepEqual` to
safely handle slices, maps, and other non-comparable types.
| b, _ := json.Marshal(Sample) | ||
| _ = json.Unmarshal(b, &r) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists
find . -name "pkg_test.go" -path "*/test/api/analysis/*" | head -20Repository: konveyor/tackle2-hub
Length of output: 184
🏁 Script executed:
# List files in the test/api/analysis directory
find . -path "*/test/api/analysis/*" -type f | head -30Repository: konveyor/tackle2-hub
Length of output: 179
🏁 Script executed:
# Search for pkg_test.go more broadly
fd "pkg_test.go" testRepository: konveyor/tackle2-hub
Length of output: 93
🏁 Script executed:
# Read the file to verify the code around lines 25-27
cat -n test/api/analysis/pkg_test.go | sed -n '1,40p'Repository: konveyor/tackle2-hub
Length of output: 1253
🏁 Script executed:
# Search for assert.Must in the codebase
rg "assert\.Must" --type go -B 2 -A 2 | head -50Repository: konveyor/tackle2-hub
Length of output: 2663
🏁 Script executed:
# Check for testing patterns used in pkg_test.go
rg "assert\." test/api/analysis/pkg_test.goRepository: konveyor/tackle2-hub
Length of output: 128
Handle JSON marshal/unmarshal errors using assert.Must().
Lines 25–26 ignore both errors, which would allow the test to continue with zero values if Sample ever becomes non-serializable, masking the real failure. Apply the suggested fix to match the error-handling pattern used elsewhere in this test.
Suggested fix
- b, _ := json.Marshal(Sample)
- _ = json.Unmarshal(b, &r)
+ b, err := json.Marshal(Sample)
+ assert.Must(t, err)
+ err = json.Unmarshal(b, &r)
+ assert.Must(t, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| b, _ := json.Marshal(Sample) | |
| _ = json.Unmarshal(b, &r) | |
| b, err := json.Marshal(Sample) | |
| assert.Must(t, err) | |
| err = json.Unmarshal(b, &r) | |
| assert.Must(t, err) |
🤖 Prompt for AI Agents
In `@test/api/analysis/pkg_test.go` around lines 25 - 27, The test currently
ignores errors from json.Marshal(Sample) and json.Unmarshal(b, &r); update the
code to check those errors with assert.Must() so failures fail the test
immediately: call assert.Must on the result of json.Marshal(Sample) (or on the
error returned) when creating b and call assert.Must on json.Unmarshal(b, &r)
before using r, referencing the variables and functions in this snippet
(json.Marshal, json.Unmarshal, Sample, b, r, and assert.Must) to locate and fix
the assertions.
| gotInsights, err := anAPI.GetInsights() | ||
| if len(r.Insights) != len(gotInsights) { | ||
| return | ||
| } | ||
| for i := range r.Insights { | ||
| if !analysis.EqInsight(r.Insights[i], gotInsights[i]) { | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Test get insights. | ||
| gotDeps, err := anAPI.GetDependencies() | ||
| if len(r.Dependencies) != len(gotDeps) { | ||
| return | ||
| } | ||
| for i := range r.Dependencies { | ||
| if !analysis.EqDependency(r.Dependencies[i], gotDeps[i]) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Fail the test on API errors or mismatches (avoid silent returns).
Currently, errors are ignored and mismatches just return, which can produce false positives.
✅ Suggested fix
- gotInsights, err := anAPI.GetInsights()
- if len(r.Insights) != len(gotInsights) {
- return
- }
+ gotInsights, err := anAPI.GetInsights()
+ assert.Must(t, err)
+ if len(r.Insights) != len(gotInsights) {
+ t.Fatalf("insights length mismatch: got %d, expected %d", len(gotInsights), len(r.Insights))
+ }
for i := range r.Insights {
if !analysis.EqInsight(r.Insights[i], gotInsights[i]) {
- return
+ t.Fatalf("insight mismatch at index %d", i)
}
}
@@
- gotDeps, err := anAPI.GetDependencies()
- if len(r.Dependencies) != len(gotDeps) {
- return
- }
+ gotDeps, err := anAPI.GetDependencies()
+ assert.Must(t, err)
+ if len(r.Dependencies) != len(gotDeps) {
+ t.Fatalf("dependencies length mismatch: got %d, expected %d", len(gotDeps), len(r.Dependencies))
+ }
for i := range r.Dependencies {
if !analysis.EqDependency(r.Dependencies[i], gotDeps[i]) {
- return
+ t.Fatalf("dependency mismatch at index %d", i)
}
}🤖 Prompt for AI Agents
In `@test/api/application/analysis_test.go` around lines 51 - 69, The test
currently ignores API errors and returns silently on mismatches; update the
assertions in the GetInsights and GetDependencies blocks to fail the test on
error and mismatch instead of returning: check the returned err from
anAPI.GetInsights and anAPI.GetDependencies and call t.Fatalf/t.Errorf with a
descriptive message if err != nil, assert lengths of r.Insights vs gotInsights
and r.Dependencies vs gotDeps using t.Fatalf/t.Errorf when they differ, and use
t.Fatalf/t.Errorf when analysis.EqInsight or analysis.EqDependency finds a
mismatch (include index and values in the message); this involves editing the
code around anAPI.GetInsights, gotInsights, anAPI.GetDependencies, and gotDeps
checks.
Signed-off-by: Jeff Ortel <[email protected]>
1 participant
Add support for POST /analyses to better support automated testing.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.