-
Notifications
You must be signed in to change notification settings - Fork 5
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
EES-5627 Adding Label
to the Release
#5457
Conversation
table: "Releases", | ||
columns: new[] { "PublicationId", "Year", "TimePeriodCoverage", "Label" }, | ||
unique: true, | ||
filter: "[Label] IS NOT NULL"); |
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.
Out of interest, does this unique constraint still take effect for the combination of PublicationId, Year and TimePeriodCoverage when Label IS null? We'd hope it does! 🙂
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 would hope so! I'll double check
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've fixed this with adding HasFilter(null)
to the configuration for the index
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 wasn't sure how the null filter applied by the code HasFilter(null)
is getting related specifically to the Label
column.
I think the documentation here covers this. It says
When using the SQL Server provider EF adds an 'IS NOT NULL' filter for all nullable columns that are part of a unique index. To override this convention you can supply a null value.
So I think the answer is that it's probably not exclusive to the Label
column, and would be applicable to other nullable columns if there were any.
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.
That's correct yes, that's also what I found :)
This PR adds a new optional
Label
database column to theRelease
.It has been set to have a max character limit of
50
.The plan is, in a subsequent PR, to change how we calculate the
Slug
for aRelease
such that it appends thisLabel
to the end, prefixed with a-
. For this reason, the character limit of theSlug
has been extended by51
.As part of this, we have changed the existing unique index that exists on the
PublicationId
/Year
/TimePeriodCoverage
columns, and have added theLabel
to the mix.Other Changes
Label
in theReleaseGeneratorExtensions
in the spirit of avoiding conflicts in some further work that might be coming along at the same time (namely, EES-5628 and EES-5634)