Skip to content

Remove explicit default value for not nullable columns#76

Merged
rwasef1830 merged 1 commit intonpgsql:masterfrom
xklonx:master
Sep 18, 2017
Merged

Remove explicit default value for not nullable columns#76
rwasef1830 merged 1 commit intonpgsql:masterfrom
xklonx:master

Conversation

@xklonx
Copy link
Copy Markdown
Contributor

@xklonx xklonx commented Sep 7, 2017

System.Data.SqlClient never adds default when it is not setted.

CREATE TABLE "Project_Administrator"("Id" uuid NOT NULL DEFAULT '00000000-0000-0000-0000-000000000000',
vs
CREATE TABLE [Project_Administrator] (
[Id] [uniqueidentifier] NOT NULL,

@rwasef1830
Copy link
Copy Markdown
Contributor

rwasef1830 commented Sep 10, 2017

@roji What do you think about this ? This constitutes a breaking change in behavior (not sending explicit default in create statements).

@xklonx
Copy link
Copy Markdown
Contributor Author

xklonx commented Sep 12, 2017

I also think that provider must not add custom string to index names like here
sql.Append(GetTableNameFromFullTableName(createIndexOperation.Table) + "_" + createIndexOperation.Name);
but thats the real breaking change that breaks create/drop of indexes created earlier.

@roji
Copy link
Copy Markdown
Member

roji commented Sep 17, 2017

@rwasef1830 I'm OK with this change, really not sure what the original intention was behind this implicit default...

While it's true that this is a breaking change, the impact is relatively limited since it's in migration SQL generation - so any migrations already generated will not be affected. Also, it seems like we should bump the version to 3.2.0, both because of the long wait from the previous minor release and also because of #41. So I vote to merge this for 3.2.0 - but I'm leaving the final call to you whether to merge or not.

I'd be against changing the index naming though as later suggested.

@rwasef1830, I made some version bumps and cleanup. Note that there are several failing tests at the moment: TestComputedValue, TestScalarValuedStoredFunctions, TestScalarValuedStoredFunctions_with_null_StoreFunctionName and TestTableValuedStoredFunctions. I have no time to find out what's wrong but it may be test-related rather than actual bugs. It would be good if you can take a look before we release 3.2.0, otherwise let me know and I'll release.

@rwasef1830
Copy link
Copy Markdown
Contributor

@xklonx Could you update the commit message since there is no configuration-related changes in the commit. Also could you add or update a unit test to reflect this change ?

@xklonx xklonx changed the title The default value must be specified in the configuration and should not appear from nowhere. Remove explicit default value for not nullable columns Sep 18, 2017
@xklonx
Copy link
Copy Markdown
Contributor Author

xklonx commented Sep 18, 2017

@rwasef1830 Fixed two tests, its ok now?

@rwasef1830
Copy link
Copy Markdown
Contributor

rwasef1830 commented Sep 18, 2017

@xklonx Could you squash both commits and set the the commit message to be the same as this pull request title then force push the branch ? Thanks for your contribution!

@xklonx
Copy link
Copy Markdown
Contributor Author

xklonx commented Sep 18, 2017

@rwasef1830 done

@rwasef1830 rwasef1830 merged commit 43bb1b8 into npgsql:master Sep 18, 2017
@rwasef1830
Copy link
Copy Markdown
Contributor

rwasef1830 commented Sep 18, 2017

@roji whoops I think this got merged into master, should be in dev. Leave it ?

@roji
Copy link
Copy Markdown
Member

roji commented Sep 22, 2017

@rwasef1830 can you please undo this merge to master (use git reset --hard to roll back and then git push --force)? Then you can merge this to dev instead.

@rwasef1830
Copy link
Copy Markdown
Contributor

@roji done. force pushed master. rebased to dev. pushed dev.

@roji roji added this to the 3.2.0 milestone Sep 23, 2017
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.

3 participants