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.

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

Prisma CLI non-interactive detection is incorrect. #14620

Closed
AndrewSouthpaw opened this issue Aug 2, 2022 · 3 comments · Fixed by #14649
Closed

Prisma CLI non-interactive detection is incorrect. #14620

AndrewSouthpaw opened this issue Aug 2, 2022 · 3 comments · Fixed by #14649
Labels
bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. kind/bug A reported bug. team/schema Issue for team Schema. topic: cli topic: migrate
Milestone

Comments

@AndrewSouthpaw
Copy link
Contributor

Bug description

The prisma migrate CLI command guards against use in non-interactive environments, but it can lead to false positives.

How to reproduce

Run prisma migrate from a CLI helper tool like oclif using zx to execute the command. It is capable of receiving input, but Prisma thinks it's non-interactive.

Expected behavior

prisma migrate doesn't error with the non-interactive error.

Prisma information

Not relevant, applies to any schema

Environment & setup

n/a

Prisma Version

prisma                  : 4.1.1
@prisma/client          : 4.1.1
Current platform        : darwin
Query Engine (Node-API) : libquery-engine 8d8414deb360336e4698a65aa45a1fbaf1ce13d8 (at node_modules/@prisma/engines/libquery_engine-darwin.dylib.node)
Migration Engine        : migration-engine-cli 8d8414deb360336e4698a65aa45a1fbaf1ce13d8 (at node_modules/@prisma/engines/migration-engine-darwin)
Introspection Engine    : introspection-core 8d8414deb360336e4698a65aa45a1fbaf1ce13d8 (at node_modules/@prisma/engines/introspection-engine-darwin)
Format Binary           : prisma-fmt 8d8414deb360336e4698a65aa45a1fbaf1ce13d8 (at node_modules/@prisma/engines/prisma-fmt-darwin)
Default Engines Hash    : 8d8414deb360336e4698a65aa45a1fbaf1ce13d8
Studio                  : 0.469.0
@AndrewSouthpaw AndrewSouthpaw added the kind/bug A reported bug. label Aug 2, 2022
@AndrewSouthpaw
Copy link
Contributor Author

AndrewSouthpaw commented Aug 4, 2022

Recording some notes for future Googlers...

Okay so I figured out the code for isCi was the issue, here's the call and the definition, which checks process.stdout.isTTY.

It turns out zx doesn't set up a TTY stdout by default, because it's passing in ['inherit', 'pipe', 'pipe'] by default. I don't understand TTY and processes very well, but it seems that setting it to ["inherit", "inherit", "inherit"] makes it work, and then process.stdout.isTTY is true. You can set that here.

Unfortunately I'm stuck on older version of zx, so I ran it manually with a child_process:

  const child = require("child_process").spawn(command, {
    cwd: process.cwd(),
    shell: "bash",
    // Tells the child process to receive the same stdin/stdout/stderr as the parent
    // https://nodejs.org/api/child_process.html#optionsstdio
    stdio: ["inherit", "inherit", "inherit"],
    windowsHide: true,
  });

So I've found a workaround, but having the child process inherit the parent causes other minor issues with oclif.

It also surprises me to check stdout, because it's the stdin that really matters. With zx for example, stdin.isTTY is true, because it is indeed piping inputs and I can interact with the Prisma migrate command if I hack around the isCi check. So it doesn't seem to be accurate.

But also I really don't understand the in's and out's of processes and child processes. I looked through the history of the repo and didn't see a design justification for using stdout over stdin, so I've created a PR proposing to swap them.

@do4gr do4gr added team/schema Issue for team Schema. bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. labels Aug 4, 2022
@Jolg42
Copy link
Member

Jolg42 commented Aug 19, 2022

Thanks @AndrewSouthpaw for all the work, I'm reviewing this now.

About

It also surprises me to check stdout, because it's the stdin that really matters.
The recommendation from Node.js docs is to check process.stdout.isTTY

https://nodejs.org/docs/latest-v14.x/api/tty.html

When Node.js detects that it is being run with a text terminal ("TTY") attached, process.stdin will, by default, be initialized as an instance of tty.ReadStream and both process.stdout and process.stderr will, by default, be instances of tty.WriteStream. The preferred method of determining whether Node.js is being run within a TTY context is to check that the value of the process.stdout.isTTY property is true

So what's the difference between stdin.isTTY and stdout.isTTY?

  • stdin.isTTY can tell us if the input is from a terminal (interactive) or from a | pipe (non interactive).
  • stdout.isTTY can tell us if the output is going to the terminal (interactive) or to a | pipe (non interactive).

The library we use prompts does the following:
https://github.com/terkelg/prompts#stdin-and-stdout

By default, prompts uses process.stdin for receiving input and process.stdout for writing output.

Checking the codebase, why do we use isCi?

  • handlePanic: to skip the "send us the crash logs" prompt
  • db push: to throw with CLI flags to use
  • migrate dev: to throw environment is non-interactive, which is not supported
  • migrate reset: to throw environment is non-interactive, which is not supported

Notes:
is-interactive default to checking stdout.isTTY
https://github.com/sindresorhus/is-interactive/blob/main/index.js

Testing zx with prompts

console.log("process.stdin.isTTY", process.stdin.isTTY);
console.log("process.stdout.isTTY", process.stdout.isTTY);
const prompts = require("prompts");

(async () => {
  const response = await prompts({
    type: "number",
    name: "value",
    message: "How old are you?",
    validate: (value) => (value < 18 ? `Nightclub is 18+ only` : true),
  });

  console.log(response); // => { value: 24 }
})();
#!/usr/bin/env zx
import "zx/globals";

// ./index.mjs
// process.stdin.isTTY true
// process.stdout.isTTY true

// Piping
// echo "hey" | ./index.mjs
// process.stdin.isTTY undefined
// process.stdout.isTTY true

// Redirecting output
// ./index.mjs > test.txt
// process.stdin.isTTY true
// process.stdout.isTTY undefined

console.log("process.stdin.isTTY", process.stdin.isTTY);
console.log("process.stdout.isTTY", process.stdout.isTTY);

// Default is `.stdio('inherit', 'pipe', 'pipe')`.
console.log("default (inherit for stdin, pipe for stdout)");
await $`node isTTY.js`;
// process.stdin.isTTY true
// process.stdout.isTTY undefined
//
// Works in first and third"
// OK: ./index.mjs
// KO: echo "hey" | ./index.mjs
// OK: ./index.mjs > output.txt
// await $`node prompts.js`;

console.log("inherit for stdin and stdout");
await $`node isTTY.js`.stdio("inherit", "inherit");
// process.stdin.isTTY true
// process.stdout.isTTY true
//
// Works only only in first:
// OK: ./index.mjs
// KO: echo "hey" | ./index.mjs
// KO: ./index.mjs > output.txt
// await $`node prompts.js`.stdio("inherit", "inherit");

console.log("pipe for stdin and stdout");
await $`node isTTY.js`.stdio("pipe", "pipe");
// process.stdin.isTTY undefined
// process.stdout.isTTY undefined
//
// Doesn't work (non interactive!)
// await $`node prompts.js`.stdio("pipe", "pipe");

console.log("pipe for stdin - inherit for stdout");
await $`node isTTY.js`.stdio("pipe", "inherit");
// process.stdin.isTTY undefined
// process.stdout.isTTY true
//
// Doesn't work (non interactive!)
// await $`node prompts.js`.stdio("pipe", "inherit");

During testing I could confirm what you mentioned, that only stdin.isTTY needs to be true for the interactive prompt to work 🙌🏼

@Jolg42 Jolg42 added this to the 4.3.0 milestone Aug 19, 2022
@Jolg42
Copy link
Member

Jolg42 commented Aug 25, 2022

Note: random find in Inquirer.js
It has a skipTTYChecks option and defaults to check stdin.isTTY.

  // Inquirer 8.x:
  // opt.skipTTYChecks = opt.skipTTYChecks === undefined ? opt.input !== undefined : opt.skipTTYChecks;
  opt.skipTTYChecks = opt.skipTTYChecks === undefined ? true : opt.skipTTYChecks;

  // Default `input` to stdin
  const input = opt.input || process.stdin;

  // Check if prompt is being called in TTY environment
  // If it isn't return a failed promise
  if (!opt.skipTTYChecks && !input.isTTY) {
    const nonTtyError = new Error(
      'Prompts can not be meaningfully rendered in non-TTY environments'
    );
    nonTtyError.isTtyError = true;
    throw nonTtyError;
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. kind/bug A reported bug. team/schema Issue for team Schema. topic: cli topic: migrate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants