fix: [AH-2825]: fix for download stat query#78
Conversation
📝 WalkthroughWalkthroughThe change modifies the download stat database operation to dynamically construct query arguments instead of using a fixed parameter list. The artifactType parameter is now conditionally appended only when it's non-nil and non-empty, adjusting the query behavior based on whether filtering by image type is needed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
registry/app/store/database/download_stat.go (2)
133-141: Missing query observability — inconsistent with peer methods.Every other method in this file logs the formatted SQL query before execution via
util.FormatQuery+log.Ctx(ctx).Debug().CreateByRegistryIDImageAndArtifactNameomits this, making it harder to diagnose production query issues.♻️ Proposed addition
+ finalQuery := util.FormatQuery(sqlStr, args) + log.Ctx(ctx).Debug().Str("sql", finalQuery).Msg("Executing CreateByRegistryIDImageAndArtifactName query") + _, err = db.ExecContext(ctx, sqlStr, args...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/store/database/download_stat.go` around lines 133 - 141, CreateByRegistryIDImageAndArtifactName is missing the SQL observability present in its peers; before calling db.ExecContext, call util.FormatQuery(sqlStr, args...) and log it with log.Ctx(ctx).Debug() so the executed query (with parameters) is emitted to logs. Locate the block in download_stat.go where args are built and db.ExecContext is invoked, format the query using util.FormatQuery(sqlStr, args...) into a variable, and pass that to log.Ctx(ctx).Debug() (matching the pattern used by other methods in this file) immediately before executing the query.
122-141: Fragile manual arg ordering — squirrel-generated args silently discarded.The squirrel builder accumulates args alongside
?placeholders forWhere()calls, but those args are thrown away (_at line 123) and rebuilt by hand. Any future edit — adding a WHERE condition, reordering columns, or changing theSelect(...)literal count — must keep this manualargsslice in sync, with no compiler or runtime guard until the query executes. The rest of the file binds values directly in squirrel (e.g.,sq.Eq{...}) so the returned args are correct by construction.Consider binding values inline so squirrel tracks them:
♻️ Sketch of a squirrel-native binding
- selectQuery := databaseg.Builder. - Select( - "a.artifact_id", - "?", - "?", - "?", - "?", - "?", - ). - From("artifacts a"). - Join("images i ON a.artifact_image_id = i.image_id"). - Where("a.artifact_version = ? AND i.image_registry_id = ? AND i.image_name = ? ") - if artifactType != nil && *artifactType != "" { - selectQuery = selectQuery.Where("i.image_type = ?") - } else { - selectQuery = selectQuery.Where("i.image_type IS NULL") - } - selectQuery = selectQuery.Limit(1) + now := time.Now().UnixMilli() + session, _ := request.AuthSessionFrom(ctx) + user := session.Principal.ID + + selectQuery := databaseg.Builder. + Select( + "a.artifact_id", + sq.Expr("?", now), + sq.Expr("?", now), + sq.Expr("?", now), + sq.Expr("?", user), + sq.Expr("?", user), + ). + From("artifacts a"). + Join("images i ON a.artifact_image_id = i.image_id"). + Where("a.artifact_version = ? AND i.image_registry_id = ? AND i.image_name = ?", + version, regID, image) + if artifactType != nil && *artifactType != "" { + selectQuery = selectQuery.Where("i.image_type = ?", *artifactType) + } else { + selectQuery = selectQuery.Where("i.image_type IS NULL") + } + selectQuery = selectQuery.Limit(1) insertQuery := databaseg.Builder. Insert("download_stats"). Columns( "download_stat_artifact_id", "download_stat_timestamp", "download_stat_created_at", "download_stat_updated_at", "download_stat_created_by", "download_stat_updated_by", ). Select(selectQuery) - sqlStr, _, err := insertQuery.ToSql() + sqlStr, args, err := insertQuery.ToSql() if err != nil { return fmt.Errorf("failed to generate SQL: %w", err) } - - session, _ := request.AuthSessionFrom(ctx) - user := session.Principal.ID db := dbtx.GetAccessor(ctx, d.db) - now := time.Now().UnixMilli() - args := []interface{}{now, now, now, user, user, version, regID, image} - - // Only add artifactType parameter if the WHERE clause includes it - if artifactType != nil && *artifactType != "" { - args = append(args, artifactType) - } - _, err = db.ExecContext(ctx, sqlStr, args...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/store/database/download_stat.go` around lines 122 - 141, The code discards squirrel's generated args (using `_` from insertQuery.ToSql()) and rebuilds them manually, which is fragile; instead capture the args returned by insertQuery.ToSql() (replace `sqlStr, _, err := insertQuery.ToSql()` with `sqlStr, args, err := insertQuery.ToSql()`), remove the manual args slice and any manual artifactType appending, ensure the squirrel `insertQuery`/`Where`/`Set` calls include the now/user/version/regID/image and optional artifactType bindings so squirrel produces the correct placeholders, and then call db.ExecContext(ctx, sqlStr, args...) (references: insertQuery, ToSql(), artifactType, db.ExecContext).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@registry/app/store/database/download_stat.go`:
- Around line 133-141: CreateByRegistryIDImageAndArtifactName is missing the SQL
observability present in its peers; before calling db.ExecContext, call
util.FormatQuery(sqlStr, args...) and log it with log.Ctx(ctx).Debug() so the
executed query (with parameters) is emitted to logs. Locate the block in
download_stat.go where args are built and db.ExecContext is invoked, format the
query using util.FormatQuery(sqlStr, args...) into a variable, and pass that to
log.Ctx(ctx).Debug() (matching the pattern used by other methods in this file)
immediately before executing the query.
- Around line 122-141: The code discards squirrel's generated args (using `_`
from insertQuery.ToSql()) and rebuilds them manually, which is fragile; instead
capture the args returned by insertQuery.ToSql() (replace `sqlStr, _, err :=
insertQuery.ToSql()` with `sqlStr, args, err := insertQuery.ToSql()`), remove
the manual args slice and any manual artifactType appending, ensure the squirrel
`insertQuery`/`Where`/`Set` calls include the now/user/version/regID/image and
optional artifactType bindings so squirrel produces the correct placeholders,
and then call db.ExecContext(ctx, sqlStr, args...) (references: insertQuery,
ToSql(), artifactType, db.ExecContext).
2 participants
Summary by CodeRabbit