Skip to content

Proposed fix for Optional RestartSequenceOperation.StartValue #26560#29346

Merged
bricelam merged 6 commits intodotnet:mainfrom
dmihaita:main
Mar 30, 2023
Merged

Proposed fix for Optional RestartSequenceOperation.StartValue #26560#29346
bricelam merged 6 commits intodotnet:mainfrom
dmihaita:main

Conversation

@dmihaita
Copy link
Contributor

@dmihaita dmihaita commented Oct 13, 2022 •
edited by roji
Loading

Modified the RestartSequenceOperation.StartValue's type from long to Nullable and adapted the dependencies.

Closes #26560

@dnfadmin
Copy link

dnfadmin commented Oct 13, 2022 •
edited
Loading

CLA assistant check
All CLA requirements met.

roji
roji previously requested changes Oct 13, 2022
View reviewed changes
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Can you please add two tests in MigrationsTestBase for restarting a sequence with and without a value (seems like we're missing value there)? You can look at Alter_sequence_increment_by as a basis for that. These would need to be overridden in MigrationsSqlServerTest to assert on the actual SQL generated, and also in MigrationsSqliteTest, which should simply assert not supported (since SQLite doesn't support sequences).

If you're having trouble figuring it out, let me know.

If this is

@dmihaita
Copy link
Contributor Author

dmihaita commented Oct 14, 2022

Suggestions applied.
Adding the two unit tests revealed that the functionality was not complete ... In order to manage to pass the RESTART without WITH clause, I had to add a new property to ISequence interface (InternalStartValue), and use it only for RestartSequenceOperation, on Diff. Not sure it's the most proper way to establish if the startValue was specified or not.

@dmihaita dmihaita requested a review from roji October 18, 2022 06:18
@roji roji mentioned this pull request Oct 18, 2022
Closed
dmihaita and others added 3 commits October 31, 2022 20:07
@dmihaita
Merge branch 'dotnet:main' into main
98b9286
@dmihaita
Reverted Sequence alterations
143d4da 
Added unit tests for Restart operation (with and without startsAt parameter)
Minor styling correction
0046206
@dmihaita
Copy link
Contributor Author

dmihaita commented Nov 8, 2022

I think the scope of this issue was covered with the latest commit.

Reverted all changes to Sequence and related.
Adjusted the unit tests (removed the unit test that checked the operation creation through diff, added two unit-tests that cover the query generation based on restart operation definition)

@ajcvickers ajcvickers assigned bricelam and unassigned roji Mar 29, 2023
@bricelam bricelam dismissed roji’s stale review March 30, 2023 16:45

Feedback addressed

@bricelam bricelam merged commit 50b55d3 into dotnet:main Mar 30, 2023
@ajcvickers ajcvickers mentioned this pull request Apr 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roji roji Awaiting requested review from roji

1 more reviewer

@bricelam bricelam bricelam approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bricelam bricelam

Labels

community-contribution providers-beware

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Optional RestartSequenceOperation.StartValue

4 participants

@dmihaita @dnfadmin @bricelam @roji