Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Implement Display of Destructive Changes #146

Closed
mavilein opened this issue Sep 25, 2019 · 9 comments
Closed

Implement Display of Destructive Changes #146

mavilein opened this issue Sep 25, 2019 · 9 comments
Assignees
Labels
kind/feature A request for a new feature.
Milestone

Comments

@mavilein
Copy link
Member

The migration engine has now implemented support for checking for destructive changes. The output of applyMigration can now return a non empty array in the warnings property. If that is the case the CLI must send the force parameter as true so the user acknowledges the destructive change.

The JSON for warnings looks like this:

[
  {"description": "You are about to drop the table `User`, which is not empty (11 rows)."},
  {"description": "You are about to drop the column `name` on the `User` table, which still contains 23 non-null values."}
]

Note: The migration engine currently ignores the force flag and applies the migration even if there are destructive changes. This is temporary until the CLI has implemented support for this and we can enable the proper behavior.

@mavilein mavilein added the process/candidate Candidate for next Milestone. label Sep 25, 2019
@janpio janpio added the kind/feature A request for a new feature. label Sep 25, 2019
@mavilein
Copy link
Member Author

I have created a specs issue about this topic.

@matthewmueller
Copy link
Contributor

matthewmueller commented Sep 26, 2019

@mavilein do we receive enough information here to also show a diff?

image

@mavilein
Copy link
Member Author

@matthewmueller : No. Currently it is just a plain String describing the data loss. What kind of diff would you like to show?

@matthewmueller
Copy link
Contributor

matthewmueller commented Sep 26, 2019

Thanks, I'll formalize this more, but quick stream of thoughts on what I'd propose:

Delete a model:

- model User {
-  id Int @id
-  firstName String
-  lastName String
-  @unique([firstName, lastName], name: "fullName")
- }

Rename a field + update an attribute:

model User {
   id Int @id
-  firstName String
+  givenName String
   lastName String
~  @unique([givenName, lastName], name: "fullName")
}

We may have this information already on the client, but if not, it probably makes sense to send it

@janpio janpio added this to the Preview 14 milestone Sep 27, 2019
@janpio janpio removed the process/candidate Candidate for next Milestone. label Sep 27, 2019
@mavilein
Copy link
Member Author

@matthewmueller : How are your proposals related to destructive changes? I think we have this kind of data model diffing in the CLI already. A deleted field might not lead to data loss at all hence i don't see how this moves this forward.

@matthewmueller
Copy link
Contributor

matthewmueller commented Sep 30, 2019

The spec is not describing the --force flag or a replacement that we had in P1. To my understanding we definitely need that.

I originally had a confirmation dialog with an --auto-approve dialog during lift up. I removed that flow after I learned that lift up doesn't seem to use it.

We should think about when we want to display destructive changes. Shouldn’t we maybe show already in lift save? It would be annoying to run lift save and later find out that the migration is actually destructive when running lift up.

How are these messages obtained? It feels like it would be slow to learn some of this and halting the developer flow to save a migration seems iffy.

@mavilein
Copy link
Member Author

mavilein commented Oct 1, 2019

@matthewmueller : What do you mean with How are these messages obtained??

@matthewmueller
Copy link
Contributor

matthewmueller commented Oct 1, 2019

@mavilein for example:

"You are about to drop the column name on the User table, which still contains 23 non-null values."

I'm assuming this would do something like:

select count(name) from User where name is not null

If that's the case, could that get really slow on large datasets? And if you're making sweeping changes, it could require N queries for N changes.

I'm trying to get a gauge of how slow this could be when I type lift save, since you don't expect saving a migration locally to be slow.

@mavilein
Copy link
Member Author

mavilein commented Oct 1, 2019

@matthewmueller : Yes this is what we do. I think this is a valid concern. We have not had problems with this approach in Prisma 1 though.

@janpio janpio added process/candidate Candidate for next Milestone. process/next-milestone and removed process/candidate Candidate for next Milestone. labels Oct 10, 2019
@mavilein mavilein added the process/candidate Candidate for next Milestone. label Oct 11, 2019
@janpio janpio assigned timsuchanek and unassigned matthewmueller Oct 11, 2019
@janpio janpio modified the milestones: Preview 14, Preview 15 Oct 11, 2019
@janpio janpio removed the process/candidate Candidate for next Milestone. label Oct 11, 2019
timsuchanek pushed a commit that referenced this issue Oct 21, 2019
* Add a Studio class to handle communication with @prisma/studio-server

* Add a StudioCommand class

* Use this new Studio class in Lift's main class

* Remove getDatamodelPath from Lift's main class and use the new utility

* Remove `datamodel` param from `recreateStudioServer` since it is not required by StudioServer anymore.

* Use fs.existsSync instead of fs.stat

* 0.3.55

[skip ci]

* add destructive changes ui for dev. Closes #146

* 0.3.56

[skip ci]

* 0.3.57

[skip ci]

* fix auto approve

* 0.3.58

[skip ci]

* Use this new Studio class in Lift's main class

* Remove getDatamodelPath from Lift's main class and use the new utility

* Remove occupyPath from StudioCommand
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature A request for a new feature.
Projects
None yet
Development

No branches or pull requests

4 participants