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

External data still broken after #22527 #23648

Closed
Felixoid opened this issue Apr 26, 2021 · 8 comments · Fixed by #27762
Closed

External data still broken after #22527 #23648

Felixoid opened this issue Apr 26, 2021 · 8 comments · Fixed by #27762
Assignees
Labels

Comments

@Felixoid
Copy link
Member

Describe the bug
After #22527 the post of external data still broken when there is only one line. When there are more then one, \r is appended to the latest.

Does it reproduce on recent release?
It's reproducible in 21.4.5.46
The list of releases

How to reproduce

docker run --rm --net=host --name=clickhouse yandex/clickhouse-server:21.4.5.46
$ printf 'string\nstring\n' | curl -F 'metrics_list=@-;' 'http://localhost:8123/?metrics_list_format=TSV&metrics_list_structure=Path+String&query=SELECT+*+FROM+metrics_list'
string
string
$ printf 'string\nstring' | curl -F 'metrics_list=@-;' 'http://localhost:8123/?metrics_list_format=TSV&metrics_list_structure=Path+String&query=SELECT+*+FROM+metrics_list'
string
string\r
$ printf 'string\n' | curl -F 'metrics_list=@-;' 'http://localhost:8123/?metrics_list_format=TSV&metrics_list_structure=Path+String&query=SELECT+*+FROM+metrics_list'
string
$ printf 'string' | curl -F 'metrics_list=@-;' 'http://localhost:8123/?metrics_list_format=TSV&metrics_list_structure=Path+String&query=SELECT+*+FROM+metrics_list'
Code: 117, e.displayText() = DB::Exception:
You have carriage return (\r, 0x0D, ASCII 13) at end of first row.
It's like your input data has DOS/Windows style line separators, that are illegal in TabSeparated format. You must transform your file to Unix format.
But if you really need carriage return at end of string value of last column, you need to escape it as \r.: (at row 1)

Row 1:
Column 0,   name: Path, type: String, parsed text: "string<CARRIAGE RETURN>"

: While executing SourceFromInputStream (version 21.4.5.46 (official build))

Expected behavior
I expect the first two and the last two commands would return the same output.

@Felixoid Felixoid added the bug Confirmed user-visible misbehaviour in official release label Apr 26, 2021
@filimonov filimonov added the comp-http http protocol related label Apr 28, 2021
@filimonov
Copy link
Contributor

Seems related to multipart parsing.

@filimonov
Copy link
Contributor

21.1 & 21.2 return correct result

@Felixoid
Copy link
Member Author

Yes, it works in 21.1 and 21.2, but broken in 21.3 and 21.4

@alexey-milovidov alexey-milovidov added backward compatibility and removed major bug Confirmed user-visible misbehaviour in official release labels Jun 13, 2021
@alexey-milovidov
Copy link
Member

No one guarantees that you can write TSV without a newline character on the last line.
That's why I change the category from "bug" to "compatibility".

@abyss7
Copy link
Contributor

abyss7 commented Jun 17, 2021

@Felixoid can you check v21.7? - I can't reproduce it.

@abyss7 abyss7 added the st-need-repro We were not able to reproduce the problem, please help us. label Jun 17, 2021
@Felixoid
Copy link
Member Author

21.7 is not available on docker hub, unfortunately

But it's still there for 21.6

for str in 'string\nstring\n' 'string\nstring' 'string\n' 'string'; do echo test $str:; printf $str curl -F 'metrics_list=@-;' 'http://localhost:8123/?metrics_list_format=TSV&metrics_list_structure=Path+String&query=SELECT+*+FROM+metrics_list'; done
test string
string
:
string
string
test string
string:
string
stringtest string
:
string
test string:
string(no EOL here)

@evillique
Copy link
Member

evillique commented Aug 16, 2021

After some tests it turned out that this issue was resolved in #24399. This fix wasn't yet included with 21.4.5.46 and some other versions, that's why you get this error.

@alexey-milovidov alexey-milovidov removed the st-need-repro We were not able to reproduce the problem, please help us. label Aug 17, 2021
@alexey-milovidov
Copy link
Member

@evillique Let's add the tests from this issue.

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

Successfully merging a pull request may close this issue.

6 participants