Fix db reset failing after wasp clean

[sodic]

This PR fixes wasp db reset failing after wasp clean:

$ wasp clean

🐝 --- Deleting the .wasp/ directory... -------------------------------------------


✅ --- Deleted the .wasp/ directory. ----------------------------------------------


🐝 --- Deleting the node_modules/ directory... ------------------------------------


✅ --- Deleted the node_modules/ directory. ---------------------------------------

$ wasp db reset

🐝 --- Resetting the database... --------------------------------------------------

wasp-bin: /home/filip/Projects/wasp/wasp-repo/waspc/examples/todoApp/node_modules/.bin/prisma: streamingProcess: chdir: invalid argument (Bad file descriptor)
$ 

[sodic]

I introduced this bug when adding the reset case into the dbCli. I did it by copying the line that handles db start and just changing the command's name (here).

As it turns out, all other commands inside dbCli perform their task on a database and rely on the same prerequisites. wasp db start is the only exception.

So, to prevent this bug from happening again, I pulled the runDbCommand call out of the case statement in dbCli.

The dbCli function is now only in charge of the commands that act on the database (I've added that to its documentation).

That said, the whole thing now looks more complicated than it maybe needs to be, so I could be convinced to change it back.

@Martinsos @infomiho What do you guys think?

[infomiho]

+1 for using the runDbCommand for every db command, makes sense to me that we check prerequisites for db commands in the same way, if they don't need it, they are some other type of command.

One change I'd propose here is that this line stays like before:

Command.Call.Db dbArgs -> dbCli dbArgs

and you handle the wasp db start case above:

["start", "db"] -> Command.Call.StartDb
["db", "start"] -> Command.Call.StartDb

[Martinsos]

So I get both what you did @sodic and also I get what @infomiho suggested, and all of these make sense to me. I would be ok with any of these solutions.
That said, I would probably revert it back, because it was quite simple and straightforward, plus it doesn't assume that everything that is under db needs a database running, which I think is ok. Yes, it has some space for the mistake that happened, but if you take runDbCommand out, one can also easily make the opposite type of mistake, which is adding a command that doesn't need a DB, but it will require it, and you test that with your DB on and think it is all ok and then later it doesn't work somebody when they don't have the DB on. So at least so far it was explicit, even if not very obvious. How can we make it more obvious? We could add a comment, warning about the difference between runCommand and runDbCommand. We could make runDbCommand more verbose so it is easier to notice it is not the same as runCommand, for example runCommandThatRequiresDatabaseRunning. Or, we could just group these commands into two groups, but under dbCli: first group that applies runCommand on all of them, the other that applies runDbCommand. Now it is clear there are two types of commands in play here and you will see both and will have to consider in which group to put it, forcing you to differentiate betwen 'db-require' commands and the others. Yeah I probably like the last one the best.

[sodic]

Or, we could just group these commands into two groups, but under dbCli: first group that applies runCommand on all of them, the other that applies runDbCommand

Can you give a code example of what you mean? I'm not sure I got it.

[Martinsos]

You are right, it is a bit tricky: I likely didn't go enough into details when I was imagining it in my head.
You could do it I believe by doing what you did twice: having one case for normal commands, that returns Maybe, then another for those that require a DB, that also returns Maybe, and then connect those two together and if both are Nothing return printDbUsage. It has its drawbacks as you could by accident have same command string in both cases and now know it, although I don' think that is likely.
But since this is a bit more complicated than I imagined, it might be easier to just keep it as it was and add comments and all good.

[sodic]

Ok, here's what I did in the end.

Since Miho and I prefer to have all commands under dbCli act on a running database, and Martin is OK with both suggestions above, I kept the core idea behind my change.

I also took Martin's suggestion of renaming the runDbCommand to something more meaningful.

The main argument is:

If the dbCli function contains commands that require a database and those that don't, then its role is purely syntactic convenience (to save us from inlining all cases in main).
If the dbCli function contains only the commands that need a database, it has a meaningful semantic connection^***, which makes it easier to give the function a meaningful name.

That's why I renamed it from dbCli to runCommandOnRunningDb. I also changed the name of runDbCommand to runCommandOnRunningDb and used a qualified import (as we usually do).

The new name should make it obvious that all commands it runs require a running database.

I've also decided not to do this:

["start", "db"] -> Command.Call.StartDb
["db", "start"] -> Command.Call.StartDb

Precisely to emphasize that, under the "db" namespace, you can run both commands that act on the database, as well as commands that don't:

Command.Call.Db dbArgs -> case dbArgs of
  ["start"] -> runCommand Command.Start.Db.start
  _commandOnDb -> runCommandOnRunningDb dbArgs

To address Martin's concerns:

plus it doesn't assume that everything that is under db needs a database running, which I think is ok.

There's a branching for "db" commands in main, so it hopefully makes it clear that only some "db" commands need a running database.

Yes, it has some space for the mistake that happened, but if you take runDbCommand out, one can also easily make the opposite type of mistake...

With the new names, the haddock above the function, the explicit branching under "db" in the main function, and the code path in runCommandOnRunningDb always going through runDbCommand -- I believe that both the author and the reviewer will be forced to think about whether their command needs a database running or not.

_{*** You could say that the semantic connection was that they were all under the "db" namespace, but that logic was fully handled in main, and the function's call site shouldn't affect its inner semantics.}

[infomiho]

The new structure feels okay to me. The name is a bit weird to me: runCommandOnRunningDb, maybe runCommandRequiringDb or runCommandWithRunningDb or full runCommandThatRequiresDbRunning.

But, I'm also okay with the situation as-is.

[sodic]

runCommandThatRequiresDbRunning - Martin suggested this one too, so I'll rename it.

[Martinsos]

Ok! What's a bit weird now is that if it is not a "normal" command then we assume it is command running on db, well thtat is ok, but then we also assume that runCommandOnRunningDb will print usage if no command is found. Which is weird a bit, that that logic got moved into runCommandOnRunningDb. Also, that logic will be printing info about wasp db start, which is not even one of the commands in the case inside that function. So this is looking weird.

One way to improve this is to inline it directly here. It will be a bit crowded though.

Another way is to go back to waht we had before and just add comments, maybe that is best.

Third way is to make "db commands that don't need db running" and "db commands that need db running" equivalent, something like this:

Command.Call.Db dbArgs -> dbCli dbArgs

dbCli :: [String] -> IO ()
dbCli args = fromMaybe printDbUsage $
  runCommand <$> parseRegularCmd args
  <|> runDbCommand <$> parseCmdThatRequiresRunningDb args
  where
    parseRegularCmd args' = case args' of
      ["start"] -> Just Command.Db.Reset.reset
      _ -> Nothing

    parseCmdThatRequiresRunningDb args' = case args' of
      ["reset"] -> Just Command.Db.Reset.reset
      "migrate-dev" : optionalMigrateArgs -> Just $ Command.Db.Migrate.migrateDev optionalMigrateArgs
      ["seed"] -> Just $ Command.Db.Seed.seed Nothing
      ["seed", seedName] -> Just $ Command.Db.Seed.seed $ Just seedName
      ["studio"] -> Just Command.Db.Studio.studio
      _ -> Nothing

I haven't tried compiling the code so maybe I wrote some nonsense but the idea is to have two parsers that return Maybe and connect them with Alternative (<|>) and if both fail go with printDbUsage.

[sodic]

... but then we also assume that runCommandOnRunningDb will print usage if no command is found. Which is weird a bit, that that logic got moved into runCommandOnRunningDb. Also, that logic will be printing info about wasp db start

Ahh, I did consider that, but not thoroughly enough.
I thought it was a little messy (having a catch-all in a nested function), but I didn't notice that it will also print info about wasp db start. Yeah, that's bad. Nice catch!

We can also do this:

Command.Call.Db dbArgs -> case dbArgs of
  ["start"] -> runCommand Command.Start.Db.start
  _commandOnDb -> fromMaybe printDbUsage (runCommandOnRunningDbIfValid dbArgs)

runCommandOnRunningDbIfValid :: [String] -> Maybe (IO ())
runCommandOnRunningDbIfValid args = Command.Db.runCommandOnRunningDb <$> parsedCommand
  ...

That solves all the problems I think, but is too weird for my taste.

About the parser suggestion - also too complicated IMO. Thanks for sharing it though, the mechanics are interesting.

Another way is to go back to what we had before and just add comments, maybe that is best.

Yes, that's my favorite option now. I was opposed to it at first because I really wanted the express constraints through code rather than through comments, but in this case the complexity isn't worth it.

@Martinsos Let me know if you by any chance prefer the runCommandOnRunningDbIfValid approach. If not, I'm sticking with the comment.

[Martinsos]

Yeah, not worth it. What you presented now is better, but let's say I want to add a third class of db commands or something here, then this printUsage is again stuck under comandOnDb case, it is a bit weird, it is still not clear this is actually the third case next to normal command and db running command.
Which is why I liked a bit better the code I proposed because it is more explicit in the sense of "it is a normal command, or command on running db, or I will print usage", and it allows for an modification of that on the high level (like inserting a new case) or adding commands to each of the groups.

But yeah, if you think that is too complex, let's do the comments and we are good. Plus maybe longer name for runDbCommand, that is good change.