adding tests and removing unnecessary not null constraint addition on drop

go/libraries/doltcore/schema/alterschema/addpk_test.go

@@ -16,6 +16,7 @@ package alterschema_test
 
 import (
 	"context"
+	"github.com/dolthub/dolt/go/libraries/doltcore/schema"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -45,6 +46,51 @@ var setupAdd = []testCommand{
 }
 
 func TestAddPk(t *testing.T) {
+	t.Run("adding and dropping primary keys does not result in duplicate NOT NULL constraints", func(t *testing.T) {
+		dEnv := dtestutils.CreateTestEnv()
+		ctx := context.Background()
+
+		for _, c := range setupAdd {
+			c.exec(t, ctx, dEnv)
+		}
+
+		table, err := getTable(ctx, dEnv, "test")
+		assert.NoError(t, err)
+
+		exitCode := commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD PRIMARY KEY(id)"}, dEnv)
+		require.Equal(t, 0, exitCode)
+
+		exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test DROP PRIMARY KEY"}, dEnv)
+		require.Equal(t, 0, exitCode)
+
+		exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD PRIMARY KEY(id)"}, dEnv)
+		require.Equal(t, 0, exitCode)
+
+		exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test DROP PRIMARY KEY"}, dEnv)
+		require.Equal(t, 0, exitCode)
+
+		exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD PRIMARY KEY(id)"}, dEnv)
+		require.Equal(t, 0, exitCode)
+
+		exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test DROP PRIMARY KEY"}, dEnv)
+		require.Equal(t, 0, exitCode)
+
+		exitCode = commands.SqlCmd{}.Exec(ctx, "sql", []string{"-q", "ALTER TABLE test ADD PRIMARY KEY(id)"}, dEnv)
+		require.Equal(t, 0, exitCode)
+
+		// Count number of NOT NULL constraints per column
+		sch, err := table.GetSchema(ctx)
+		for _, col := range sch.GetAllCols().GetColumns() {
+			count := 0
+			for _, cc := range col.Constraints {
+				if cc.GetConstraintType() == schema.NotNullConstraintType {
+					count++
+				}
+			}
+			require.Less(t, count, 2)
+		}
+	})
+
 	t.Run("Add primary key to table with index", func(t *testing.T) {
 		dEnv := dtestutils.CreateTestEnv()
 		ctx := context.Background()

go/libraries/doltcore/schema/alterschema/droppk.go

@@ -39,10 +39,10 @@ func DropPrimaryKeyFromTable(ctx context.Context, table *doltdb.Table, nbf *type
 	// Modify the schema to convert the primary key cols into non primary key cols
 	newCollection := schema.MapColCollection(sch.GetAllCols(), func(col schema.Column) schema.Column {
 		col.IsPartOfPK = false
-		// TODO: should be impossible for column to have been a primary key, but not have a NOT NULL constraint
-		if col.IsNullable() {
-			col.Constraints = append(col.Constraints, schema.NotNullConstraint{})
-		}
+		// TODO: should be impossible for column to have been a primary key, but not have a NOT NULL constraint, do I still bother to check?
+		//if col.IsNullable() {
+		//	col.Constraints = append(col.Constraints, schema.NotNullConstraint{})
+		//}
 		return col
 	})
 

integration-tests/bats/sql-merge.bats

@@ -199,7 +199,7 @@ SQL
     dolt checkout main
     run dolt sql -q "SELECT $head_variable"
     [ $status -eq 0 ]
-    [[ "$output" =~ $head_hash ]] || false    
+    [[ "$output" =~ $head_hash ]] || false
 }
 
 @test "sql-merge: DOLT_MERGE correctly merges branches with differing content in same table without conflicts" {
@@ -328,11 +328,11 @@ SQL
 
     run dolt status
     [ $status -eq 0 ]
-    [[ $output =~ "working tree clean" ]] || false    
-    
+    [[ $output =~ "working tree clean" ]] || false
+
     run dolt merge --abort
     [ $status -eq 1 ]
-    [[ $output =~ "no merge to abort" ]] || false    
+    [[ $output =~ "no merge to abort" ]] || false
 
     # make sure a clean SQL session doesn't have any merge going
     run dolt sql -q "SELECT DOLT_MERGE('--abort');"
@@ -350,7 +350,7 @@ SQL
     [ $status -eq 0 ]
     [[ "$output" =~ "pk1,c1,c2" ]] || false
     [[ ! "$output" =~ "0,0,0" ]] || false
-    [[ "$output" =~ "0,1,1" ]] || false    
+    [[ "$output" =~ "0,1,1" ]] || false
 }
 
 @test "sql-merge: DOLT_MERGE detects conflicts, returns them in dolt_conflicts table" {
@@ -380,7 +380,7 @@ SQL
     run dolt status
     [ $status -eq 0 ]
     [[ "$output" =~ "On branch main" ]] || false
-    [[ "$output" =~ "working tree clean" ]] || false    
+    [[ "$output" =~ "working tree clean" ]] || false
     [[ ! "$output" =~ "You have unmerged tables" ]] || false
     [[ ! "$output" =~ ([[:space:]]*both modified:[[:space:]]*one_pk) ]] || false
 
@@ -741,6 +741,27 @@ SQL
    [[ "$output" =~ "current fast forward from a to b. a is ahead of b already" ]] || false
 }
 
+@test "sql-merge: adding and dropping primary keys any number of times not produce schema merge conflicts" {
+    dolt commit -am "commit all changes"
+    dolt sql -q "create table test_null (i int)"
+    dolt commit -am "initial"
+
+    dolt checkout -b b1
+    dolt sql -q "alter table test_null add primary key(i)"
+    dolt sql -q "alter table test_null drop primary key"
+    dolt sql -q "alter table test_null add primary key(i)"
+    dolt sql -q "alter table test_null drop primary key"
+    dolt sql -q "alter table test_null add primary key(i)"
+    dolt commit -am "b1 primary key changes"
+
+    dolt checkout main
+    dolt sql -q "alter table test_null add primary key(i)"
+    dolt commit -am "main primary key changes"
+
+    run dolt merge b1
+    [ $status -eq 0 ]
+}
+
 get_head_commit() {
     dolt log -n 1 | grep -m 1 commit | cut -c 13-44
 }