Reject malformed version strings in ClickHouseVersion parser#1981
Reject malformed version strings in ClickHouseVersion parser#1981il9ue wants to merge 4 commits into
Conversation
The parser reads the version left to right. The moment it hit anything that wasn't a number (including a blank piece), it assumed "this must be the suffix" and quit reading the rest. Because of this, typos slipped through. In 26..1 the blank between the dots, the x in 26.x.1, and the empty piece after the dot in 26.1. all got mistaken for a suffix, so the parser accepted a broken version instead of complaining The fix: a piece only counts as the suffix if it's the very last piece and isn't blank. That rules out the three cases above Normal inputs are unaffected: short values like 25.3 (used in the compatibility setting) and real builds like 26.3.1.20001.altinityantalya still parse exactly as before Closes ClickHouse#1831
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| "26.3.1.20001.altinityfips"); /// another suffix variant | ||
| EXPECT_EQ(ClickHouseVersion("26.1.altinityantalya").toString(), | ||
| "26.1.altinityantalya"); /// suffix after only two components (non-canonical, still valid) | ||
| EXPECT_EQ(ClickHouseVersion("26.altinityantalya").toString(), |
There was a problem hiding this comment.
this IS also a malformed input. there is no 26.altinityantalya.
| EXPECT_EQ(ClickHouseVersion("26.3.1.20001.altinityfips").toString(), | ||
| "26.3.1.20001.altinityfips"); /// another suffix variant | ||
| EXPECT_EQ(ClickHouseVersion("26.1.altinityantalya").toString(), | ||
| "26.1.altinityantalya"); /// suffix after only two components (non-canonical, still valid) |
There was a problem hiding this comment.
and this also is INVALID. if one is not sure what or why needs to be done -- one has to ask, not do something that does not make sense.
|
Also, this PR is AGAIN from the wrong repo, CI will never work. Please, pay attention. |
|
@zvonand Will check again, but let me ask, to fix this correctly: which suffixed forms should be considered valid:
I'll tighten the parser and tests accordingly, and re-open from the Altinity repo so CI runs. |
3 participants
The version parser stopped at the first non-numeric or empty token and treated the rest as the suffix, so
26..1,26.1.and26.x.1were accepted instead of rejected. A typo in thecompatibilitysetting could therefore apply the wrong profile silently.This restricts the suffix to a single non-empty trailing token, so malformed inputs throw BAD_ARGUMENTS again; valid versions and short forms like
25.3still parse. Adds a gtest for the parser.Closes #1831.
Changelog category (leave one):
CI/CD Options
Exclude tests:
Regression jobs to run: