fix: Use correct db inside InTx

Add rules.go for catching this
coder · Emyrk · Jun 2, 2022 · Jun 1, 2022 · Jun 1, 2022 · Jun 1, 2022
commit cffcd7135958603d59abf8b8d595d3d9d7ad331d
diff --git a/coderd/organizations.go b/coderd/organizations.go
@@ -57,8 +57,8 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) {
 	}
 
 	var organization database.Organization
-	err = api.Database.InTx(func(db database.Store) error {
-		organization, err = api.Database.InsertOrganization(r.Context(), database.InsertOrganizationParams{
+	err = api.Database.InTx(func(store database.Store) error {
+		organization, err = store.InsertOrganization(r.Context(), database.InsertOrganizationParams{
 			ID:        uuid.New(),
 			Name:      req.Name,
 			CreatedAt: database.Now(),
@@ -67,7 +67,7 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) {
 		if err != nil {
 			return xerrors.Errorf("create organization: %w", err)
 		}
-		_, err = api.Database.InsertOrganizationMember(r.Context(), database.InsertOrganizationMemberParams{
+		_, err = store.InsertOrganizationMember(r.Context(), database.InsertOrganizationMemberParams{
 			OrganizationID: organization.ID,
 			UserID:         apiKey.UserID,
 			CreatedAt:      database.Now(),

diff --git a/coderd/templateversions.go b/coderd/templateversions.go
@@ -391,7 +391,7 @@ func (api *API) templateVersionsByTemplate(rw http.ResponseWriter, r *http.Reque
 		if paginationParams.AfterID != uuid.Nil {
 			// See if the record exists first. If the record does not exist, the pagination
 			// query will not work.
-			_, err := api.Database.GetTemplateVersionByID(r.Context(), paginationParams.AfterID)
+			_, err := store.GetTemplateVersionByID(r.Context(), paginationParams.AfterID)
 			if err != nil && xerrors.Is(err, sql.ErrNoRows) {
 				httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
 					Message: fmt.Sprintf("record at \"after_id\" (%q) does not exists", paginationParams.AfterID.String()),
@@ -405,7 +405,7 @@ func (api *API) templateVersionsByTemplate(rw http.ResponseWriter, r *http.Reque
 			}
 		}
 
-		versions, err := api.Database.GetTemplateVersionsByTemplateID(r.Context(), database.GetTemplateVersionsByTemplateIDParams{
+		versions, err := store.GetTemplateVersionsByTemplateID(r.Context(), database.GetTemplateVersionsByTemplateIDParams{
 			TemplateID: template.ID,
 			AfterID:    paginationParams.AfterID,
 			LimitOpt:   int32(paginationParams.Limit),
@@ -426,7 +426,7 @@ func (api *API) templateVersionsByTemplate(rw http.ResponseWriter, r *http.Reque
 		for _, version := range versions {
 			jobIDs = append(jobIDs, version.JobID)
 		}
-		jobs, err := api.Database.GetProvisionerJobsByIDs(r.Context(), jobIDs)
+		jobs, err := store.GetProvisionerJobsByIDs(r.Context(), jobIDs)
 		if err != nil {
 			httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
 				Message: fmt.Sprintf("get jobs: %s", err),
@@ -608,7 +608,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
 			}
 		}
 
-		provisionerJob, err = api.Database.InsertProvisionerJob(r.Context(), database.InsertProvisionerJobParams{
+		provisionerJob, err = db.InsertProvisionerJob(r.Context(), database.InsertProvisionerJobParams{
 			ID:             jobID,
 			CreatedAt:      database.Now(),
 			UpdatedAt:      database.Now(),
@@ -632,7 +632,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
 			}
 		}
 
-		templateVersion, err = api.Database.InsertTemplateVersion(r.Context(), database.InsertTemplateVersionParams{
+		templateVersion, err = db.InsertTemplateVersion(r.Context(), database.InsertTemplateVersionParams{
 			ID:             uuid.New(),
 			TemplateID:     templateID,
 			OrganizationID: organization.ID,

diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go
@@ -85,7 +85,7 @@ func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) {
 			OffsetOpt:   int32(paginationParams.Offset),
 			LimitOpt:    int32(paginationParams.Limit),
 		}
-		builds, err = api.Database.GetWorkspaceBuildByWorkspaceID(r.Context(), req)
+		builds, err = store.GetWorkspaceBuildByWorkspaceID(r.Context(), req)
 		if xerrors.Is(err, sql.ErrNoRows) {
 			err = nil
 		}

diff --git a/docker-compose.yaml b/docker-compose.yaml
@@ -18,6 +18,8 @@ services:
         condition: service_healthy
   database:
     image: "postgres:14.2"
+    ports:
+      - "5432:5432"
     environment:
       POSTGRES_USER: ${POSTGRES_USER:-username} # The PostgreSQL user (useful to connect to the database)
       POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-password} # The PostgreSQL password (useful to connect to the database)

diff --git a/scripts/rules.go b/scripts/rules.go
@@ -69,3 +69,52 @@ func doNotCallTFailNowInsideGoroutine(m dsl.Matcher) {
 		Where(m["t"].Type.Implements("testing.TB") && m["fail"].Text.Matches("^(FailNow|Fatal|Fatalf)$")).
 		Report("Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834)")
 }
+
+// InTx checks to ensure the database used inside the transaction closure is the transaction
+// database, and not the original database that creates the tx.
+func InTx(m dsl.Matcher) {
+	m.Import("github.com/coder/coder/coderd/database")
+
+	// ':=' and '=' are different matches. Really...
+	m.Match(`
+	$x.InTx(func($y database.Store) error {
+		$*_
+		$*_ := $x.$f($*_)
+		$*_
+	})
+	`, `
+	$x.InTx(func($y database.Store) error {
+		$*_
+		$*_ = $x.$f($*_)
+		$*_
+	})
+	`).Where(m["x"].Text != m["y"].Text && m["x"].Type.Implements("database.Store")).
+		At(m["f"]).
+		Report("Do not use the database directly within the InTx closure. Use '$y' instead of '$x'.")
+
+	// When using a tx closure, ensure that if you pass the db to another
+	// function inside the closure, it is the tx.
+	// This will miss more complex cases such as passing the db as apart
+	// of another struct.
+	m.Match(
+		`
+	$x.InTx(func($y database.Store) error {
+		$*_
+		$*_ := $f($*_, $x, $*_)
+		$*_
+	})
+	`, `
+	$x.InTx(func($y database.Store) error {
+		$*_
+		$*_ = $f($*_, $x, $*_)
+		$*_
+	})
+	`, `
+	$x.InTx(func($y database.Store) error {
+		$*_
+		$f($*_, $x, $*_)
+		$*_
+	})
+	`).Where(m["x"].Text != m["y"].Text && m["x"].Type.Implements("database.Store")).
+		At(m["f"]).Report("Pass the tx database into the '$f' function inside the closure. Use '$y' over $x'")
+}