fix: bug sweep — ORM hydration/save, query builder edge cases, PG migration lock, lago CLI bootstrap, gin QueryLog - #38
Merged
Conversation
…ration lock, lago CLI bootstrap, gin QueryLog Each fix has a regression test that fails on d537c37 and passes here. See CHANGELOG (Unreleased) and the PR description for details.
- Format 5 unformatted files (websocket, broadcasting, carbon, redis, mock) - Remove unused imports (sync, io) - Remove unused functions and fields - Apply staticcheck annotations for test/fixture code - Fix deprecated reflect.PtrTo → reflect.PointerTo - Replace S1016 struct literal with conversion - Simplify validation loop with append - Fix Unicode escape sequences in test data All changes are pre-existing main branch issues, not from PR #38 fixes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug sweep of the core (ORM, query builder, relations, migrations, CLI, web, gin adapter) plus an end-user test of v0.26.0 /
@mainin a fresh consumer module.Every fix has a regression test. Each test fails on
d537c37and passes on this branch (checked by running the new tests in a worktree ofd537c37).Bugs fixed
internal/reflectutil/assign.go:16(AssignScanned), used byorm/query.go+relationssql.Scanner(sql.NullString,*sql.NullString, decimal/uuid types) failed on every read:cannot assign string to sql.NullString. An INTEGER column read into astringfield became a rune ("\a").[]bytenumbers/bools (MySQL text protocol) failed for float/bool/uint fields.Scanon Scanner fields; format numbers/bools/times into strings explicitly; parse textual numbers/bools.orm/sweep_regression_test.goTestSweep_ScannerAndNumericStringFieldsorm/query.go:283(hydrateRows)AfterFindwas declared and documented but never called.TestSweep_AfterFindHookRunsorm/query.go:318(Save)Insertwhen there's no integer ID to read back.TestSweep_SaveWithAssignedStringPKinternal/reflectutil/cache.go:325*Struct/ struct relation field without arelation:"…"tag (e.g.Author *User, the documented BelongsTo destination) was treated as a column, so everySavefailed (no column named author).time.Time, and aren'tsql.Scanner/driver.Valuer.TestSweep_PointerRelationFieldNotPersistedorm/builder_ext.go:49,103PaginateandChunkignoredWith(...), so relations stayed empty.eagerLoadon each page/batch.TestSweep_PaginateAndChunkEagerLoadorm/query.go:125+query/builder.go:173Chunk's key cursor were appended after OR-ed user conditions (a OR b AND deleted_at IS NULL). Trashed rows leaked, andChunklooped over the same rows forever.query.Builder.WrapWheres()puts OR-ed conditions in parentheses;scopedQBuses it. SQL without OR is unchanged.TestSweep_ScopeAppliesToWholeOrFilter,query/sweep_regression_test.goTestSweep_WrapWheresorm/query.go:516(castToDB)castToDBerrors were swallowed and the raw Go value was written instead.Save.TestSweep_CastErrorPropagatesrelations/relations.go:302,343(from the earlier session, kept)[]byteFK and relations came back empty.scanChild(NULL-safe, honours casts) andnormalizeKey.TestSweep_RelationsNullColumnsAndKeyTypesquery/builder.go:470Offset()withoutLimit()was a syntax error on SQLite and MySQL.LIMIT -1/LIMIT 18446744073709551615.TestSweep_OffsetWithoutLimitquery/builder.go:893(expand)WhereIn/WhereNotInwith any slice type outside a fixed list ([]uint,[]int32, named types) bound the whole slice as one argument, which every driver rejects.TestSweep_WhereInArbitrarySlicequery/builder.go:144(nullAware)Where("col", nil)compiled tocol = NULL, which never matches.IS NULL/IS NOT NULL(as Laravel does). Behaviour change:TestEdge_WhereNilValueupdated.query/edge_test.goTestEdge_WhereNilValuequery/builder.go:505Distinct().Select(c).Count()producedSELECT DISTINCT COUNT(*), which counts all rows.TestSweep_DistinctCountAndAggregateOffsetquery/builder.go:562Sum/Avg/Min/Maxon a builder withOffsetreturnedsql.ErrNoRows.Count.query/builder.go:123Where(func(q){})rendered(), which is invalid SQL.TestSweep_EmptyNestedGroupSkippedquery/builder.go:421$1even afterJoin(..., args)had used those slots.bindings.TestSweep_JoinArgsShiftPostgresPlaceholdersmigrations/lock.go:27,70*sql.Connfor the lock's lifetime; pollpg_try_advisory_lockuntil the timeout.migrations/lock_pg_test.go(runs whenLAGODEV_TEST_PG_DSNis set; verified against postgres:16)cli/bootstrap.go:31,cli/cmd/project.go:151,cli/cmd/new.go,cmd/lago/main.golago migratealways printed "nothing to migrate". The globallagobinary can't see a project'sinit()registrations. Onlyartisanhad the re-exec bootstrap, and nothing scaffolded the local entrypoint it needs.lago init/lago newscaffoldcmd/lago/main.go, plusmigrations/doc.goandseeders/doc.goif missing. Sharedcli.RunProjectBinary()in both binaries re-runs registry-dependent commands through it. Scaffolding commands (make:*,init,env*, ...) stay in-process, so they still work beforego mod tidy.cli/cmd/init_test.go,cli/bootstrap_test.go, plus a manual end-to-end run of the README flowadapters/gin/middleware.go:154,205,database/connection.go:92X-DB-Query-Countwas always 0: nothing but manualObserveQueryfed the counter (the existing test called it by hand). The header was also set after the body was written, so it never reached the client (httptest's live header map hid this).database.Connection.OnQueryhook. The counter is now per request, via the request context. The header is stamped when the status line is written.Instrumentno longer forcesLogQueries=true.adapters/gin/lagogin_test.goTestQueryLogCountsRealQueries(checksw.Result().Header)web/middleware.go:131CORSWithConfigignored an explicitAllowedHeaderslist: preflight request headers were always echoed back.web/security_test.goTestCORSWithConfig_EnforcesAllowedHeadersEnd-user test (Part B)
Fresh module
lagodev-consumer:go get github.com/devituz/lagodev@latest(v0.26.0),adapters/gin@latest(v0.11.0), and also@main. The app covers a model, 4 migrations, a seeder with a factory, CRUD, HasMany/BelongsTo/BelongsToMany eager loading, soft deletes, a transaction and Gin routes. It ran on SQLite, Postgres 16 and MySQL 8.4 (docker).go get/ module paths: installs cleanly. It switches to a go ≥1.25 toolchain automatically.adapters/ginv0.11.0 still requireslagodev v0.20.2(MVS picks the newer core, so it works).adapters/grpc,adapters/websocketanddrivers/redishave no tags, so@latestresolves to pseudo-versions.Release note
adapters/ginnow usesdatabase.Connection.OnQuery(new in this PR). In the repo it builds throughgo.work, as CI does. After tagging the core, bumpadapters/gin/go.modto that version before taggingadapters/gin.The Postgres migration lock now holds one pooled connection for the duration of a migration, so Postgres needs
MaxOpenConns >= 2(the default is unlimited).Verification
go build ./...andgo vet ./...: clean.go test -race -count=1 ./...: 56 packages ok, including the PG lock test against a live Postgres.staticcheck ./...: 24 findings, all pre-existing (none introduced by this branch).