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

Auto-indenting Console requests converts triple-quoted strings to escaped quotes #44729

Closed
cjcenizal opened this issue Sep 4, 2019 · 7 comments · Fixed by #45249
Closed

Auto-indenting Console requests converts triple-quoted strings to escaped quotes #44729

cjcenizal opened this issue Sep 4, 2019 · 7 comments · Fixed by #45249
Labels
bug Fixes for quality problems that affect the customer experience Feature:Console Dev Tools Console Feature Feature:Dev Tools Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more

Comments

@cjcenizal
Copy link
Contributor

To reproduce, enter this input:

PUT bar/_doc/1
{
  "content": """trip"le"""
}

Auto-indent this request and it will be converted to:

PUT bar/_doc/1
{
  "content": "trip\"le"
}

Ideally, triple-quoted strings should remain triple-quoted.

@cjcenizal cjcenizal added bug Fixes for quality problems that affect the customer experience Feature:Console Dev Tools Console Feature Feature:Dev Tools Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Sep 4, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui

@jloleysens
Copy link
Contributor

jloleysens commented Sep 5, 2019

@cjcenizal Looking at the code on this, it looks like this behaviour may be a feature not a bug 😅 . That is if you've formatted it once, and format it again you should be seeing

{"content": "trip\"le"}
// =>
{
  "content": """trip"le"""
}
...

The idea probably being that if you want to have the body in a format you can paste into a non-triple quote friendly environment you can. If this is something we want to change (I do find this toggling a bit strange tbh) then perhaps we can discuss some alternative behaviour that would work best? Are you think de-indent but keep the """?

@jloleysens
Copy link
Contributor

@cjcenizal After messing around with this a bit I see that I misunderstood what you meant. The indentation toggle is something else - I was able to reproduce what you were describing though :). Including this fix here: #44646

@cjcenizal
Copy link
Contributor Author

@bleskes Do you know if the behavior described in this PR description is intended?

@bleskes
Copy link
Contributor

bleskes commented Sep 9, 2019

that sounds broken to me. I did have some minimums for automatically using """ - I think I had some minimal length requirement, of a maximum number of escaping. The thinking is that short strings can be consumed raw

@EmilBode
Copy link

Something else is broken the other way around:
Escaped quotes that should not be triple-quoted are.
I'm on 7.3 downloaded several weeks ago, and able to reproduce the original problem

Except when I use a different string, starting and ending with an escaped double quote:

PUT bar/_doc/1
{
  "content": "\"triple\""
}

auto-indents gives me here:

PUT bar/_doc/1
{
  "content": """"triple""""
}

Which generates an error, as we now have something starting and ending with four quotes, which is interpreted as a string "triple, followed by an unexpected extra double quote.

But I haven't tested this with the new patch yet.

@jloleysens
Copy link
Contributor

Hi @EmilBode thanks for reporting this! What you've described is a separate issue unfortunately and I could reproduce this in current master. I'll open another issue to make tracking this a bit cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Console Dev Tools Console Feature Feature:Dev Tools Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants