Implement IDNA and punycode encoding/decoding (2nd attempt) #58454

rschu1ze · 2024-01-03T13:10:09Z

This PR reverts #58277 which reverted #57969 and addresses the feedback.

Original PR description:
Inspired by #34439, especially #34439 (comment), which was dormant for too long. This PR also uses the ada-idna library instead of a 15 year old homegrown implementation.

More specifically, this PR

Reverts the recent revert of the punycode encoding/decoding function (see the discussion after Implement punycode encoding/decoding #57969 (comment)): 2186aa8
Does some refactoring: 45613f1
Introduces OrNull variants of functions punycodeEncode/Decode(): b5d7ea2
Does more refactoring: 9a49f0c
implements idnaEncode/Decode[OrNull](): f652c60

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Added functions punycodeEncode(), punycodeDecode(), idnaEncode() and idnaDecode() which are useful for translating international domain names to an ASCII representation according to the IDNA standard.

This reverts commit 345d29a.

…irectly

robot-clickhouse-ci-2 · 2024-01-03T13:12:28Z

This is an automated comment for commit dd2d9ff with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Successful checks

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker image for servers	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs Check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	✅ success
Mergeable Check	Checks if all other necessary checks are successful	✅ success
SQLTest	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
SQLancer	Fuzzing tests that detect logical bugs with SQLancer tool	✅ success
Sqllogic	Run clickhouse on the sqllogic test set against sqlite and checks that all statements are passed	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ success
Style Check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success

Check name	Description	Status
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	⏳ pending
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	❌ failure
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	❌ failure

vitlibar · 2024-01-03T21:52:06Z

src/Functions/punycode.cpp

+
+/// Implementation of
+/// - punycode(En|De)code[OrNull](), see [1]
+/// - idna(En|De)code[OrNull](), see [2, 3]


I understand about punycodeDecode but how punycodeEncode can fail? I mean why do we need punycodeEncodeOrNull?

As far as I see, the only way for punycode encoding to fail currently is the (weird) edge that the input is not UTF-8. But from a ClickHouse POV, the ada-idna library API simply returns "failed" and there could be further sources of errors in future (e.g. a failure to allocate memory). For these reasons, I guess we should keep punycodeEncodeOrNull(). I can make the docs more concise tomorrow.

From looking at the code of ada-idna it seems right now it returns "failed" statuses only when there are some coding errors, and not on allocation failures. If it can't allocate it will probably just throw std::bad_alloc.

We don't usually handle too much if the input is not a proper UTF-8 string, see upperUTF8 or base64Encode. I think it's better to be consistent.

Okay, I changed the code so that these functions remain:

punycodeEncode(), punycodeDecode() and tryPunycodeDecode()

idnaEncode(), tryIdnaEncode() and idnaDecode()

This is more consistent with other SQL functions.

Yes, it is still a bit unexpected why in the case of idna encoding may fail but this is required by the algorithm and I'll document it.

Shouldn't it be idnaEncode(), tryIdnaDecode() and idnaDecode()?

No. It is a bit confusing but it is really idnaEncode(), tryIdnaEncode() and idnaDecode().

There are actually strings that cannot be idna-encoded (in particular ones with xn-- prefix as far as I understand). idnaEncode() throws an exception then, tryIdnaEncode() returns an empty string.

And there is idnaDecode() but no tryIdnaDecode() because the IDNA decoding algorithm (as specified by the standard) requires that invalid input values are returned as is. There is no point in other error handling.

Oh, I see. It will probably also fail for long domains, for example

idnaEncode('1234567890123456789012345678901234567890123456789012345678901234')

src/Functions/punycode.cpp

vitlibar · 2024-01-04T10:28:55Z

src/Functions/punycode.cpp

+
+            const size_t utf8_length = ada::idna::utf8_length_from_utf32(value_utf32.data(), value_utf32.size());
+            value_utf8.resize(utf8_length);
+            ada::idna::utf32_to_utf8(value_utf32.data(), value_utf32.size(), value_utf8.data());


Probably it's better to reassign utf8_length to the result of ada::idna::utf32_to_utf8 here.

…evert

src/Functions/FunctionSqid.cpp

…evert

vitlibar · 2024-01-08T13:01:50Z

src/Functions/idna.cpp

+            const size_t value_length = offsets[row] - prev_offset - 1;
+
+            std::string_view value_view(value, value_length);
+            if (!value_view.empty()) /// to_ascii() expects non-empty input


Shouldn't idnaEncode throw an exception for an empty input?

Empty input is an edge case which is generally legal. ada::idna::to_ascii() handles empty inputs gracefully (it doesn't throw, it returns an empty output). The comment on l. 65 is only there because the API docs say so and I wanted to program against the API, not the implementation. In any case, it slightly simplifies the actual error handling (l. 68ff) which would otherwise be ambiguous in the empty string case.

alexey-milovidov · 2024-01-14T23:01:51Z

It works! https://play.clickhouse.com/play?user=play#U0VMRUNUIHJhbmssIGlkbmFEZWNvZGUoZG9tYWluKSBGUk9NIG1pbmljcmF3bCBXSEVSRSBkb21haW4gTElLRSAnJXhuLS0lJyBPUkRFUiBCWSByYW5rIExJTUlUIDEwMDA=

rschu1ze added 4 commits January 3, 2024 08:03

Revert "Revert "Implement punycode encoding/decoding""

2186aa8

This reverts commit 345d29a.

Refactor punycodeEncode/Decode() functions to inerit from IFunction d…

45613f1

…irectly

Introduce OrNull() overloads for punycodeEncode/Decode()

b5d7ea2

Factorize common code

9a49f0c

robot-clickhouse-ci-2 added pr-feature Pull request with new product feature submodule changed At least one submodule changed in this PR. labels Jan 3, 2024

vitlibar self-assigned this Jan 3, 2024

Implement idnaEn/Decode()

f652c60

rschu1ze changed the title *wip* Implement IDNA and punycode encoding/decoding (2nd attempt) Implement IDNA and punycode encoding/decoding (2nd attempt) Jan 3, 2024

rschu1ze marked this pull request as ready for review January 3, 2024 18:14

rschu1ze mentioned this pull request Jan 3, 2024

Update link to PR in ClickHouse ada-url/idna#40

Merged

Fix spelling

a813431

vitlibar reviewed Jan 3, 2024

View reviewed changes

vitlibar reviewed Jan 4, 2024

View reviewed changes

src/Functions/punycode.cpp Outdated Show resolved Hide resolved

vitlibar reviewed Jan 4, 2024

View reviewed changes

rschu1ze added 2 commits January 5, 2024 12:18

Incorporate review feedback

2cc2a90

Merge remote-tracking branch 'rschu1ze/master' into punycode-revert-r…

8169b3e

…evert

rschu1ze force-pushed the punycode-revert-revert branch from 2161f01 to 8169b3e Compare January 5, 2024 12:19

rschu1ze commented Jan 5, 2024

View reviewed changes

src/Functions/FunctionSqid.cpp Show resolved Hide resolved

rschu1ze added 2 commits January 7, 2024 08:08

Merge remote-tracking branch 'rschu1ze/master' into punycode-revert-r…

d54e500

…evert

Minor fixes

dd2d9ff

vitlibar reviewed Jan 8, 2024

View reviewed changes

vitlibar approved these changes Jan 8, 2024

View reviewed changes

rschu1ze merged commit fb4fd29 into ClickHouse:master Jan 8, 2024

Implement IDNA and punycode encoding/decoding (2nd attempt) #58454

Implement IDNA and punycode encoding/decoding (2nd attempt) #58454

Uh oh!

Conversation

rschu1ze commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Uh oh!

robot-clickhouse-ci-2 commented Jan 3, 2024 • edited by robot-ch-test-poll2 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitlibar Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Jan 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rschu1ze commented Jan 3, 2024 •

edited

Loading

robot-clickhouse-ci-2 commented Jan 3, 2024 •

edited by robot-ch-test-poll2

Loading

vitlibar Jan 4, 2024 •

edited

Loading