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

Fix quoting in to nuon and refactor quoting functions #14180

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

aionescu
Copy link
Contributor

@aionescu aionescu commented Oct 28, 2024

Description

This PR fixes the quoting and escaping of column names in to nuon. Before the PR, column names with quotes inside them would get quoted, but not escaped:

> { 'a"b': 2 } | to nuon
{ "a"b": 2 }

> { 'a"b': 2 } | to nuon | from nuon
Error:   × error when loading nuon text
   ╭─[entry #1:1:27]
 1 │ { "a\"b": 2 } | to nuon | from nuon
   ·                           ────┬────
   ·                               ╰── could not load nuon text
   ╰────

Error:   × error when parsing nuon text
   ╭─[entry #1:1:27]
 1 │ { "a\"b": 2 } | to nuon | from nuon
   ·                           ────┬────
   ·                               ╰── could not parse nuon text
   ╰────

Error:   × error when parsing
   ╭────
 1 │ {"a"b": 2}
   ·          ┬
   ·          ╰── Unexpected end of code.
   ╰────

> [['a"b']; [2] [3]] | to nuon
[["a"b"]; [2], [3]]

> [['a"b']; [2] [3]] | to nuon | from nuon
Error:   × error when loading nuon text
   ╭─[entry #1:1:32]
 1  [['a"b']; [2] [3]] | to nuon | from nuon
   ·                                ────┬────
   ·                                    ╰── could not load nuon text
   ╰────

Error:   × error when parsing nuon text
   ╭─[entry #1:1:32]
 1  [['a"b']; [2] [3]] | to nuon | from nuon
   ·                                ────┬────
   ·                                    ╰── could not parse nuon text
   ╰────

Error:   × error when parsing
   ╭────
 1  [["a"b"]; [2], [3]]
   ·                   ┬
   ·                   ╰── Unexpected end of code.
   ╰────

After this PR, the quote is escaped properly:

> { 'a"b': 2 } | to nuon
{ "a\"b": 2 }

> { 'a"b': 2 } | to nuon | from nuon
╭─────┬───╮
 a"b │ 2 │
╰─────┴───╯

> [['a"b']; [2] [3]] | to nuon
[["a\"b"]; [2], [3]]

> [['a"b']; [2] [3]] | to nuon | from nuon
╭─────╮
│ a"b 
├─────┤
   2 
   3 
╰─────╯

The cause of the issue was that to nuon simply wrapped column names in '"' instead of calling escape_quote_string.

As part of this change, I also moved the functions related to quoting (needs_quoting and escape_quote_string) into nu-utils, since previously they were defined in very ad-hoc places (and, in the case of escape_quote_string, it was defined multiple times with the same body!).

User-Facing Changes

to nuon now properly escapes quotes in column names.

Tests + Formatting

All tests pass, including workspace and stdlib tests.

After Submitting

@aionescu
Copy link
Contributor Author

aionescu commented Oct 28, 2024

I found the issue fixed by this PR while working on #13362.

The quoting functions refactor isn't strictly needed for this fix, but it will be helpful for the follow-up PR fixing the linked issue, so I decided to sneak it in here. If you think it's better to split it into a separate PR, please let me know.

Also, in nu-parser/src/deparse.rs there is a function string_should_be_quoted which implements different-but-very-similar logic to needs_quoting. Namely, it only checks for $ at the beginning of the string, and doesn't check for []{}| and a few other characters. I wasn't sure if I should replace its uses with needs_quoting, so I left it as-is. What do you think? Upon further testing it seems the difference in behavior is intended, since string_should_be_quoted is used for nu command-line arguments and quoting things like 123 would change their type.

lscolors = { workspace = true, default-features = false, features = ["nu-ansi-term"] }
log = { workspace = true }
num-format = { workspace = true }
once_cell = { workspace = true }
Copy link
Collaborator

@fdncred fdncred Oct 28, 2024

Choose a reason for hiding this comment

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

maybe i'm wrong but i thought this once_cell stuff was built into rust now?

rust-lang/rust#105587
https://doc.rust-lang.org/core/cell/struct.OnceCell.html
https://doc.rust-lang.org/core/cell/struct.LazyCell.html

maybe it's not the same? we probably already have a dependency on once_cell but it would be nice to remove all these (in a separate pr) if it's built in now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't know that. I just moved the dependency from crate nuon to crate nu-utils, but I'll check if it can be removed entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've taken a look, and we have multiple crates that depend on once_cell for Lazy. I'll make a separate PR to replace them with the now-built-in LazyCell once this gets merged.

@fdncred
Copy link
Collaborator

fdncred commented Oct 29, 2024

Sorry, there's a conflict now.

@aionescu
Copy link
Contributor Author

@fdncred Just fixed it.

@fdncred fdncred merged commit 79ea70d into nushell:main Oct 29, 2024
14 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Oct 29, 2024

Thanks

@github-actions github-actions bot added this to the v0.100.0 milestone Oct 29, 2024
@aionescu aionescu deleted the improve-quoting branch November 2, 2024 15:37
@fdncred fdncred added the pr:commands This PR changes our commands in some way label Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:commands This PR changes our commands in some way
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants