Skip to content

settings over URI is broken#82137

Merged
CheSema merged 2 commits intomasterfrom
chesema-fix-CGI-params
Jun 19, 2025
Merged

settings over URI is broken#82137
CheSema merged 2 commits intomasterfrom
chesema-fix-CGI-params

Conversation

@CheSema
Copy link
Copy Markdown
Member

@CheSema CheSema commented Jun 18, 2025

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

when passing settings over URI the last value is considered

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jun 18, 2025

Workflow [PR], commit [83b2445]

Summary:

job_name test_name status info comment
Stateless tests (amd_binary, old analyzer, s3 storage, DatabaseReplicated, 2/2) failure
02443_detach_attach_partition FAIL

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Jun 18, 2025
@CheSema CheSema marked this pull request as draft June 18, 2025 23:03
@CheSema CheSema force-pushed the chesema-fix-CGI-params branch from 7262a43 to 6a1cc0d Compare June 18, 2025 23:08
output_format_parallel_formatting=1&output_format_parallel_formatting=0
output_format_parallel_formatting 0 1
output_format_parallel_formatting=0&output_format_parallel_formatting=1
output_format_parallel_formatting 0 1
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Here is a problem:
URI /?output_format_parallel_formatting=0&output_format_parallel_formatting=1
does not change the value.

@CheSema CheSema marked this pull request as ready for review June 18, 2025 23:15
@CheSema CheSema changed the title setting settings over URI is broken settings over URI is broken Jun 18, 2025
@tuanpach tuanpach self-assigned this Jun 18, 2025
/// Other than query parameters are treated as settings.
if (!customizeQueryParam(context, key, value))
settings_changes.push_back({key, value});
settings_changes.setSetting(key, value);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we throw an exception if the key already exists in the setting?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, we should not.

We exploit that fact in our tests.
When you do curl "${CLICKHOUSE_URL}&output_format_parallel_formatting=1"
CLICKHOUSE_URL could already has a lot of random settings, you do not know which one.
By adding new one at the end you expect that its last appearance would be taken.

@CheSema
Copy link
Copy Markdown
Member Author

CheSema commented Jun 19, 2025

@CheSema CheSema enabled auto-merge June 19, 2025 12:06
@CheSema CheSema added this pull request to the merge queue Jun 19, 2025
Merged via the queue into master with commit 7f15422 Jun 19, 2025
119 of 122 checks passed
@CheSema CheSema deleted the chesema-fix-CGI-params branch June 19, 2025 12:21
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants