Skip to content
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

fix: set unique to true for primary key fields in the proto.schema #1292

Conversation

louisaspicer
Copy link
Contributor

Fixes the proto schema to have primary key fields set as unique. It is misleading if the flag is set to false.

@louisaspicer louisaspicer requested a review from a team November 9, 2023 15:52
@louisaspicer louisaspicer force-pushed the ke-1247-primary-key-fields-arent-marked-as-unique-in-the-proto branch from e3b708c to 552bb8e Compare November 9, 2023 15:54
@louisaspicer louisaspicer changed the title fix: set unique to true for primary key fields in the proto.Schema fix: set unique to true for primary key fields in the proto.schema Nov 9, 2023
Copy link
Contributor

@davenewza davenewza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@@ -289,7 +289,7 @@ func New(ctx context.Context, schema *proto.Schema, database db.Database) (*Migr
return c.TableName == tableName && c.ConstraintType == "u" && len(c.ConstrainedColumns) == 1 && c.ConstrainedColumns[0] == int64(column.ColumnNum)
})

if field.Unique && !hasUniqueConstraint {
if field.Unique && !field.PrimaryKey && !hasUniqueConstraint {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@louisaspicer louisaspicer merged commit 569b8ba into main Nov 13, 2023
10 checks passed
@louisaspicer louisaspicer deleted the ke-1247-primary-key-fields-arent-marked-as-unique-in-the-proto branch November 13, 2023 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants