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

Coerce string integer destinations to file descriptors #1180

Merged
merged 2 commits into from Nov 22, 2021
Merged

Conversation

@jsumners
Copy link
Member

@jsumners jsumners commented Oct 24, 2021

It's easy to accidentally write code like the following and expect it "to work":

'use strict'

const pino = require('pino')
const transport = pino.transport({
  targets: [
    { target: 'pino/file', level: 'debug', options: { destination: '2' } },
    { target: 'pino/file', level: 'info', options: { destination: '1' } }
  ]
})

const log = pino({ level: 'debug' }, transport)

log.error('something bad')
log.info('it works')

Notice that the destination in each target is a string integer. The clear intention is that they write to stderr and stdout respectively. This pull request accounts for this.

@jsumners jsumners requested a review from mcollina Oct 24, 2021
Copy link
Member

@mcollina mcollina left a comment

Should this be done inside pino.destination instead? why having it differently in transports vs there?

@jsumners
Copy link
Member Author

@jsumners jsumners commented Oct 25, 2021

Should this be done inside pino.destination instead? why having it differently in transports vs there?

No reason other than lack of understanding. I'll update when I get a chance.

@jsumners jsumners requested a review from mcollina Nov 20, 2021
@@ -262,7 +262,7 @@ const transport = pino.transport({
pino(transport)
```
The `options.destination` property may also be a number to represent a file descriptor. Typically this would be `1` to write to STDOUT or `2` to write to STDERR. If `options.destination` is not set, it defaults to `1` which means logs will be written to STDOUT.
The `options.destination` property may also be a number to represent a filedescriptor. Typically this would be `1` to write to STDOUT or `2` to write to STDERR. If `options.destination` is not set, it defaults to `1` which means logs will be written to STDOUT. If `options.destination` is a string integer, e.g. `'1'`, it will be coerced to a number and used as a file descriptor. If this is not desired, provide a full path, e.g. `/tmp/1`.
Copy link
Member

@mcollina mcollina Nov 20, 2021

Choose a reason for hiding this comment

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

The docs for pino.destination should be updated too.

Copy link
Member Author

@jsumners jsumners Nov 20, 2021

Choose a reason for hiding this comment

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

🤦‍♂️ will do. I literally went back to bed after this.

@mojavelinux
Copy link
Contributor

@mojavelinux mojavelinux commented Nov 20, 2021

I don't understand why we need to coerce a string to a number. Isn't the differentiation between a file path and file descriptor the type? What if you want to write to a file that is named "1". Now you lose that ability. For logs, that's not an unreasonable scenario (though probably rare) since logs are often sequenced in a directory. Can't we make the user get the type right?

@jsumners
Copy link
Member Author

@jsumners jsumners commented Nov 20, 2021

The included docs show how to write to a file named '1'.

@mojavelinux
Copy link
Contributor

@mojavelinux mojavelinux commented Nov 20, 2021

That just seems really weird to me that we are not accepting what the user is providing. Sure, we can try to analyze the value and figure whether it is one or the other, but the type was already doing that.

@jsumners
Copy link
Member Author

@jsumners jsumners commented Nov 20, 2021

No, it wasn't. This is a reduction in surprise. Passing a single integer as a destination is a mistake and most likely an attempt to write to stdout or stderr. If the string is accepted as-is, it is not defined what will actually happen. If it does write to a file, then that file can be in the current working directory or somewhere else entirely. If a file is the intended destination then an actual file path should be specified.

@mojavelinux
Copy link
Contributor

@mojavelinux mojavelinux commented Nov 20, 2021

We can agree to disagree, but I think the current behavior is the logical one. An integer is a file descriptor (which might be stdout, stderr, or one the application is managing). A string is a file path. As a user of pino, that's how I understood it and that contract makes complete sense to me.

@jsumners jsumners requested a review from mcollina Nov 21, 2021
Copy link
Member

@mcollina mcollina left a comment

lgtm

@mcollina mcollina merged commit 4b0009e into master Nov 22, 2021
15 checks passed
@mcollina mcollina deleted the coerce-fds branch Nov 22, 2021
@mojavelinux
Copy link
Contributor

@mojavelinux mojavelinux commented Dec 7, 2021

After sitting on this for awhile, I now agree this is consistent with the *nix convention. I retract my earlier objection. Thanks for making this improvement.

@jsumners
Copy link
Member Author

@jsumners jsumners commented Dec 7, 2021

After sitting on this for awhile, I now agree this is consistent with the *nix convention. I retract my earlier objection. Thanks for making this improvement.

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants