Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Escape Prisma command when running with script on Linux #1561

Merged
merged 10 commits into from
Nov 14, 2023
Merged

Escape Prisma command when running with script on Linux #1561

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b3d21f8
Fixes running of command when project dir has spaces in path
infomiho Nov 7, 2023
4b375fc
Cleanup
infomiho Nov 7, 2023
66ffb01
Debug #1
infomiho Nov 7, 2023
492a6a5
Debug #2
infomiho Nov 7, 2023
f846635
Explainer comment
infomiho Nov 7, 2023
ea11b78
Cleanup
infomiho Nov 7, 2023
affad38
Fixes unit tests
infomiho Nov 8, 2023
1095e95
Merge branch 'main' into miho-path-space
infomiho Nov 13, 2023
da5c2b0
Simplify the impl
infomiho Nov 13, 2023
1a24fee
Cleanup
infomiho Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 41 additions & 13 deletions waspc/src/Wasp/Generator/DbGenerator/Jobs.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import qualified Wasp.Generator.Job as J
import Wasp.Generator.Job.Process (runNodeCommandAsJob, runNodeCommandAsJobWithExtraEnv)
import Wasp.Generator.ServerGenerator.Common (serverRootDirInProjectRootDir)
import Wasp.Generator.ServerGenerator.Db.Seed (dbSeedNameEnvVarName)
import qualified Wasp.Util.ShellCommand as Shell

migrateDev :: Path' Abs (Dir ProjectRootDir) -> MigrateArgs -> J.Job
migrateDev projectDir migrateArgs =
Expand All @@ -46,31 +47,38 @@ migrateDev projectDir migrateArgs =
scriptArgs =
if System.Info.os == "darwin"
then -- NOTE(martin): On MacOS, command that `script` should execute is treated as multiple arguments.
["-Fq", "/dev/null"] ++ prismaMigrateCmd
["-Fq", "/dev/null"] ++ Shell.getShellArgValues prismaMigrateCmd
else -- NOTE(martin): On Linux, command that `script` should execute is treated as one argument.
["-feqc", unwords prismaMigrateCmd, "/dev/null"]
-- NOTE(miho): Since we are passing the arguments as a single string, we need to wrap the arguments
-- which might contain spaces in them. We are using Shell.Quoted to know which
-- arguments should be wrapped in quotes and which shouldn't.
-- Specifically, we are using `showShellArgs` to wrap file paths in quotes. Other arguments
-- are not wrapped in quotes since we know their values and they don't contain spaces.
infomiho marked this conversation as resolved.
Show resolved Hide resolved
["-feqc", Shell.showShellArgs prismaMigrateCmd, "/dev/null"]

-- NOTE(martin): For this to work on Mac, filepath in the list below must be as it is now - not
-- wrapped in any quotes.
-- NOTE(martin): We do "--skip-seed" here because I just think seeding happening automatically
-- in some situations is too aggressive / confusing.
prismaMigrateCmd =
[ absPrismaExecutableFp projectDir,
[ Shell.Quoted $ absPrismaExecutableFp projectDir,
"migrate",
"dev",
"--schema",
SP.fromAbsFile schemaFile,
Shell.Quoted $ SP.fromAbsFile schemaFile,
"--skip-generate",
"--skip-seed"
]
infomiho marked this conversation as resolved.
Show resolved Hide resolved
++ asPrismaCliArgs migrateArgs

asPrismaCliArgs :: MigrateArgs -> [String]
asPrismaCliArgs :: MigrateArgs -> [Shell.ShellCommandArg]
asPrismaCliArgs migrateArgs = do
concat . concat $
[ [["--create-only"] | _isCreateOnlyMigration migrateArgs],
[["--name", name] | Just name <- [_migrationName migrateArgs]]
]
concat . concat $ [createOnlyArg, nameArg]
where
createOnlyArg =
[["--create-only"] | _isCreateOnlyMigration migrateArgs]
nameArg =
[["--name", Shell.Raw name] | Just name <- [_migrationName migrateArgs]]

-- | Diffs the Prisma schema file against the db.
-- Because of the --exit-code flag, it changes the exit code behavior
Expand All @@ -93,15 +101,25 @@ migrateDiff projectDir = runPrismaCommandAsDbJob projectDir $ \schema ->
-- Therefore, this should be checked **after** a command that ensures connectivity.
migrateStatus :: Path' Abs (Dir ProjectRootDir) -> J.Job
migrateStatus projectDir = runPrismaCommandAsDbJob projectDir $ \schema ->
["migrate", "status", "--schema", SP.fromAbsFile schema]
[ "migrate",
"status",
"--schema",
SP.fromAbsFile schema
]
Copy link
Member

Choose a reason for hiding this comment

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

If you really like it better in multiple lines fine, but I liked it better in one line, found it easier to read. And since it has nothing to do with this PR maybe we could revert it? Same goes for other couple of functions in this file.


-- | Runs `prisma migrate reset`, which drops the tables (so schemas and data is lost) and then
-- reapplies all the migrations.
reset :: Path' Abs (Dir ProjectRootDir) -> J.Job
reset projectDir = runPrismaCommandAsDbJob projectDir $ \schema ->
-- NOTE(martin): We do "--skip-seed" here because I just think seeding happening automatically on
-- reset is too aggressive / confusing.
["migrate", "reset", "--schema", SP.fromAbsFile schema, "--skip-generate", "--skip-seed"]
[ "migrate",
"reset",
"--schema",
SP.fromAbsFile schema,
"--skip-generate",
"--skip-seed"
]

-- | Runs `prisma db seed`, which executes the seeding script specified in package.json in
-- prisma.seed field.
Expand All @@ -125,7 +143,14 @@ dbExecuteTest projectDir =
let absSchemaPath = projectDir </> dbSchemaFileInProjectRootDir
in runPrismaCommandAsDbJob
projectDir
(const ["db", "execute", "--stdin", "--schema", SP.fromAbsFile absSchemaPath])
( const
[ "db",
"execute",
"--stdin",
"--schema",
SP.fromAbsFile absSchemaPath
]
)

-- | Runs `prisma studio` - Prisma's db inspector.
runStudio :: Path' Abs (Dir ProjectRootDir) -> J.Job
Expand All @@ -135,7 +160,10 @@ runStudio projectDir = runPrismaCommandAsDbJob projectDir $ \schema ->
generatePrismaClient :: Path' Abs (Dir ProjectRootDir) -> (String, String) -> JobType -> J.Job
generatePrismaClient projectDir prismaClientOutputDirEnv jobType =
runPrismaCommandAsJobWithExtraEnv jobType envVars projectDir $ \schema ->
["generate", "--schema", SP.fromAbsFile schema]
[ "generate",
"--schema",
SP.fromAbsFile schema
]
where
envVars = [prismaClientOutputDirEnv]

Expand Down
22 changes: 22 additions & 0 deletions waspc/src/Wasp/Util/ShellCommand.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
module Wasp.Util.ShellCommand where
Copy link
Member

Choose a reason for hiding this comment

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

This is quite cool, I never used IsString!

I like the concept you used here, it is quite neat. So basically we have this notion of Shell command argument, and we can say if it is quoted or not, and we carry this knowledge around, and then can act on it.

What I found tricky is:

  1. There are no comments in this file explaining the reasoning behind it and what it is for. It is somewhat obvious, but I don't think it is obvious enough.
  2. Continuing on above, I think the reason it is not obvious enough is that usecase feels quite trivial. If I want args quoted, why don't I continue using String and just add quotes to it? Ok, I guess this gives more knowledge, so you can avoid quoting already quoted arg, or you can unquote already quoted arg (although that feels a bit weird, why was it quoted then, but ok, maybe we know somebody else will quote it). Still, I think it is a bit confusing so explanation would help.
  3. Using it above in the Jobs.hs feels a bit like overkill. Basically we need to quote these two args that are paths. And in one situation we need to quote them, in other we don't. But now we have to introduce this new type in all of the args there. But, not all of them, just some of them -> its not like we are using it across the whole codebase, just here. Feels weird that we are using it just in the small part of the codebase, how do we choose? When should I use this? Btw, isn't there a simpler solution -> can't we just quote those paths and that is it? Is it a problem if they are also quoted in that situation where they shouldn't be quoted, or will it just work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the problem we have is the following: we need the same command in a slightly different ways for Linux and for MacOS.

MacOS

  • The arguments should be a list of arguments
  • The paths passed into the prisma command MUST NOT be quoted

Linux

  • The arguments are concatenated into a single string
  • The paths MUST be quoted (since they can have spaces in them)

I'll try to simplify the solution and make it very local (it will only affect the array of prisma db migrate args and nothing else).

Copy link
Member

Choose a reason for hiding this comment

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

Nice, this is much simpler now!

I left some small comments but otherwise it is good.

I would possibly consider skipping this file here, ShellCommand.hs, and just inlining this function quoteArg where it is used, since it is so niche / local, I don't think we benefit much from having it here in a separate file.


import Data.String (IsString (fromString))

data ShellCommandArg = Raw String | Quoted String
deriving (Eq)

instance Show ShellCommandArg where
show (Raw arg) = arg
show (Quoted arg) = "\"" ++ arg ++ "\""

instance IsString ShellCommandArg where
fromString = Raw

showShellArgs :: [ShellCommandArg] -> String
showShellArgs = unwords . map show
infomiho marked this conversation as resolved.
Show resolved Hide resolved

getShellArgValues :: [ShellCommandArg] -> [String]
infomiho marked this conversation as resolved.
Show resolved Hide resolved
getShellArgValues = map getShellArgValue
where
getShellArgValue (Raw arg) = arg
getShellArgValue (Quoted arg) = arg
2 changes: 1 addition & 1 deletion waspc/waspc.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ data-files:
packages/studio/dist/**/*.js
packages/studio/dist/**/*.html
packages/studio/dist/**/*.css
packages/studio/dist/**/*.png
infomiho marked this conversation as resolved.
Show resolved Hide resolved
packages/studio/package.json
packages/studio/package-lock.json
data-dir: data/
Expand Down Expand Up @@ -346,6 +345,7 @@ library
Wasp.Util.StrongPath
Wasp.Util.HashMap
Wasp.Util.WebRouterPath
Wasp.Util.ShellCommand
Wasp.WaspignoreFile
Wasp.Generator.NpmDependencies
Wasp.Generator.NpmInstall
Expand Down
Loading