Feat/sp 4373 project service#14
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughIntroduces new project data access functionality with a ProjectService, adds a project lookup method by PURL name and type, creates a public Project response type with nullable source fields, renames GetProjectByPurlName to GetProjectByPurlNameAndMineID, and adds an ErrProjectNotFound error. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant ProjectService
participant ProjectModel
participant Database
participant TypeConversion as types.Project
Client->>ProjectService: GetProject(purl)
ProjectService->>ProjectService: Validate & parse PURL
ProjectService->>ProjectModel: GetProjectByPurlName(purlName, purlType)
ProjectModel->>Database: SELECT with mines join
Database-->>ProjectModel: Project row with source fields
ProjectModel-->>ProjectService: models.Project
ProjectService->>TypeConversion: Convert to types.Project
TypeConversion-->>ProjectService: types.Project (or ErrProjectNotFound)
ProjectService-->>Client: types.Project, error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 38 minutes and 4 seconds.Comment |
agustingroh
force-pushed
the
feat/SP-4373-project-service
branch
from
fd29acb to
78eedfc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/models/projects.go (1)
155-164:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent "not found" behavior between methods.
GetProjectByPurlNameAndMineIDreturns an emptyProject{}withnilerror when no rows match, whileGetProjectByPurlNamereturnssql.ErrNoRows. This inconsistency could confuse callers—one must check the struct for emptiness, the other checks the error.Consider aligning the behavior by returning
sql.ErrNoRowswhenrows.Next()is false:Proposed fix
var project Project if rows.Next() { err = rows.StructScan(&project) if err != nil { s.Errorf("Failed to parse projects table results for %#v: %v", rows, err) s.Errorf("Query failed for purl_name = %v, mine_id = %v", purlName, mineID) return Project{}, fmt.Errorf("failed to query the projects table: %v", err) } + } else { + return Project{}, sql.ErrNoRows } return project, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/projects.go` around lines 155 - 164, GetProjectByPurlNameAndMineID currently returns an empty Project with nil error when rows.Next() is false; change it to return sql.ErrNoRows in that case to match GetProjectByPurlName. Specifically, in GetProjectByPurlNameAndMineID check the result of rows.Next() and if false return Project{} and sql.ErrNoRows (instead of nil), keeping the existing error handling for StructScan unchanged so callers receive a consistent "not found" error.
🧹 Nitpick comments (2)
pkg/models/projects_test.go (1)
29-101: ⚡ Quick winMissing test coverage for
GetProjectByPurlName(purlName, purlType).The new
GetProjectByPurlNamemethod (used byProjectService.GetProject) is not exercised by this test suite. Consider adding test cases covering:
- Successful lookup with valid purlName + purlType
sql.ErrNoRowswhen project doesn't exist- Empty purlName/purlType validation errors
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/projects_test.go` around lines 29 - 101, Add unit tests in pkg/models/projects_test.go that exercise the new GetProjectByPurlName method (used by ProjectService.GetProject): reuse the existing projectsModel := NewProjectModel(db) and ctx setup, then add three subtests: (1) a successful lookup calling projectsModel.GetProjectByPurlName(ctx, "tablestyle", "gem") and assert no error and non-empty result; (2) a non-existent lookup calling GetProjectByPurlName with a bogus name/type and assert the error is sql.ErrNoRows; and (3) validation checks calling GetProjectByPurlName with empty purlName and/or empty purlType and assert a validation error is returned. Ensure tests import database/sql for sql.ErrNoRows and follow the existing pattern for setup/teardown and error assertions.pkg/models/projects.go (1)
74-103: 💤 Low valueConsider extracting the shared query columns.
The SELECT column list is duplicated across three methods (
GetProjectsByPurlName,GetProjectByPurlNameAndMineID,GetProjectByPurlName). If a column is added or renamed, all three must be updated in sync.Extracting the common column list into a constant would reduce duplication and maintenance burden.
Also applies to: 117-139, 182-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/models/projects.go` around lines 74 - 103, The SELECT column list is duplicated in GetProjectsByPurlName, GetProjectByPurlNameAndMineID and GetProjectByPurlName—extract that repeated column list into a single package-level constant (e.g., projectSelectCols) and concatenate it into the SQL strings in those three methods; update each method to replace the inline SELECT ... column block with the constant, preserve the same JOIN/WHERE fragments and parameter placeholders, and run tests to ensure the SQL formatting/spacing and returned fields (used by structs like the one scanned into allProjects) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/models/projects.go`:
- Around line 155-164: GetProjectByPurlNameAndMineID currently returns an empty
Project with nil error when rows.Next() is false; change it to return
sql.ErrNoRows in that case to match GetProjectByPurlName. Specifically, in
GetProjectByPurlNameAndMineID check the result of rows.Next() and if false
return Project{} and sql.ErrNoRows (instead of nil), keeping the existing error
handling for StructScan unchanged so callers receive a consistent "not found"
error.
---
Nitpick comments:
In `@pkg/models/projects_test.go`:
- Around line 29-101: Add unit tests in pkg/models/projects_test.go that
exercise the new GetProjectByPurlName method (used by
ProjectService.GetProject): reuse the existing projectsModel :=
NewProjectModel(db) and ctx setup, then add three subtests: (1) a successful
lookup calling projectsModel.GetProjectByPurlName(ctx, "tablestyle", "gem") and
assert no error and non-empty result; (2) a non-existent lookup calling
GetProjectByPurlName with a bogus name/type and assert the error is
sql.ErrNoRows; and (3) validation checks calling GetProjectByPurlName with empty
purlName and/or empty purlType and assert a validation error is returned. Ensure
tests import database/sql for sql.ErrNoRows and follow the existing pattern for
setup/teardown and error assertions.
In `@pkg/models/projects.go`:
- Around line 74-103: The SELECT column list is duplicated in
GetProjectsByPurlName, GetProjectByPurlNameAndMineID and
GetProjectByPurlName—extract that repeated column list into a single
package-level constant (e.g., projectSelectCols) and concatenate it into the SQL
strings in those three methods; update each method to replace the inline SELECT
... column block with the constant, preserve the same JOIN/WHERE fragments and
parameter placeholders, and run tests to ensure the SQL formatting/spacing and
returned fields (used by structs like the one scanned into allProjects) remain
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d42d7c4c-29a3-49ba-a2e8-3dd44431093c
📒 Files selected for processing (7)
CHANGELOG.mdinternal/testutils/mock/mines.sqlpkg/models/projects.gopkg/models/projects_test.gopkg/scanoss/client.gopkg/services/project.gopkg/types/types.go
…etrieve project data by PURL
agustingroh
force-pushed
the
feat/SP-4373-project-service
branch
from
78eedfc to
671b8cb
Compare
3 participants
Summary by CodeRabbit
New Features
Breaking Changes