-
Notifications
You must be signed in to change notification settings - Fork 937
chore: Update sqlc to v1.16.0 #5788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured out my comments 👍
ae9aa46
to
cc002a1
Compare
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 See 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:
This doesn't fix all issues, though, fixing the issues reported by
|
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// 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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above comment on the function says we do not go into nested structs. Isn't the Nullable
case a nested struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
.
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:
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.