chore: Update sqlc to v1.16.0 #5788

mafredri · 2023-01-19T16:10:39Z

I wanted to use enum validation in #5749 and saw sqlc had added it, so I updated.

On the road, I noticed we've had nullable values that we didn't really treat that way. So I did a migration to change them:

UPDATE parameter_schemas SET default_source_scheme = 'none' WHERE default_source_scheme IS NULL;
ALTER TABLE parameter_schemas ALTER COLUMN default_source_scheme SET NOT NULL;

UPDATE parameter_schemas SET default_destination_scheme = 'none' WHERE default_destination_scheme IS NULL;
ALTER TABLE parameter_schemas ALTER COLUMN default_destination_scheme SET NOT NULL;

I considered adding DEFAULT 'none', but I thought that would be more likely to hide issues. Does this seem like the right solution to that?

Also note the UserStatus change, we were not treating it as nullable before, although the database schema allowed it.

chore: Update sqlc to v1.16.0
chore: Make gen
chore: Fix null issues

Emyrk

Overall this looks ok. I think the userstatus being null is a mistake? But it is my first time looking at that audit log sql and don't have a db atm to try things against.

coderd/audit.go

coderd/database/models.go

Emyrk

Figured out my comments 👍

mafredri · 2023-01-20T11:02:52Z

I noticed invalid inserts in telemetry test (i.e. things that would never work in real postgres), so I went ahead and implemented validation in databasefake.

See d8c1d68 (#5788):

We'll now have some form of validation on inserts in the fake as well. Unfortunately it requires some manual plumbing when creating fake methods:

    telemetry_test.go:72:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:72
        	Error:      	Received unexpected error:
        	            	invalid database type fields:
        	            	    	InsertProvisionerJobParams.Provisioner: invalid ProvisionerType: ""
        	            	    	InsertProvisionerJobParams.StorageMethod: invalid ProvisionerStorageMethod: ""
        	            	    	InsertProvisionerJobParams.Type: invalid ProvisionerJobType: "":
        	            	    github.com/coder/coder/coderd/database/databasefake.validateDatabaseType
        	            	        /home/coder/src/coder/coder/coderd/database/databasefake/databasefake.go:183
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:78:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:78
        	Error:      	Received unexpected error:
        	            	invalid database type fields:
        	            	    	InsertTemplateParams.Provisioner: invalid ProvisionerType: "":
        	            	    github.com/coder/coder/coderd/database/databasefake.validateDatabaseType
        	            	        /home/coder/src/coder/coder/coderd/database/databasefake/databasefake.go:183
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:89:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:89
        	Error:      	Received unexpected error:
        	            	invalid database type fields:
        	            	    	InsertUserParams.LoginType: invalid LoginType: "":
        	            	    github.com/coder/coder/coderd/database/databasefake.validateDatabaseType
        	            	        /home/coder/src/coder/coder/coderd/database/databasefake/databasefake.go:183
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:101:
[...]

This doesn't fix all issues, though, fixing the issues reported by 8d143c0 (#5788) still leaves the following issues when switching to real database:

=== CONT  TestTelemetry/Snapshot
    telemetry_test.go:58:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:58
        	Error:      	Received unexpected error:
        	            	pq: null value in column "ip_address" of relation "api_keys" violates not-null constraint
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:66:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:66
        	Error:      	Received unexpected error:
        	            	pq: insert or update on table "parameter_schemas" violates foreign key constraint "parameter_schemas_job_id_fkey"
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:74:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:74
        	Error:      	Received unexpected error:
        	            	pq: invalid input syntax for type json
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:80:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:80
        	Error:      	Received unexpected error:
        	            	pq: insert or update on table "templates" violates foreign key constraint "templates_organization_id_fkey"
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:85:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:85
        	Error:      	Received unexpected error:
        	            	pq: insert or update on table "template_versions" violates foreign key constraint "template_versions_organization_id_fkey"
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:91:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:91
        	Error:      	Received unexpected error:
        	            	pq: null value in column "rbac_roles" of relation "users" violates not-null constraint
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:96:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:96
        	Error:      	Received unexpected error:
        	            	pq: insert or update on table "workspaces" violates foreign key constraint "workspaces_owner_id_fkey"
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:103:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:103
        	Error:      	Received unexpected error:
        	            	pq: insert or update on table "workspace_apps" violates foreign key constraint "workspace_apps_agent_id_fkey"
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:108:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:108
        	Error:      	Received unexpected error:
        	            	pq: insert or update on table "workspace_agents" violates foreign key constraint "workspace_agents_resource_id_fkey"
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:115:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:115
        	Error:      	Received unexpected error:
        	            	pq: insert or update on table "workspace_builds" violates foreign key constraint "workspace_builds_workspace_id_fkey"
        	Test:       	TestTelemetry/Snapshot
    telemetry_test.go:121:
        	Error Trace:	/home/coder/src/coder/coder/coderd/telemetry/telemetry_test.go:121
        	Error:      	Received unexpected error:
        	            	pq: insert or update on table "workspace_resources" violates foreign key constraint "workspace_resources_job_id_fkey"
        	Test:       	TestTelemetry/Snapshot

mafredri · 2023-01-20T12:29:52Z

coderd/provisionerdserver/provisionerdserver_test.go

+		// Ignore log errors because we get:
+		//
+		//	(*Server).FailJob       audit log - get build {"error": "sql: no rows in result set"}
+		ignoreLogErrors := true
+		srv := setup(t, ignoreLogErrors)


I invite extra scrutiny to this test here! The audit log implementation may not have ideal behavior. Please ensure that we actually shouldn't audit log when a workspace build wasn't created.

johnstcn

👍

Emyrk · 2023-01-20T13:36:33Z

coderd/database/databasefake/databasefake.go

+	// Note: database.Null* types don't have a Valid method, we skip them here
+	// because their embedded types may have a Valid method and we don't want
+	// to bother with checking both that tha Valid field is true and that the
+	// type it embeds validates to true. We would need to check:
+	//
+	//	dbNullEnum.Valid && dbNullEnum.Enum.Valid()
+	if strings.HasPrefix(v.Type().Name(), "Null") {
+		return nil
+	}


The above comment on the function says we do not go into nested structs. Isn't the Nullable case a nested struct?

So this is to do nothing in the following scenario:

validateDatabaseType(database.NullThing{}) // HasPrefix Null

If we have:

validateDatabaseType(database.MyType{Thing: database.NullThing{}})

We ultimately will not validate Thing since we only go one level deep and the validator function doesn't check structs.

This was just to keep the logic (simpler) and not having to look at both database.NullThing.Valid == true && database.NullThing.Thing.Valid().

mafredri requested review from Emyrk, coadler and kylecarbs January 19, 2023 16:10

mafredri self-assigned this Jan 19, 2023

Emyrk reviewed Jan 19, 2023

View reviewed changes

coderd/audit.go Show resolved Hide resolved

coderd/database/models.go Show resolved Hide resolved

Emyrk approved these changes Jan 19, 2023

View reviewed changes

mafredri added 3 commits January 20, 2023 09:10

chore: Update sqlc to v1.16.0

ca90a0b

chore: Make gen

5ac5860

chore: Fix null issues

cc002a1

mafredri force-pushed the mafredri/chore-update-sqlc branch from ae9aa46 to cc002a1 Compare January 20, 2023 09:15

mafredri added 3 commits January 20, 2023 09:18

fixup! chore: Fix null issues

3cbb276

chore: Add enum validation to database fake

d8c1d68

chore: Fix Telemetry test DB inserts

8d143c0

mafredri added 4 commits January 20, 2023 11:05

fixup! chore: Add enum validation to database fake

a1c4e8f

fixup! fixup! chore: Add enum validation to database fake

8f9282b

chore: Fix all tests that skipping enum values

d7dcc25

fix: Use correct err in providionerdserver audit log failure log

bf43352

mafredri commented Jan 20, 2023

View reviewed changes

johnstcn approved these changes Jan 20, 2023

View reviewed changes

Emyrk reviewed Jan 20, 2023

View reviewed changes

Merge branch 'main' into mafredri/chore-update-sqlc

fc9c12f

mafredri merged commit 8afdf24 into main Jan 23, 2023

mafredri deleted the mafredri/chore-update-sqlc branch January 23, 2023 11:14

github-actions bot locked and limited conversation to collaborators Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore: Update sqlc to v1.16.0 #5788

chore: Update sqlc to v1.16.0 #5788

Uh oh!

mafredri commented Jan 19, 2023 •

edited

Loading

Uh oh!

Emyrk left a comment

Uh oh!

Uh oh!

Uh oh!

Emyrk left a comment

Uh oh!

mafredri commented Jan 20, 2023

Uh oh!

mafredri Jan 20, 2023

Uh oh!

johnstcn left a comment

Uh oh!

Emyrk Jan 20, 2023

Uh oh!

mafredri Jan 20, 2023

Uh oh!

Uh oh!

chore: Update sqlc to v1.16.0 #5788

chore: Update sqlc to v1.16.0 #5788

Uh oh!

Conversation

mafredri commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

mafredri commented Jan 20, 2023

Uh oh!

mafredri Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

johnstcn left a comment

Choose a reason for hiding this comment

Uh oh!

Emyrk Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

mafredri Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mafredri commented Jan 19, 2023 •

edited

Loading