Refactor common_mysql - #37245
Conversation
- Extract common HTTP error types to server/platform/http package to enable decoupling bounded contexts from the fleet package - Make CommonEndpointer fully generic ([H any]) so handler function types can be defined in their respective packages - Replace deprecated AuthFunc/FleetService fields with pre-built AuthMiddleware for cleaner dependency injection - Remove fleet import from endpoint_utils package (now only imports contexts/*, platform/http, and middleware/*)
…-endpoint_utils # Conflicts: # server/service/endpoint_utils.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #37245 +/- ##
==========================================
+ Coverage 65.82% 65.83% +0.01%
==========================================
Files 2367 2368 +1
Lines 187665 187752 +87
Branches 8012 8012
==========================================
+ Hits 123523 123600 +77
- Misses 52845 52856 +11
+ Partials 11297 11296 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…into victor/37244-refactor-common_mysql # Conflicts: # server/datastore/mysql/common_mysql/common.go # server/datastore/mysql/mysql.go # server/service/middleware/endpoint_utils/endpoint_utils.go
getvictor
changed the title
b92ceee to
b19db6b
Compare
…-common_mysql # Conflicts: # server/contexts/ctxerr/ctxerr.go # server/contexts/ctxerr/ctxerr_otel_test.go # server/contexts/ctxerr/ctxerr_test.go # server/contexts/ctxerr/metadata.go # server/contexts/ctxerr/statistics.go # server/contexts/host/host.go # server/contexts/license/license.go # server/contexts/logging/logging.go # server/contexts/viewer/viewer.go # server/mdm/android/service/endpoint_utils.go # server/platform/http/errors.go # server/service/appconfig.go # server/service/endpoint_middleware.go # server/service/endpoint_utils.go # server/service/middleware/auth/http_auth.go # server/service/middleware/endpoint_utils/endpoint_utils.go # server/service/middleware/endpoint_utils/transport_error.go
b19db6b to
1babe78
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR refactors the MySQL datastore layer to introduce shared database connections and decouple configuration types. It moves DB configuration structures into the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
server/platform/http/errors.go (1)
199-218: Centralizing NotFound/AlreadyExists interfaces and IsNotFound helper looks goodDefining
NotFoundError/AlreadyExistsErrorhere and addingIsNotFoundgives a clear, shared contract for not-found semantics and mirrors the existingIsForeignKeypattern.If you anticipate similar call-site ergonomics for “already exists” conditions, consider adding a small helper:
func IsExists(err error) bool { var aee AlreadyExistsError if errors.As(err, &aee) { return aee.IsExists() } return false }This would keep the API symmetric and allow fleet-level aliases the same way
IsNotFoundis exposed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
cmd/fleet/serve.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/common_mysql/arch_test.goserver/datastore/mysql/common_mysql/common.goserver/datastore/mysql/common_mysql/config.goserver/datastore/mysql/common_mysql/errors.goserver/datastore/mysql/common_mysql/list_options.goserver/datastore/mysql/common_mysql/retry.goserver/datastore/mysql/common_mysql/testing_utils/testing_utils.goserver/datastore/mysql/common_mysql/types.goserver/datastore/mysql/config.goserver/datastore/mysql/mysql.goserver/datastore/mysql/testing_utils.goserver/fleet/app.goserver/fleet/datastore.goserver/fleet/db.goserver/fleet/errors.goserver/platform/http/errors.gotools/mysql-tests/rds/iam_auth.go
💤 Files with no reviewable changes (1)
- server/fleet/db.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/datastore/mysql/common_mysql/types.goserver/platform/http/errors.goserver/datastore/mysql/common_mysql/retry.goserver/datastore/mysql/common_mysql/config.goserver/datastore/mysql/common_mysql/arch_test.goserver/fleet/errors.goserver/datastore/mysql/config.goserver/fleet/datastore.goserver/datastore/mysql/testing_utils.goserver/datastore/mysql/common_mysql/common.gocmd/fleet/serve.goserver/datastore/mysql/common_mysql/testing_utils/testing_utils.gotools/mysql-tests/rds/iam_auth.goserver/fleet/app.goserver/datastore/mysql/common_mysql/list_options.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/common_mysql/errors.goserver/datastore/mysql/mysql.go
🧠 Learnings (8)
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/datastore/mysql/common_mysql/retry.goserver/datastore/mysql/common_mysql/arch_test.goserver/datastore/mysql/common_mysql/common.gocmd/fleet/serve.goserver/datastore/mysql/common_mysql/testing_utils/testing_utils.gotools/mysql-tests/rds/iam_auth.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/datastore/mysql/common_mysql/retry.goserver/datastore/mysql/common_mysql/arch_test.goserver/datastore/mysql/common_mysql/common.gocmd/fleet/serve.goserver/datastore/mysql/common_mysql/testing_utils/testing_utils.go
📚 Learning: 2025-07-08T16:12:48.797Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Applied to files:
server/datastore/mysql/common_mysql/retry.goserver/datastore/mysql/apple_mdm.go
📚 Learning: 2025-08-13T18:20:42.136Z
Learnt from: titanous
Repo: fleetdm/fleet PR: 31075
File: tools/redis-tests/elasticache/iam_auth.go:4-10
Timestamp: 2025-08-13T18:20:42.136Z
Learning: For test harnesses and CLI tools in the Fleet codebase, resource cleanup on error paths (like closing connections before log.Fatalf) may not be necessary since the OS handles cleanup when the process exits. These tools prioritize simplicity over defensive programming patterns used in production code.
Applied to files:
server/datastore/mysql/common_mysql/retry.goserver/datastore/mysql/common_mysql/testing_utils/testing_utils.go
📚 Learning: 2025-08-20T21:24:59.261Z
Learnt from: sgress454
Repo: fleetdm/fleet PR: 31075
File: server/datastore/mysql/common_mysql/aws_iam_auth.go:111-130
Timestamp: 2025-08-20T21:24:59.261Z
Learning: In Fleet's MySQL IAM authentication implementation, the TLS and AllowCleartextPasswords configuration is handled in the generateMysqlConnectionString function in server/datastore/mysql/common_mysql/common.go, not in the awsIAMAuthConnector.Connect method. The connector receives a base DSN that already includes the necessary IAM authentication parameters.
Applied to files:
server/datastore/mysql/testing_utils.goserver/datastore/mysql/common_mysql/common.gocmd/fleet/serve.gotools/mysql-tests/rds/iam_auth.go
📚 Learning: 2025-12-11T23:02:59.284Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36978
File: server/mdm/android/service/profiles.go:441-449
Timestamp: 2025-12-11T23:02:59.284Z
Learning: Fleet’s server/mdm/android/service.Service has fields ds (type fleet.AndroidDatastore) and fleetDS (type fleet.Datastore). The fleet.Datastore interface embeds AndroidDatastore (server/fleet/datastore.go), so assigning r.DS (a fleet.Datastore) to Service.ds is valid in Go due to interface-to-interface assignment using the dynamic type.
Applied to files:
cmd/fleet/serve.gotools/mysql-tests/rds/iam_auth.goserver/datastore/mysql/apple_mdm.goserver/datastore/mysql/mysql.go
📚 Learning: 2025-08-20T21:24:59.261Z
Learnt from: sgress454
Repo: fleetdm/fleet PR: 31075
File: server/datastore/mysql/common_mysql/aws_iam_auth.go:111-130
Timestamp: 2025-08-20T21:24:59.261Z
Learning: In Fleet's MySQL IAM authentication implementation, the TLS and AllowCleartextPasswords configuration is handled in the generateMysqlConnectionString function in server/datastore/mysql/common_mysql/common.go. When no password is configured and the endpoint is an RDS endpoint, the function automatically sets allowCleartextPasswords=true and configures appropriate TLS settings. The awsIAMAuthConnector receives a properly configured base DSN and only needs to inject the IAM token as the password.
Applied to files:
tools/mysql-tests/rds/iam_auth.go
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/datastore/mysql/apple_mdm.go
🧬 Code graph analysis (12)
server/platform/http/errors.go (2)
server/fleet/datastore.go (2)
IsNotFound(2823-2823)NotFoundError(2820-2820)server/datastore/mysql/common_mysql/errors.go (1)
NotFoundError(10-15)
server/datastore/mysql/common_mysql/retry.go (1)
server/datastore/mysql/common_mysql/types.go (1)
DBReadTx(7-12)
server/datastore/mysql/common_mysql/config.go (1)
server/datastore/mysql/config.go (2)
SQLMode(72-77)TracingEnabled(54-59)
server/datastore/mysql/common_mysql/arch_test.go (2)
server/archtest/archtest.go (2)
ModuleName(34-34)NewPackageTest(43-45)server/ptr/ptr.go (1)
T(86-88)
server/datastore/mysql/testing_utils.go (1)
server/datastore/mysql/common_mysql/testing_utils/testing_utils.go (1)
MysqlTestConfig(167-174)
server/datastore/mysql/common_mysql/common.go (3)
server/datastore/mysql/common_mysql/config.go (2)
MysqlConfig(6-23)LoggingConfig(27-30)server/datastore/mysql/config.go (1)
Replica(37-42)server/datastore/mysql/mysql.go (1)
NewDB(383-385)
cmd/fleet/serve.go (1)
server/datastore/mysql/mysql.go (2)
NewDBConnections(233-284)NewDatastore(288-306)
server/fleet/app.go (1)
server/datastore/mysql/common_mysql/list_options.go (1)
ListOptions(15-24)
server/datastore/mysql/common_mysql/list_options.go (2)
server/fleet/app.go (1)
ListOptions(1253-1277)server/variables/variables.go (1)
Contains(104-106)
server/datastore/mysql/apple_mdm.go (1)
server/datastore/mysql/common_mysql/types.go (1)
DBReadTx(7-12)
server/datastore/mysql/common_mysql/errors.go (2)
server/fleet/datastore.go (1)
NotFoundError(2820-2820)server/platform/http/errors.go (1)
NotFoundError(200-203)
server/datastore/mysql/mysql.go (5)
server/datastore/mysql/common_mysql/config.go (2)
MysqlConfig(6-23)LoggingConfig(27-30)server/datastore/mysql/config.go (5)
DBOption(18-18)Replica(37-42)Logger(21-26)SQLMode(72-77)TracingEnabled(54-59)server/datastore/mysql/common_mysql/common.go (2)
DBConnections(48-52)NewDB(54-110)server/datastore/mysql/android_mysql.go (1)
NewAndroidDatastore(23-29)server/datastore/mysql/common_mysql/list_options.go (3)
SanitizeColumn(28-42)ListOptions(15-24)AppendListOptionsWithParams(53-106)
🔇 Additional comments (25)
server/datastore/mysql/common_mysql/types.go (1)
5-12: DBReadTx interface shape looks appropriate for read-only transactionsThe new
DBReadTxcleanly constrains read paths toQueryerContext/PreparerContextplusRebind, which matches howsqlx.SelectContextandsqlx.Inare used in read-only helpers.server/datastore/mysql/apple_mdm.go (1)
3251-3335: Switch to common_mysql.DBReadTx + withReadTx is consistent and beneficialUsing
ds.withReadTxwith the newcommon_mysql.DBReadTxfor the ToInstall, ToRemove, and combined helpers keeps these paths explicitly read-only while remaining compatible withsqlx.SelectContext. Running the combined install/remove queries inside a single read transaction inListMDMAppleProfilesToInstallAndRemovealso improves snapshot consistency.tools/mysql-tests/rds/iam_auth.go (1)
11-17: RDS IAM test now correctly uses mysql.NewDB entrypointSwitching the tool to
mysql.NewDB(mysqlConfig, dbOpts)(instead ofcommon_mysql.NewDB) aligns it with the main datastore initialization path, so IAM auth, TLS, andallowCleartextPasswordshandling are all driven by the same DSN-generation logic as in production.Also applies to: 65-67
server/datastore/mysql/config.go (1)
36-59: The concern aboutTracingEnabledis unfounded—toCommonLoggingConfig()already contains a nil check and returns nil safely. Additionally, bothReplica()andTracingEnabled()are only called fromcmd/fleet/serve.gowith conditional guards that ensure non-nil configs are passed (e.g.,Replica()is only called whenconfig.MysqlReadReplica.Address != ""). Adding defensive nil checks at the function level is unnecessary given the actual call patterns in the codebase.Likely an incorrect or invalid review comment.
cmd/fleet/serve.go (1)
246-252: LGTM! Clean refactoring to two-step initialization.The separation of database connection creation (
NewDBConnections) from datastore initialization (NewDatastore) improves modularity and enables connection sharing across datastores. Error handling is properly implemented at both stages.server/fleet/app.go (1)
1287-1306: LGTM! Correct implementation of the ListOptions interface.The interface methods properly expose the ListOptions fields to satisfy the
common_mysql.ListOptionsinterface. TheGetPerPage()method appropriately returnsDefaultPerPagewhenPerPageis 0, maintaining backward compatibility.server/datastore/mysql/common_mysql/list_options.go (2)
28-42: LGTM! Proper column sanitization.The
SanitizeColumnfunction correctly sanitizes column names by removing invalid characters and wrapping components in backticks for SQL safety. The handling of dotted names (e.g.,table.column) is appropriate.
78-106: LGTM! ORDER BY, LIMIT, and OFFSET clauses are correctly implemented.The logic properly handles:
- Primary order key with direction
- Optional secondary order key (for deterministic sorting in tests)
- LIMIT with optional +1 for pagination metadata
- OFFSET calculation based on page number
server/datastore/mysql/common_mysql/retry.go (1)
22-22: LGTM! Type reference updated for common_mysql refactoring.The change from
fleet.DBReadTxtoDBReadTxaligns with the PR objective of moving shared database types to thecommon_mysqlpackage. The interface contract remains the same.server/datastore/mysql/common_mysql/testing_utils/testing_utils.go (1)
167-174: LGTM! Test config updated for common_mysql refactoring.The function now returns
*common_mysql.MysqlConfiginstead of the old config type, and includes theAddressfield for complete test configuration. This aligns with the PR's objective of consolidating MySQL configuration in thecommon_mysqlpackage.server/datastore/mysql/testing_utils.go (2)
44-45: LGTM! Clean conversion pattern.The refactor appropriately uses the new
common_mysql.MysqlConfigtype from test utilities and converts to the legacyconfig.MysqlConfigviafromCommonMysqlConfig. This maintains backward compatibility while introducing the decoupled configuration types.
358-358: LGTM! Appropriate use of conversion helper.The conversion from
config.MysqlConfigtocommon_mysql.MysqlConfigviatoCommonMysqlConfigaligns with the refactoring to store the decoupled config type in the datastore.server/datastore/mysql/common_mysql/errors.go (1)
7-7: LGTM! Proper decoupling from fleet package.The refactor correctly updates the compile-time interface assertion to reference
platform_http.NotFoundErrorinstead offleet.NotFoundError. This aligns with the PR's objective to decouplecommon_mysqlfrom heavier dependencies while maintaining the same interface contract.Also applies to: 18-18
server/datastore/mysql/common_mysql/arch_test.go (1)
14-28: LGTM! Excellent architectural boundary enforcement.This test effectively validates the PR's objective to decouple
common_mysqlfrom other Fleet domain packages. The allowed dependencies (platform/http,contexts/ctxerr) are appropriately lightweight, and the test will catch any future violations of the architectural boundaries.server/fleet/errors.go (1)
513-514: LGTM! Consistent with error aliasing pattern.The replacement of the
Errorerinterface with a type alias toplatform_http.Erroreris consistent with other error type aliases in this file (e.g.,ErrWithInternal,ErrWithLogFields). This maintains backward compatibility while centralizing error definitions in the platform layer.server/datastore/mysql/common_mysql/config.go (2)
6-23: LGTM! Well-designed decoupled configuration struct.The
MysqlConfigstruct appropriately captures all necessary MySQL connection configuration while avoiding heavy dependencies on AWS SDK, viper, etc. The fields cover connection basics, TLS settings, connection pool configuration, and other MySQL-specific options. The comment clearly explains the decoupling rationale.
27-30: LGTM! Minimal and focused logging configuration.The
LoggingConfigstruct appropriately captures the essential logging configuration fields (TracingEnabled,TracingType) needed by the database layer without pulling in heavier dependencies. This maintains the decoupling objective.server/fleet/datastore.go (1)
2819-2827: Unifying datastore errors viaplatform_httpaliases looks goodAliasing
NotFoundError,IsNotFound, andAlreadyExistsErrortoplatform_httpkeeps the public Fleet API intact while centralizing error semantics in one place. This is a clean consolidation with no visible behavior change in callers.server/datastore/mysql/common_mysql/common.go (2)
30-43: DecouplingDBOptionsfromserver/configtypes is appropriateSwitching
ReplicaConfigto*MysqlConfigandTracingConfigto*LoggingConfigkeepscommon_mysqllow‑dependency and aligns it with the new shared config types without changing behavior.
45-52: DBConnections + NewDB refactor preserves behavior while enabling shared connectionsThe new
DBConnectionsstruct and the updatedNewDB/generateMysqlConnectionStringusingMysqlConfigcleanly support sharing primary/replica connections across bounded‑context datastores. Tracing selection, interceptor handling, IAM connector usage, and DSN construction (including SQL mode and TLS/allowCleartextPasswordsfor IAM) are preserved in their existing locations, which is consistent with how IAM auth is expected to work in this codebase. Based on learnings, this keeps TLS/cleartext behavior correctly in the DSN layer rather than the connector.Also applies to: 54-110, 114-152
server/datastore/mysql/mysql.go (5)
73-75: SwitchingreadReplicaConfigto*common_mysql.MysqlConfigis consistent
readReplicaConfigis only used as a presence flag inHealthCheckandClose, so changing its type to the sharedcommon_mysql.MysqlConfigkeeps it aligned with the new config layer without changing runtime behavior.
230-284:NewDBConnectionswiring for primary/replica and IAM auth looks sound
NewDBConnectionsnow:
- Builds
common_mysql.DBOptionswith sane defaults.- Applies
DBOptionsetters.- Runs
checkConfigon primary and (converted) replica configs.- Applies
setupIAMAuthIfNeededseparately for primary and replica.- Constructs writer/reader via the local
NewDBwrapper.This keeps all password/TLS file handling and IAM connector setup in the mysql package while delegating the actual dialing to
common_mysql.NewDB, which is a clean separation.
286-315:NewDatastoreandNewcorrectly leverage sharedDBConnectionsCreating a
DatastorefromDBConnectionsand delegatingNewtoNewDBConnections→NewDatastorelets multiple bounded-context datastores share the same underlying connections. Logger, minLastOpenedAtDiff, serverPrivateKey, and Android datastore wiring are all pulled from the shared options, which keeps configuration centralized and avoids duplication.
383-443: Config/logging conversion helpers cleanly bridgeconfig↔common_mysqlThe new
NewDBwrapper andtoCommonMysqlConfig/fromCommonMysqlConfig/toCommonLoggingConfigfunctions copy all the relevant fields betweenconfig.MysqlConfig/LoggingConfigandcommon_mysqlequivalents. This keeps the common layer independent ofserver/configwhile preserving behavior (including TLS/SQLMode/Region and pool settings) for callers still usingconfig.MysqlConfig.
830-847: List-options and column sanitization facades correctly centralize behaviorThe new
sanitizeColumn,appendListOptionsToSQL, andappendListOptionsWithCursorToSQLwrappers delegate tocommon_mysqlhelpers and standardizePerPagedefaulting viafleet.DefaultPerPage. This reduces duplication while keeping SQL-ordering and pagination behavior in one place; the explicit comment about mutatingopts.PerPagedocuments the only side-effect.
| if cursor := opts.GetCursorValue(); cursor != "" && orderKey != "" { | ||
| cursorSQL := " WHERE " | ||
| if strings.Contains(strings.ToLower(sql), "where") { | ||
| cursorSQL = " AND " | ||
| } | ||
| if strings.HasSuffix(orderKey, "id") { | ||
| i, _ := strconv.Atoi(cursor) | ||
| params = append(params, i) | ||
| } else { | ||
| params = append(params, cursor) | ||
| } | ||
| direction := ">" // ASC | ||
| if opts.IsDescending() { | ||
| direction = "<" // DESC | ||
| } | ||
| sql = fmt.Sprintf("%s %s %s %s ?", sql, cursorSQL, orderKey, direction) | ||
|
|
||
| // Cursor-based pagination supersedes page-based pagination | ||
| page = 0 | ||
| } |
There was a problem hiding this comment.
Critical logic error in cursor pagination for ID columns.
The check at line 62 (strings.HasSuffix(orderKey, "id")) will never match because orderKey has already been sanitized at line 54, which wraps it in backticks. For example:
- Input:
"host_id" - After
SanitizeColumn:"`host_id`" strings.HasSuffix("host_id", "id")returnsfalse(ends with"`")
This means ID columns are never converted to integers (line 63), potentially causing type mismatches in the cursor pagination WHERE clause.
🔎 Proposed fix
Check the original (unsanitized) order key before sanitization:
func AppendListOptionsWithParams(sql string, params []any, opts ListOptions) (string, []any) {
- orderKey := SanitizeColumn(opts.GetOrderKey())
+ origOrderKey := opts.GetOrderKey()
+ orderKey := SanitizeColumn(origOrderKey)
page := opts.GetPage()
if cursor := opts.GetCursorValue(); cursor != "" && orderKey != "" {
cursorSQL := " WHERE "
if strings.Contains(strings.ToLower(sql), "where") {
cursorSQL = " AND "
}
- if strings.HasSuffix(orderKey, "id") {
+ if strings.HasSuffix(origOrderKey, "id") {
i, _ := strconv.Atoi(cursor)
params = append(params, i)
} else {
params = append(params, cursor)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/datastore/mysql/common_mysql/list_options.go around lines 57 to 76,
the suffix check uses the already-sanitized orderKey (with backticks) so
HasSuffix(..., "id") never matches; restore or preserve the unsanitized column
name (e.g., keep originalOrderKey before calling SanitizeColumn or strip
surrounding backticks) and use that unsanitized name for the strings.HasSuffix
check, converting the cursor to an int and appending it to params when it ends
with "id"; keep the sanitized orderKey for building the SQL and ensure page is
zeroed for cursor-based pagination as before.
There was a problem hiding this comment.
Nice find @coderabbitai
However, this is not a critical logic error since MySQL automatically coerces strings to integers here. So, this is just dead code.
There was a problem hiding this comment.
@getvictor, thank you for the clarification! You're absolutely right—I appreciate the correction. MySQL's automatic type coercion from string to integer in the WHERE clause comparison means the query will work correctly regardless of whether the cursor value is passed as a string or int parameter.
So the integer conversion logic at line 63 is indeed dead code (never executed due to the backtick-wrapped suffix check), but it doesn't cause any functional issues. The string cursor value works fine for ID comparisons thanks to MySQL's implicit type handling.
If you'd like to clean up the dead code for clarity, you could either:
- Remove the suffix check and int conversion entirely (simplest)
- Fix the logic to check the unsanitized column name (as originally suggested)
But given it's not causing issues, it's perfectly reasonable to leave as-is or address in a future cleanup pass.
✏️ Learnings added
Learnt from: getvictor
Repo: fleetdm/fleet PR: 37245
File: server/datastore/mysql/common_mysql/list_options.go:56-73
Timestamp: 2025-12-31T23:23:00.373Z
Learning: In server/datastore/mysql/common_mysql/list_options.go, the strings.HasSuffix(orderKey, "id") check at line 62 never matches because orderKey is already sanitized with backticks, making the integer conversion code at line 63 unreachable dead code. However, this doesn't cause functional issues because MySQL automatically coerces string parameters to integers when comparing with integer columns in WHERE clauses.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: ee/server/service/hostidentity/depot/depot.go:115-115
Timestamp: 2025-07-08T16:11:49.555Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the error from result.RowsAffected() is intentionally ignored because the information is only used for logging purposes, not for critical program logic.
| dbConns, err := mysql.NewDBConnections(config.Mysql, opts...) | ||
| if err != nil { | ||
| initFatal(err, "initializing database connections") | ||
| } | ||
|
|
||
| mds, err := mysql.NewDatastore(dbConns, config.Mysql, clock.C) |
There was a problem hiding this comment.
| dbConns, err := mysql.NewDBConnections(config.Mysql, opts...) | |
| if err != nil { | |
| initFatal(err, "initializing database connections") | |
| } | |
| mds, err := mysql.NewDatastore(dbConns, config.Mysql, clock.C) | |
| mds, err := mysql.New(config.Mysql, clock.C, opts...) |
There was a problem hiding this comment.
It makes sense for this PR, but my plan is to split this for activity bounded context and pass dbConns to create the new bounded context, like here:
Line 1252 in 4297351
So, I'll keep it as is since I'm planning to use it in the next story.
4 participants
Related issue: Resolves #37244
Goal: Make common_mysql package independent of domain packages so it can be reused by future bounded contexts.
Changes made:
The AppendListOptionsToSQL functions previously required fleet.ListOptions directly. Now common_mysql defines its own interface that describes what a list options type must provide (page number, per-page limit, sort order, etc.). The fleet.ListOptions type implements this interface through new getter methods. This lets any bounded context use the SQL helpers without importing the fleet package.
Database-specific error types like IsDuplicate and IsChildForeignKeyError were moved from fleet package to common_mysql where they belong. A new http/errors.go file was created for the HTTP-specific error helpers that remain in the platform layer.
MySQL configuration types and functions were moved to common_mysql/config.go, reducing coupling between packages.
A new arch_test.go file enforces that common_mysql doesn't import domain packages like fleet, preventing future regressions.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.