Adjust the DB check error message #1558

infomiho · 2023-11-03T13:53:54Z

Closes #1435

We had a regression with the way we handle DB server being available but the DB not existing.

Before the DB check was introduced
- we wouldn't check DB server connectivity
- Prisma would create the DB if missing
After the DB check was introduced
- we check for DB server connectivity by trying to execute a dummy SQL command on the DB
- Wasp fails with an error "DB server is not running"
After the fix
- we check for DB server connectivity by trying to execute a dummy SQL command on the DB
- if the error is "DB doesn't exist" -> we tolerate it, since we know Prisma will create the DB if missing
- in all other cases -> Wasp fails with an error "DB server is not running"

Martinsos

@infomiho I like the solution, and very nice job on writing the main description, it gave me all the context I needed!

I left comments mostly on naming / clean code and one very unlikely bug.

waspc/src/Wasp/Generator/DbGenerator/Operations.hs

Martinsos · 2023-11-06T15:23:01Z

waspc/src/Wasp/Generator/DbGenerator/Operations.hs

+      textOutput <- collectJobTextOutputUntilExitReceived chan
+
+      -- "Database not created" error is fine since Prisma will create it for us.
+      let isErrorTolerated = any containsDatabaseNotCreatedError textOutput
+      return isErrorTolerated


I like how you wrote some "extra" code here to make it clearer what is happening!
I found textOutput confusing -> I thought it is a single text, while I believe it is lines, so I suggest making that clearer.
I also tried to make the code even a bit more explicit, as to the reasoning of what makes a check fail.

Suggested change

textOutput <- collectJobTextOutputUntilExitReceived chan

-- "Database not created" error is fine since Prisma will create it for us.

let isErrorTolerated = any containsDatabaseNotCreatedError textOutput

return isErrorTolerated

outputLines <- collectJobTextOutputUntilExitReceived chan

let databaseNotCreated = any containsDatabaseNotCreatedError outputLines

let checkSucceeded = databaseNotCreated

return checkSucceeded

Martinsos · 2023-11-06T15:24:49Z

waspc/src/Wasp/Generator/DbGenerator/Operations.hs

+      return isErrorTolerated
+  where
+    -- Prisma error code for "Database not created" is P1003.
+    containsDatabaseNotCreatedError = T.isInfixOf "P1003"


So, while this will work in 99% cases, what if the prisma error text contains PIP1003 or P10030 or anything else that is not \bP1003\b? Then it will give false positive.

So to be 100% safe, I would do a bit fancier check here. Probably use regex \bP1003\b.

Martinsos · 2023-11-06T15:27:37Z

waspc/src/Wasp/Generator/Job/IO.hs

+collectJobMessages :: Chan J.JobMessage -> IO [J.JobMessageData]
+collectJobMessages = collect []
+  where
+    collect messages chan = do
+      jobMsg <- readChan chan
+      case J._data jobMsg of
+        J.JobExit {} -> return messages
+        message -> collect (message : messages) chan


If you suffixed that other function with untilExitReceived, I would do the same here, since it behaves like that.

Btw I can't remember what is the difference between JobMessage and JobMessageData, but this function says it collects JobMessages but it returns JobMessageData, is that an issue? Should the name also have Data in it?

Aha ok, I realized it now from the casing in another function above. So JobMessageData is either Text or Exit? In that case, since you know you are returning only Text's here, why not return IO [Text]? It is more precise!

Martinsos · 2023-11-06T15:30:41Z

waspc/src/Wasp/Generator/Job/IO.hs

+collectJobMessages :: Chan J.JobMessage -> IO [J.JobMessageData]
+collectJobMessages = collect []
+  where
+    collect messages chan = do
+      jobMsg <- readChan chan
+      case J._data jobMsg of
+        J.JobExit {} -> return messages
+        message -> collect (message : messages) chan


Suggested change

collectJobMessages :: Chan J.JobMessage -> IO [J.JobMessageData]

collectJobMessages = collect []

where

collect messages chan = do

jobMsg <- readChan chan

case J._data jobMsg of

J.JobExit {} -> return messages

message -> collect (message : messages) chan

collectJobMessages :: Chan J.JobMessage -> IO [J.JobMessageData]

collectJobMessages = collect []

where

collect jobMsgsData chan = do

jobMsg <- readChan chan

case J._data jobMsg of

J.JobExit {} -> return messages

jobMsgData -> collect (jobMsgData : jobMsgsData) chan

Martinsos · 2023-11-06T15:32:12Z

waspc/src/Wasp/Generator/Job/IO.hs

+    jobMessageToText :: J.JobMessageData -> Maybe Text
+    jobMessageToText = \case
+      J.JobOutput text _ -> Just text
+      J.JobExit _ -> Nothing


Although you know JobExit will never arrive here, right? But you wanted to satisfy the compiler? Ok, well it gives extra safety!

Ok now looking at the function below, collectJobMessages, it looks to me like you really should just merge the two functions into one, into collectJobTextOutputUntilExitReceived.

Martinsos

Nice, looking good! I left a couple more comments but I think it is in good shape so I approved -> to with these comments as you wish and you can merge!

Martinsos · 2023-11-08T12:27:28Z

waspc/src/Wasp/Generator/DbGenerator/Operations.hs

+data DbConnectionTestResult
+  = DbConnectionSuccess
+  | DbNotCreated
+  | DbConnectionFailure


I would be tempted to add String to DbConnectionFailure that contains actual error message, is there a reason why you decided to drop that information?

Because we don't get an error message per se, but we get some command output that might be useless unless parsed.

Martinsos · 2023-11-08T12:29:43Z

waspc/src/Wasp/Generator/DbGenerator/Operations.hs

+containsDatabaseNotCreatedError :: T.Text -> Bool
+containsDatabaseNotCreatedError text = text TR.=~ ("\\bP1003\\b" :: String)


I would probably put this in where under the testDbConnection. But maybe you took it out so you can write tests for it? If so, I understand. But then I would give it a bit more descriptive name: e.g. prismaOutputContainsDbNotCreatedError, because now it is missing the context it would have if it was in the where.

Martinsos · 2023-11-08T12:32:57Z

waspc/src/Wasp/Generator/DbGenerator/Operations.hs

+containsDatabaseNotCreatedError :: T.Text -> Bool
+containsDatabaseNotCreatedError text = text TR.=~ ("\\bP1003\\b" :: String)
+
+isDbConnectionEstablished :: DbConnectionTestResult -> Bool


I get what you do with this function, makes sense! I wonder a bit about name -> so we call that "connection established"? Even if database does not exist? It is not a bad name, but I wonder if it is precise enough. I guess the problem is I am not sure what "db connection established" really means.
Is there a better matching name? Maybe isDbInValidState? or isDbReady? isDbInUsableState? isDbReadyForPrisma? Something in that direction? If we can't find anything better, fine, let's leave it as it is, but I do feel that isDbConnectionEstablished could maybe be better.
If nothing, you could go with didDbConnectionTestSucceed.

I've went with isDbConnectionPossible since that's the check I was going for. The DB server can be connected to.

Martinsos · 2023-11-08T12:35:40Z

waspc/src/Wasp/Generator/Job/IO.hs

@@ -29,6 +31,15 @@ readJobMessagesAndPrintThemPrefixed chan = runPrefixedWriter go
        J.JobOutput {} -> printJobMessagePrefixed jobMsg >> go
        J.JobExit {} -> return ()

+collectJobTextOutputUntilExitReceived :: Chan J.JobMessage -> IO [Text]
+collectJobTextOutputUntilExitReceived = collect []


In haskell, usual idiom is to go with go, for functions like collect, as a name.

Adjust the DB check error message

f58d772

infomiho requested review from Martinsos and sodic November 3, 2023 13:53

infomiho added 3 commits November 3, 2023 14:57

Cleanup

e12b49d

Adjusts the example app

41ea676

Cleanup

2311d68

Martinsos requested changes Nov 6, 2023

View reviewed changes

PR comments, unit tests

771923f

infomiho requested a review from Martinsos November 8, 2023 10:14

Martinsos approved these changes Nov 8, 2023

View reviewed changes

infomiho added 3 commits November 9, 2023 11:59

PR comments

76510ce

Merge branch 'main' into miho-fix-db-init

5722c8d

Updates Changelog

ec7359e

infomiho merged commit 62aa2b7 into main Nov 9, 2023
4 checks passed

infomiho deleted the miho-fix-db-init branch November 9, 2023 12:19

martinovicdev pushed a commit to martinovicdev/wasp that referenced this pull request Nov 15, 2023

Adjust the DB check error message (wasp-lang#1558)

87573ec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust the DB check error message #1558

Adjust the DB check error message #1558

infomiho commented Nov 3, 2023

Martinsos left a comment

Martinsos Nov 6, 2023

Martinsos Nov 6, 2023

Martinsos Nov 6, 2023

Martinsos Nov 6, 2023

Martinsos Nov 6, 2023

Martinsos Nov 6, 2023

Martinsos Nov 6, 2023

Martinsos Nov 6, 2023

Martinsos left a comment

Martinsos Nov 8, 2023

infomiho Nov 9, 2023

Martinsos Nov 8, 2023

Martinsos Nov 8, 2023

infomiho Nov 9, 2023

Martinsos Nov 8, 2023

		containsDatabaseNotCreatedError :: T.Text -> Bool
		containsDatabaseNotCreatedError text = text TR.=~ ("\\bP1003\\b" :: String)

Adjust the DB check error message #1558

Adjust the DB check error message #1558

Conversation

infomiho commented Nov 3, 2023

Martinsos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment