Open htdvisser opened 2 years ago
As pointed out by @cvetkovski98 we do have the notnull
annotation which creates the restriction on the tables but at the same time we are not using the nullzero
which marshals the Go zero values into NULL. Ref: https://bun.uptrace.dev/guide/models.html#struct-tags
Besides this I should write it in here what else shouldn't be NULL in the database.
Besides this I should write it in here what else shouldn't be NULL in the database.
Could you take a look for this ? I can then check the production databases for conflicts and we can write the migration and store changes.
The initial idea was to find more columns which would benefit from having more NOT NULL columns. By going through columns which already have the nullzero
restriction we can get a good idea where the addition is applicable. Hylke mentioned that there are other fields which would benefit from this addition but since the current list is extensive enough, we can develop on top of them after agreeing on the changes on the fields with nullzero
.
I went a bit all over the place while working on this, so there are two sections in this comment:
nullzero
but would benefit from having notnull
notnull
but would benefit from having nullzero
Each section is divided in:
Overall most fields which have notnull
or nullzero
either enforce the value not being null via request validation and business logic or the current behavior is adequate. But for those who require the change, imply in a different migration for each, where we would set the default value and add the constraint to the column on the table.
@adriansmares I left the queries for the fields (just counting amount of NULLs) for each fields which I believe might have the NULL or zero value on production. There are some questions regarding some specific fields which I would appreciate your take.
And lastly these lists are useful for asking questions regarding specific fields but their main purpose is also to serve as a record of things to change. This way the issue can be fragmented into smaller PRs. While it these changes have to be merged on a minor release I believe it is important to not attempt to do everything at once, specially considering the overhead generated by a mistake on the identification in one of these columns behavior. Will break the list into sections done by minor releases but I'm posting this in order to have a discussion around the questions that appeared.
This requires a migration, therefore I'm moving it to the v3.29.0
@nicholaspcr: This is completed right?
I don't think it is, I'll double check the places where I previously marked that needed changes and update this issue
Summary
A number of columns in our database schema allow
NULL
values when in factNULL
s shoud not be possible. We should fix that.Current Situation
Many columns (at least the ones marked as
nullzero
inpkg/identityserver/bunstore
, but there are more) allowNULL
values that are equivalent to empty values in The Things Stack.Desired Situation
We should not allow
NULL
s in those columns. We can set aDEFAULT
forINSERT
compatibility.Code of Conduct