Add reversed actions when using reverse

[halfmatthalfcat]

Reverses the Action order of operations when using reverse.

Closes #86

[halfmatthalfcat]

Looks like this is breaking a ton of stuff...hmmmm

[halfmatthalfcat]

So it looks like reverse is reversing within an operation (as evident by the tests) but it's not reversing when chaining operations

[nafg]

Can you confirm that your test fails without the change and passes with it?

[halfmatthalfcat]

@nafg I think I actually identified the root cause as https://github.com/nafg/slick-migration-api/blob/master/src/main/scala/slick/migration/api/Dialect.scala#L184

Sorting by the Action sort value is forcing order regardless of the appended order of the actions.

Removing the .sortBy(_.order) fixed the issue without any further changes. I'm assuming this isn't the best fix though, maybe we can pass a reverse flag to migrate table to actually reverse the sort order and apply less weighted actions first.

[nafg]

Thanks for investigating this. I had forgotten about sort, but it actually exists for this very purpose. So the solution is actually much simpler. The sort of things like DropForeignKey should be lower than things like DropTable. You should just be able to fiddle with the numbers in object Action. (I'd recommend reordering the lines as well, so that they are in increasing sort order, so that it's easy to see the order. That's how it is now as well, except that the sort and order are wrong as you've discovered.)

P.S. Why is GitHub showing a changed line in build.sbt?

[halfmatthalfcat]

The alignment of all the dependency versions were correct except for that one haha.

[halfmatthalfcat]

@nafg yeah so my proposed solution was to add a reverse to the migrateTable function and if you've reversed the table, it'll reverse the sort of the sql strings https://github.com/nafg/slick-migration-api/pull/87/files#diff-e597e55948ec9df1caeac6f1b01e9040R184

I'm using this locally right now and it works as expected. Does this solution seem good to you?

[coveralls]

Coverage remained the same at 64.063% when pulling 92ab4f37b5bf9d997ef716bb870e781ba5773b8c on halfmatthalfcat:master into 0f9193c075df6c646c5327fb02dbeb794948cfca on nafg:master.

[halfmatthalfcat]

Ok @nafg this should be good if you're ok with the solution. It looks like on every db except Postgres, you can't create, add and then remove all columns (there must be at least one). Not sure if that should be noted anywhere but this now acts as expected.

I added a test to that end that passes. I wasn't sure if there was much else you wanted added in the tests beyond this.

[nafg]

Can you comment on this suggestion?

I had forgotten about sort, but it actually exists for this very purpose. So the solution is actually much simpler. The sort of things like DropForeignKey should be lower than things like DropTable. You should just be able to fiddle with the numbers in object Action. (I'd recommend reordering the lines as well, so that they are in increasing sort order, so that it's easy to see the order. That's how it is now as well, except that the sort and order are wrong as you've discovered.)

[halfmatthalfcat]

@nafg Ah sorry I think I had mis-understood what you were originally talking about. I played around with the weights (sort values) and have verified locally it now works for me. Is this more of what you were thinking?

[nafg]

Yes, thanks.

Do you mind explaining the tests in more detail?

On Sun, Jun 28, 2020 at 10:21 PM Matt Oliver [email protected] wrote:

@nafg https://github.com/nafg Ah sorry I think I had mis-understood what you were originally talking about. I played around with the weights (sort values) and have verified locally it now works for me. Is this more of what you were thinking?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nafg/slick-migration-api/pull/87#issuecomment-650866509, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAYAUGGYR3FOEN7ALPR6G3RY73BDANCNFSM4OIVY7PQ .

[halfmatthalfcat]

It tests that a successful reversal of multiple chained migrations works. All of the other test examples, afaik, only test the reversal of one migration.

Due to db mechanics for the various kinds though, Postgres is the only db that can drop all columns. All the others throw and error stating that you must have at least one column present.

This test ensures that at least Postgres can do a compete reversal, as the others throw and we just continue on. I’m open to removing it though since really the only thing we’re testing is Postgres succeeding.

[nafg]

Should .reverse just be smarter and if it's reversing a create table skip reversing create columns?

On Mon, Jun 29, 2020, 9:28 AM Matt Oliver [email protected] wrote:

It tests that a successful reversal of multiple chained migrations works. All of the other test examples, afaik, only test the reversal of one migration.

Due to db mechanics for the various kinds though, Postgres is the only db that can drop all columns. All the others throw and error stating that you must have at least one column present.

This test ensures that at least Postgres can do a compete reversal, as the others throw and we just continue on. I’m open to removing it though since really the only thing we’re testing is Postgres succeeding.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nafg/slick-migration-api/pull/87#issuecomment-651120622, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAYAUETOS6I3A5Y57FH6TLRZCJIBANCNFSM4OIVY7PQ .

[halfmatthalfcat]

We could detect a drop table and then just drop any drop column actions from the actions list...we probably could just drop all other actions since dropping the table is the most final action of all actions. I'm not sure that's the most prudent way to go about it though, seems somewhat brute-forcy.

I'm using Postgres personally, so I'm unaffected by the last column rule.

[nafg]

I hear, I just don't like the idea of a test using isInstanceOf and try/catch like that, it seems like it's much less clear that everything works 100%.

Some things, like indexes, I'm not sure if dropping the table "includes" dropping them -- might it need a CASCADE? Or am I thinking of something else? Anyway they are separate, top-level objects, while columns are part of a table. So while dropping an index might be necessary and implied by dropping the table, it's still a distinct action. On the other hand, dropping columns and then the table is just dropping part of a thing and then the rest. Dropping columns isn't implied by dropping the table, it's included in it.

And that's besides the fact that apparently outside of postgres dropping all the columns is an error, while nothing else is.

So in short, for multiple reasons it's not crazy to "drop" the drop-columns when there's a drop-table too.

What do you think?

On Mon, Jun 29, 2020 at 12:33 PM Matt Oliver [email protected] wrote:

We could detect a drop table and then just drop any drop column actions from the actions list...we probably could just drop all other actions since dropping the table is the most final action of all actions. I'm not sure that's the most prudent way to go about it though, seems somewhat brute-forcy.

I'm using Postgres personally, so I'm unaffected by the last column rule.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nafg/slick-migration-api/pull/87#issuecomment-651229999, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAYAUAOHXRJAQGPH24JFWLRZC64HANCNFSM4OIVY7PQ .

[halfmatthalfcat]

Yeah I think that's a fine approach. So you'd detect whether a drop table action has been queued and then essentially remove any drop column actions and proceed as normal.

[nafg]

Hi, do you plan to implement this?

[halfmatthalfcat]

@nafg I don't have a lot of time right now unfortunately. I found a solution I can live with in my project for now. If I find some time in the future, I'll be able to revisit this if it's not already fixed.

[anovstrup]

@nafg Hi Naftoli, I just ran into this issue as well. If you can help me understand what's still needed to get a solution over the finish line, I'd be happy to work on it.

[nafg]

@anovstrup would you be interested in pair programming it with me?

IIRC it wouldn't be much work but I have to context-switch back into it

[anovstrup]

Sure! I probably won't be available until late this week (Thursday or Friday).

Sounds like this is what's left to implement, plus any additional testing that's needed?

Yeah I think that's a fine approach. So you'd detect whether a drop table action has been queued and then essentially remove any drop column actions and proceed as normal.

[nafg]

Sure! I probably won't be available until late this week (Thursday or Friday).

Great, can you message me on Discord?

Sounds like this is what's left to implement, plus any additional testing that's needed?

I would have to refresh my memory ;)