-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Prune unnecessary nullzeros, fixup db tags #200
Prune unnecessary nullzeros, fixup db tags #200
Conversation
Signed-off-by: kim (grufwub) <[email protected]>
Signed-off-by: kim (grufwub) <[email protected]>
Signed-off-by: kim (grufwub) <[email protected]>
Signed-off-by: kim (grufwub) <[email protected]>
internal/gtsmodel/account.go
Outdated
@@ -29,7 +29,7 @@ import ( | |||
|
|||
// Account represents either a local or a remote fediverse account, gotosocial or otherwise (mastodon, pleroma, etc). | |||
type Account struct { | |||
ID string `validate:"required,ulid" bun:"type:CHAR(26),pk,nullzero,notnull,unique"` // id of this item in the database | |||
ID string `validate:"required,ulid" bun:"type:CHAR(26),pk,notnull,unique"` // id of this item in the database |
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 issue i found with these char(26)
values is that if you set zero, and you don't have nullzero set, it will actually just set an empty string of length 26, which totally boobs up some of our logic elsewhere when we're checking for if x == ""
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 didn't actually intend to set this one, I think it should be the only example of the ID ones being set like this 😅. Nice catch, will fix
AlsoKnownAs string `validate:"omitempty,ulid" bun:"type:CHAR(26),nullzero"` // This account is associated with x account id | ||
Note string `validate:"-" bun:""` // A note that this account has on their profile (ie., the account's bio/description of themselves) | ||
Memorial bool `validate:"-" bun:",default:false"` // Is this a memorial account, ie., has the user passed away? | ||
AlsoKnownAs string `validate:"omitempty,ulid" bun:"type:CHAR(26),nullzero"` // This account is associated with x account id (TODO: migrate to be AlsoKnownAsID) |
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.
good idea re: migration
@@ -49,15 +49,15 @@ type MediaAttachment struct { | |||
type File struct { | |||
Path string `validate:"required,file" bun:",nullzero,notnull"` // Path of the file in storage. | |||
ContentType string `validate:"required" bun:",nullzero,notnull"` // MIME content type of the file. | |||
FileSize int `validate:"required" bun:",nullzero,notnull"` // File size in bytes | |||
FileSize int `validate:"required" bun:",notnull"` // File size in bytes |
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.
if this one is notnull, i think it should also be nullzero to avoid setting a filesize of 0
in the db
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.
My thinking was that a zero size file is technically valid. It would be an odd situation to have, but it saves running into weird database errors when it's a totally valid file on disk
UpdatedAt time.Time `validate:"-" bun:"type:timestamp,nullzero,notnull,default:current_timestamp"` // When was the file last updated. | ||
} | ||
|
||
// Thumbnail refers to a small image thumbnail derived from a larger image, video, or audio file. | ||
type Thumbnail struct { | ||
Path string `validate:"required,file" bun:",nullzero,notnull"` // Path of the file in storage. | ||
ContentType string `validate:"required" bun:",nullzero,notnull"` // MIME content type of the file. | ||
FileSize int `validate:"required" bun:",nullzero,notnull"` // File size in bytes | ||
FileSize int `validate:"required" bun:",notnull"` // File size in bytes |
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.
same comment as above
@@ -29,7 +29,7 @@ type Status struct { | |||
UpdatedAt time.Time `validate:"-" bun:"type:timestamp,nullzero,notnull,default:current_timestamp"` // when was item last updated | |||
URI string `validate:"required,url" bun:",unique,nullzero,notnull"` // activitypub URI of this status | |||
URL string `validate:"url" bun:",nullzero"` // web url for viewing this status | |||
Content string `validate:"-" bun:",nullzero"` // content of this status; likely html-formatted but not guaranteed | |||
Content string `validate:"-" bun:""` // content of this status; likely html-formatted but not guaranteed |
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.
what happens with an empty bun tag? can we just remove it?
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 just ended up leaving it because it makes it clear that it's IN the database, i.e. not a bun:"-"
@@ -52,13 +52,13 @@ type Status struct { | |||
BoostOf *Status `validate:"-" bun:"-"` // status that corresponds to boostOfID | |||
BoostOfAccount *Account `validate:"-" bun:"rel:belongs-to"` // account that corresponds to boostOfAccountID | |||
ContentWarning string `validate:"-" bun:",nullzero"` // cw string for this status | |||
Visibility Visibility `validate:"-" bun:",nullzero,notnull"` // visibility entry for this status | |||
Visibility Visibility `validate:"oneof=public unlocked followers_only mutuals_only direct" bun:",nullzero,notnull"` // visibility entry for this status |
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.
good change 👍
Sensitive bool `validate:"-" bun:",notnull,default:false"` // mark the status as sensitive? | ||
Language string `validate:"-" bun:",nullzero"` // what language is this status written in? | ||
CreatedWithApplicationID string `validate:"required_if=Local true,omitempty,ulid" bun:"type:CHAR(26),nullzero"` // Which application was used to create this status? | ||
CreatedWithApplication *Application `validate:"-" bun:"rel:belongs-to"` // application corresponding to createdWithApplicationID | ||
ActivityStreamsType string `validate:"required" bun:",nullzero,notnull"` // What is the activitystreams type of this status? See: https://www.w3.org/TR/activitystreams-vocabulary/#object-types. Will probably almost always be Note but who knows!. | ||
Text string `validate:"-" bun:",nullzero"` // Original text of the status without formatting | ||
Text string `validate:"-" bun:""` // Original text of the status without formatting |
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.
same question as above re: empty tag
Signed-off-by: kim (grufwub) <[email protected]>
Signed-off-by: kim (grufwub) <[email protected]>
No description provided.