✨ Add analysis profiles. #946
Conversation
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
WalkthroughAdds AnalysisProfile end-to-end: REST resource and handler, client bindings and tests, migration v21 with new analysis/application models, TargetProfile association and eager preload, file-reference discovery update, and a helper script for manual POSTing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as AnalysisProfile API
participant DB as GORM DB
rect rgb(223,239,255)
note right of Client: Create AnalysisProfile
Client->>API: POST /analysis/profiles
API->>DB: Create AnalysisProfile (persist model, then replace Targets associations)
DB-->>API: Persisted (ID)
API-->>Client: 201 Created (resource)
end
rect rgb(240,255,240)
note right of Client: Read with eager loads
Client->>API: GET /analysis/profiles/:id
API->>DB: Query AnalysisProfile with Preload(Targets, Files, Repository)
DB-->>API: Resource + relations
API-->>Client: 200 OK
end
rect rgb(255,247,223)
note right of Client: App-scoped listing and Archetype association
Client->>API: GET /applications/:id/analysis/profiles
API->>DB: Resolve archetypes -> collect AnalysisProfileIDs
API->>DB: Fetch AnalysisProfiles by IDs (preloads)
DB-->>API: []Profiles
API-->>Client: 200 OK (filtered list)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Points to focus review on:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
migration/v21/model/analysis.go (1)
147-155: Enforce a non-null name on AnalysisProfile.Right now the uniqueness constraint still permits multiple rows with a NULL
name, which breaks the invariant that profiles should always be addressable by name. Please tighten the column definition so the database enforces presence as well as uniqueness.- Name string `gorm:"uniqueIndex"` + Name string `gorm:"uniqueIndex;not null"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
api/archetype.go(5 hunks)api/pkg.go(1 hunks)api/profile.go(1 hunks)binding/analysisprofile.go(1 hunks)binding/richclient.go(2 hunks)hack/add/analysis-profile.sh(1 hunks)migration/pkg.go(2 hunks)migration/v21/migrate.go(1 hunks)migration/v21/model/analysis.go(1 hunks)migration/v21/model/application.go(1 hunks)migration/v21/model/assessment.go(1 hunks)migration/v21/model/core.go(1 hunks)migration/v21/model/mod.patch(1 hunks)migration/v21/model/pkg.go(1 hunks)migration/v21/model/platform.go(1 hunks)model/pkg.go(3 hunks)reaper/file.go(1 hunks)test/api/analysisprofile/api_test.go(1 hunks)test/api/analysisprofile/pkg.go(1 hunks)test/api/analysisprofile/samples.go(1 hunks)test/api/archetype/api_test.go(2 hunks)test/api/questionnaire/samples.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-21T11:32:18.960Z
Learnt from: jortel
Repo: konveyor/tackle2-hub PR: 897
File: migration/v19/model/application.go:25-26
Timestamp: 2025-10-21T11:32:18.960Z
Learning: In the tackle2-hub repository, new migrations (e.g., migration/v19) are created by copying all models from the previous migration version (e.g., migration/v18). Only specific models relevant to the PR are modified. When reviewing migration files, focus only on the models that have actual changes relevant to the PR objectives, and avoid flagging issues in unchanged copied models.
Applied to files:
migration/v21/migrate.gomigration/v21/model/core.gomigration/pkg.gomigration/v21/model/application.gomodel/pkg.go
📚 Learning: 2025-09-11T22:10:42.405Z
Learnt from: jortel
Repo: konveyor/tackle2-hub PR: 911
File: migration/v19/migrate.go:12-19
Timestamp: 2025-09-11T22:10:42.405Z
Learning: In the tackle2-hub migration system, the db parameter passed to Migration.Apply() is already a transaction (tx), not a regular database connection. The migration framework handles transactions at a higher level, so individual migrations don't need to wrap their operations in additional transactions.
Applied to files:
migration/v21/migrate.go
📚 Learning: 2025-07-09T11:55:52.155Z
Learnt from: jortel
Repo: konveyor/tackle2-hub PR: 849
File: test/api/application/manifest_test.go:33-33
Timestamp: 2025-07-09T11:55:52.155Z
Learning: The test `TestAppManifestGet` in test/api/application/manifest_test.go is intentionally designed to test the application-scoped manifest API functionality. It deliberately uses `Application.Manifest(application.ID)` for operations that are supported (Create, Get) while using `RichClient.Manifest.Delete` for cleanup operations that are not supported by the application-scoped API.
Applied to files:
test/api/analysisprofile/api_test.go
🧬 Code graph analysis (19)
test/api/analysisprofile/pkg.go (3)
binding/richclient.go (1)
RichClient(13-45)binding/analysisprofile.go (1)
AnalysisProfile(8-10)test/api/client/client.go (1)
PrepareRichClient(22-39)
migration/v21/migrate.go (2)
migration/pkg.go (2)
Migration(45-48)All(51-74)migration/v21/model/pkg.go (1)
All(17-63)
test/api/analysisprofile/samples.go (3)
api/profile.go (5)
AnalysisProfile(259-266)ApMode(240-242)ApScope(245-248)InExList(237-237)ApRules(251-256)migration/v21/model/analysis.go (2)
AnalysisProfile(145-156)InExList(186-189)migration/v21/model/application.go (1)
Repository(335-341)
migration/v21/model/core.go (3)
model/pkg.go (2)
Setting(43-43)File(27-27)migration/json/pkg.go (2)
Marshal(6-6)Unmarshal(5-5)migration/v21/model/pkg.go (1)
Settings(8-8)
api/pkg.go (1)
api/profile.go (1)
AnalysisProfileHandler(22-24)
test/api/analysisprofile/api_test.go (6)
api/profile.go (1)
AnalysisProfile(259-266)binding/analysisprofile.go (1)
AnalysisProfile(8-10)test/api/analysisprofile/pkg.go (1)
AnalysisProfile(10-10)migration/json/pkg.go (2)
Marshal(6-6)Unmarshal(5-5)test/api/analysisprofile/samples.go (1)
Base(8-33)test/assert/equality.go (1)
Eq(17-22)
migration/v21/model/platform.go (3)
migration/v21/model/core.go (3)
Model(15-20)Identity(197-209)Task(118-147)model/pkg.go (7)
Model(14-14)Map(60-60)Application(15-15)Identity(30-30)Task(52-52)Repository(66-66)TargetProfile(51-51)migration/v21/model/application.go (3)
Application(12-40)Repository(335-341)TargetProfile(187-195)
migration/v21/model/pkg.go (3)
api/pkg.go (2)
Settings(22-22)All(69-109)migration/pkg.go (2)
Settings(30-30)All(51-74)model/pkg.go (8)
Application(15-15)TechDependency(18-18)Incident(19-19)Analysis(20-20)AnalysisProfile(21-21)Dependency(26-26)File(27-27)Setting(43-43)
binding/richclient.go (3)
binding/analysisprofile.go (1)
AnalysisProfile(8-10)test/api/analysisprofile/pkg.go (1)
AnalysisProfile(10-10)model/pkg.go (1)
AnalysisProfile(21-21)
test/api/archetype/api_test.go (5)
binding/richclient.go (1)
RichClient(13-45)test/api/archetype/pkg.go (2)
RichClient(9-9)Archetype(10-10)test/assert/error.go (1)
Must(17-21)api/base.go (1)
Ref(344-347)api/archetype.go (1)
Archetype(518-534)
api/archetype.go (3)
api/base.go (2)
Resource(294-299)Ref(344-347)api/profile.go (1)
AnalysisProfile(259-266)migration/v21/model/analysis.go (1)
AnalysisProfile(145-156)
reaper/file.go (3)
api/profile.go (1)
AnalysisProfile(259-266)migration/v21/model/analysis.go (1)
AnalysisProfile(145-156)model/pkg.go (1)
AnalysisProfile(21-21)
api/profile.go (7)
api/application.go (1)
ApplicationRoot(23-23)api/base.go (1)
BaseHandler(37-37)api/auth.go (1)
Required(99-128)api/context.go (1)
Context(32-49)migration/v21/model/analysis.go (3)
AnalysisProfile(145-156)InExList(186-189)Target(127-139)migration/v21/model/application.go (2)
Application(12-40)Repository(335-341)assessment/membership.go (1)
NewMembershipResolver(11-24)
migration/v21/model/assessment.go (3)
migration/v21/model/core.go (1)
Model(15-20)model/pkg.go (14)
Model(14-14)Section(75-75)Thresholds(78-78)RiskMessages(79-79)Assessment(17-17)Questionnaire(41-41)Application(15-15)Archetype(16-16)Stakeholder(46-46)StakeholderGroup(47-47)Question(76-76)CategorizedTag(80-80)Answer(77-77)Tag(48-48)migration/v21/model/application.go (5)
Application(12-40)Archetype(173-185)Stakeholder(137-150)StakeholderGroup(152-161)Tag(205-211)
migration/pkg.go (1)
migration/v21/migrate.go (1)
Migration(8-8)
migration/v21/model/application.go (4)
migration/v21/model/core.go (4)
Model(15-20)BucketOwner(71-74)Task(118-147)Identity(197-209)migration/v21/model/assessment.go (2)
Review(35-46)Assessment(20-33)migration/v21/model/analysis.go (3)
Analysis(9-19)AnalysisProfile(145-156)Link(174-177)migration/v21/model/platform.go (2)
Platform(15-24)Manifest(7-13)
binding/analysisprofile.go (1)
api/profile.go (2)
AnalysisProfilesRoot(14-14)AnalysisProfileRoot(15-15)
migration/v21/model/analysis.go (3)
migration/v21/model/core.go (3)
Model(15-20)File(94-100)Identity(197-209)model/pkg.go (19)
Model(14-14)ArchivedInsight(63-63)Insight(22-22)TechDependency(18-18)Application(15-15)Analysis(20-20)RuleSet(44-44)Rule(45-45)Incident(19-19)Link(65-65)Map(60-60)File(27-27)Repository(66-66)Identity(30-30)DependencyCyclicError(87-87)TargetLabel(67-67)Target(50-50)InExList(72-72)Ref(59-59)migration/v21/model/application.go (3)
Application(12-40)Repository(335-341)DependencyCyclicError(114-114)
model/pkg.go (2)
api/profile.go (2)
AnalysisProfile(259-266)InExList(237-237)migration/v21/model/analysis.go (2)
AnalysisProfile(145-156)InExList(186-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-api
🔇 Additional comments (13)
test/api/questionnaire/samples.go (1)
11-13: Formatting looks consistent.Spacing tweak keeps the sample readable; no functional changes to the fixture.
migration/v21/model/platform.go (1)
7-38: New model definitions read clean.Manifest/Platform/Generator follow the existing GORM conventions (JSON fields, FK associations, delete behavior); no blockers spotted.
migration/v21/model/core.go (1)
118-146: Task ↔ Platform wiring looks right.
PlatformIDstays optional, indexed, and paired with a pointer field, matching the new Platform model expectations.api/pkg.go (1)
73-73: Handler registration updated.Including
AnalysisProfileHandlerensures the new routes are reachable; looks good.reaper/file.go (1)
34-39: Reaper coverage extended.Adding
AnalysisProfilekeeps uploaded profile files from going orphaned; solid catch.migration/pkg.go (1)
17-17: LGTM!The v21 migration is correctly imported and registered in the migration sequence, following the established pattern.
Also applies to: 72-72
binding/richclient.go (1)
15-15: LGTM!The AnalysisProfile field is correctly added to RichClient and initialized following the same pattern as other resource bindings.
Also applies to: 59-61
migration/v21/model/pkg.go (1)
1-63: LGTM!The v21 model package correctly exports all models including the new AnalysisProfile. The structure follows the established migration pattern.
model/pkg.go (1)
5-5: LGTM!The model package correctly migrates to v21 and exports the new AnalysisProfile and InExList types. The changes align with the v21 migration introduction.
Also applies to: 21-21, 72-72
test/api/analysisprofile/api_test.go (1)
11-54: LGTM!The CRUD test follows established patterns and provides comprehensive coverage: Create, Get, Update, and Delete operations with proper assertions. The UpdateUser alignment (line 40) correctly handles server-set fields.
migration/v21/migrate.go (1)
1-17: LGTM!The v21 migration correctly implements the Migration interface, using AutoMigrate to apply schema changes. The implementation follows the established migration pattern.
api/archetype.go (2)
55-55: LGTM!The eager loading for
Profiles.AnalysisProfileis correctly added to prevent N+1 query issues when fetching archetypes.Also applies to: 101-101
477-515: LGTM!The AnalysisProfile integration into TargetProfile follows established patterns:
- Optional reference field with proper JSON/YAML tags
- Bidirectional conversion between model and REST resource
- Correct FK assignment from the reference ID
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
Signed-off-by: Jeff Ortel <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/profile.go (1)
170-178: Persist target updates on PUT.
db.Save(m)skips many-to-many maintenance, so any change torules.targetson a PUT is dropped. Mirror the create path: omit associations while saving the profile fields and then explicitly replace theTargetsassociation to keep the join table in sync.m := r.Model() m.ID = id m.UpdateUser = h.CurrentUser(ctx) db := h.DB(ctx) - err = db.Save(m).Error + db = db.Omit(clause.Associations) + err = db.Save(m).Error if err != nil { _ = ctx.Error(err) return } db = h.DB(ctx).Model(m) err = db.Association("Targets").Replace(m.Targets)hack/add/analysis-profile.sh (1)
5-7: Don't repurpose$2; it breaks the repository override.Line 7 now expects the repository in
$3, so./hack/add/analysis-profile.sh 0 git://exampleends up naming the profilegit://example-0and still posting the default DayTrader repo. That’s a regression for anyone overriding the repository. Keep$2for the repo and expose the name via an explicit knob (e.g.NAME) so current invocations keep working.id="${1:-0}" # 0=system-assigned. -name="${2:-Test}-${id}" -repository="${3:-https://github.com/WASdev/sample.daytrader7.git}" +name="${NAME:-Test}-${id}" +repository="${2:-https://github.com/WASdev/sample.daytrader7.git}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/profile.go(1 hunks)hack/add/analysis-profile.sh(1 hunks)test/api/analysisprofile/pkg.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-09T11:55:52.155Z
Learnt from: jortel
Repo: konveyor/tackle2-hub PR: 849
File: test/api/application/manifest_test.go:33-33
Timestamp: 2025-07-09T11:55:52.155Z
Learning: The test `TestAppManifestGet` in test/api/application/manifest_test.go is intentionally designed to test the application-scoped manifest API functionality. It deliberately uses `Application.Manifest(application.ID)` for operations that are supported (Create, Get) while using `RichClient.Manifest.Delete` for cleanup operations that are not supported by the application-scoped API.
Applied to files:
test/api/analysisprofile/pkg.go
🧬 Code graph analysis (2)
test/api/analysisprofile/pkg.go (4)
binding/richclient.go (1)
RichClient(13-45)api/profile.go (1)
AnalysisProfile(269-276)binding/analysisprofile.go (1)
AnalysisProfile(8-10)test/api/client/client.go (1)
PrepareRichClient(22-39)
api/profile.go (8)
api/application.go (1)
ApplicationRoot(23-23)api/base.go (1)
BaseHandler(37-37)api/auth.go (1)
Required(99-128)api/context.go (1)
Context(32-49)migration/v21/model/analysis.go (3)
AnalysisProfile(145-156)InExList(186-189)Target(127-139)model/pkg.go (7)
AnalysisProfile(21-21)Model(14-14)Application(15-15)InExList(72-72)Ref(59-59)Repository(66-66)Target(50-50)migration/v21/model/application.go (2)
Application(12-40)Repository(335-341)assessment/membership.go (1)
NewMembershipResolver(11-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: test-unit
- GitHub Check: vet
Signed-off-by: Jeff Ortel <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/profile.go (1)
159-184: Persist target updates on PUT.
db.Save(m)at line 171 does not omit associations, which can cause Target updates to be silently dropped or handled incorrectly. The Create handler (line 109) shows the correct pattern. Please adddb.Omit(clause.Associations)before the Save call to match the create path.Apply this diff:
m := r.Model() m.ID = id m.UpdateUser = h.CurrentUser(ctx) db := h.DB(ctx) + db = db.Omit(clause.Associations) err = db.Save(m).Error if err != nil { _ = ctx.Error(err) return } db = h.DB(ctx).Model(m) err = db.Association("Targets").Replace(m.Targets) if err != nil { _ = ctx.Error(err) return } h.Status(ctx, http.StatusNoContent)
🧹 Nitpick comments (2)
api/profile.go (2)
70-88: Consider adding pagination for large datasets.The List endpoint fetches all profiles without pagination, which could impact performance and memory usage as the number of profiles grows.
220-226: Consider deduplicating profile IDs.If multiple archetypes reference the same profile, the ID will appear multiple times in the
idsslice. While GORM'sFindhandles this correctly, deduplicating the IDs before the query would be slightly more efficient.You could add a simple deduplication using a map:
idSet := make(map[uint]bool) for _, archetype := range archetypes { for _, p := range archetype.Profiles { if p.AnalysisProfileID != nil { idSet[*p.AnalysisProfileID] = true } } } var ids []uint for id := range idSet { ids = append(ids, id) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/profile.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api/profile.go (7)
api/application.go (1)
ApplicationRoot(23-23)api/base.go (1)
BaseHandler(37-37)api/auth.go (1)
Required(99-128)api/context.go (1)
Context(32-49)migration/v21/model/analysis.go (3)
AnalysisProfile(145-156)InExList(186-189)Target(127-139)model/pkg.go (7)
AnalysisProfile(21-21)Model(14-14)Application(15-15)InExList(72-72)Ref(59-59)Repository(66-66)Target(50-50)assessment/membership.go (1)
NewMembershipResolver(11-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: test-unit
- GitHub Check: vet
🔇 Additional comments (6)
api/profile.go (6)
26-37: LGTM!Route registration follows REST conventions and properly applies authorization middleware to all endpoints.
47-61: LGTM!Get handler correctly preloads associations, fetches by ID, and propagates errors.
99-123: LGTM!Create handler correctly implements the two-phase pattern: omitting associations during initial save, then explicitly replacing the Targets association to ensure the join table is properly maintained.
132-148: LGTM!Delete handler follows the standard pattern of fetching before deleting, with proper error handling.
194-246: LGTM!The error handling and empty ID guard issues flagged in previous reviews have been addressed. The handler now properly checks errors from
memberResolver.Archetypes(lines 216-219) and guards against empty ID lists (line 228).
248-334: LGTM!Type definitions and conversion methods (With/Model) correctly handle the bidirectional mapping between REST resources and database models, including proper handling of optional fields like Repository and slice conversions for Targets and Files.
Signed-off-by: Jeff Ortel <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/profile.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api/profile.go (8)
api/application.go (1)
ApplicationRoot(23-23)api/base.go (1)
BaseHandler(37-37)api/auth.go (1)
Required(99-128)api/context.go (1)
Context(32-49)migration/v21/model/analysis.go (3)
AnalysisProfile(145-156)InExList(186-189)Target(127-139)migration/v21/model/core.go (1)
Model(15-20)model/reflect/db.go (1)
Omit(19-27)assessment/membership.go (1)
NewMembershipResolver(11-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: vet
- GitHub Check: test-unit
Signed-off-by: Jeff Ortel <[email protected]>
|
@coderabbitai Review |
✅ Actions performedReview triggered.
|
aufi
left a comment
aufi
left a comment
There was a problem hiding this comment.
Nice PR, looks good to me!
Note on understanding of the analysis profiles, it could be considered as defaults for analysis task mapped to applications via archetypes. However, from a completed application analysis, is doesn't make sense and it is not expected to look back to the analysis profile, since all relevant data were already added into the the analysis tasks data, is that right understanding?
2 participants
Add support for analysis profiles.
Main routes
GET | POST /analysis/profiles
GET | PUT | DELETE /analysis/profiles/:id
Find profiles mapped to an applications.
Find profiles mapped to an application through archetypes using the analysis/profiles sub-resource.
GET /applications/:id/analysis/profiles
Rest Resource
Associate with Archetypes
Associate with optional ref: TargetProfile.analysisProfile:
closes #928
Summary by CodeRabbit