feat: pass settings via interface to the client #301
Conversation
|
Warning Rate limit exceeded@simPod has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
WalkthroughThe codebase was refactored to replace all usages of raw associative arrays for passing settings to client and request interfaces with a new abstraction: Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test/Caller
participant Provider as ArraySettingsProvider/EmptySettingsProvider
participant Client as PsrClickHouseClient/PsrClickHouseAsyncClient
participant ReqSet as RequestSettings
Test->>Provider: new ArraySettingsProvider(settings)
Test->>Client: call method(..., Provider)
Client->>ReqSet: new RequestSettings(defaultProvider, Provider)
ReqSet->>defaultProvider: get()
ReqSet->>Provider: get()
ReqSet-->>Client: merged settings
Client-->>Test: result/output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–25 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #301 +/- ##
==========================================
+ Coverage 94.66% 94.70% +0.04%
==========================================
Files 40 42 +2
Lines 750 756 +6
==========================================
+ Hits 710 716 +6
Misses 40 40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (4)
src/Client/PsrClickHouseClient.php (4)
57-64: Invalid default object in parameter; switch to nullable + fallbackReplace the default with nullable and fallback at call site.
- public function executeQuery(string $query, SettingsProvider $settings = new EmptySettingsProvider()): void + public function executeQuery(string $query, ?SettingsProvider $settings = null): void { try { - $this->executeRequest($query, params: [], settings: $settings); + $this->executeRequest($query, params: [], settings: $settings ?? new EmptySettingsProvider()); } catch (UnsupportedParamType) { absurd(); } }
90-109: Invalid default object in parameter; switch to nullable + fallback- public function selectWithParams( - string $query, - array $params, - Format $outputFormat, - SettingsProvider $settings = new EmptySettingsProvider(), - ): Output { + public function selectWithParams( + string $query, + array $params, + Format $outputFormat, + ?SettingsProvider $settings = null, + ): Output { @@ $response = $this->executeRequest( @@ - settings: $settings, + settings: $settings ?? new EmptySettingsProvider(), );
206-211: Invalid default object in parameter; switch to nullable + fallback- public function insertWithFormat( + public function insertWithFormat( Table|string $table, Format $inputFormat, string $data, - SettingsProvider $settings = new EmptySettingsProvider(), + ?SettingsProvider $settings = null, ): void { @@ - $this->executeRequest( + $this->executeRequest( @@ - settings: $settings, + settings: $settings ?? new EmptySettingsProvider(), );Also applies to: 221-227
233-239: Invalid default object in parameter; switch to nullable + fallback (and pass non-null to RequestSettings)- public function insertPayload( + public function insertPayload( Table|string $table, Format $inputFormat, StreamInterface $payload, array $columns = [], - SettingsProvider $settings = new EmptySettingsProvider(), + ?SettingsProvider $settings = null, ): void { @@ - $request = $this->requestFactory->initRequest( + $request = $this->requestFactory->initRequest( new RequestSettings( $this->defaultSettings, - $settings, + $settings ?? new EmptySettingsProvider(), ), ['query' => $sql], );Also applies to: 258-263
♻️ Duplicate comments (1)
src/Client/PsrClickHouseAsyncClient.php (1)
35-36: Same note about “new” in parameter defaults as in the interface.Constructor default new EmptySettingsProvider() also depends on PHP ≥ 8.1 support.
🧹 Nitpick comments (7)
tests/Client/SelectTest.php (1)
172-172: LGTM: settings passed via provider.Call now uses ArraySettingsProvider as intended. Optionally use a named argument for clarity: settings: new ArraySettingsProvider([...]).
src/Settings/EmptySettingsProvider.php (1)
9-12: Add PHPStan return annotation for consistency.Mirror the interface’s phpstan alias on the concrete implementation to help static analysis.
- public function get(): array + /** @phpstan-return Settings */ + public function get(): arraytests/Client/Http/RequestSettingsTest.php (1)
18-20: LGTM: Merge semantics covered.Test validates override (database) and retention (a/b). Consider adding a case with overlapping non-database keys to assert second provider precedence and, if booleans are allowed, a boolean setting to ensure proper handling.
src/Settings/ArraySettingsProvider.php (1)
15-18: Nit: add phpstan return type for stronger static typing.Add an explicit phpstan return annotation to mirror the constructor param type alias.
- public function get(): array + /** @phpstan-return Settings */ + public function get(): arraysrc/Client/Http/RequestSettings.php (1)
12-13: Consider making settings readonly.This is config assembled in ctor; mark as readonly to prevent accidental mutation.
- public array $settings; + public readonly array $settings;src/Client/PsrClickHouseAsyncClient.php (1)
26-26: Nit: remove unused phpstan import-type.The imported Settings type alias isn’t referenced in this class.
-/** @phpstan-import-type Settings from SettingsProvider */src/Client/ClickHouseClient.php (1)
19-19: Nit: remove unused phpstan import-type.The Settings type alias isn’t referenced in this interface.
-/** @phpstan-import-type Settings from SettingsProvider */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Client/ClickHouseAsyncClient.php(1 hunks)src/Client/ClickHouseClient.php(5 hunks)src/Client/Http/RequestSettings.php(1 hunks)src/Client/PsrClickHouseAsyncClient.php(4 hunks)src/Client/PsrClickHouseClient.php(7 hunks)src/Settings/ArraySettingsProvider.php(1 hunks)src/Settings/EmptySettingsProvider.php(1 hunks)src/Settings/SettingsProvider.php(1 hunks)tests/Client/Http/RequestFactoryTest.php(3 hunks)tests/Client/Http/RequestSettingsTest.php(2 hunks)tests/Client/SelectTest.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
tests/Client/SelectTest.php (5)
src/Settings/ArraySettingsProvider.php (1)
ArraySettingsProvider(8-19)src/Client/ClickHouseClient.php (1)
select(52-56)src/Client/PsrClickHouseClient.php (1)
select(78-88)src/Format/JsonCompact.php (1)
JsonCompact(14-29)src/Output/JsonCompact.php (1)
JsonCompact(18-52)
tests/Client/Http/RequestFactoryTest.php (2)
src/Settings/ArraySettingsProvider.php (1)
ArraySettingsProvider(8-19)src/Settings/EmptySettingsProvider.php (1)
EmptySettingsProvider(7-13)
src/Settings/EmptySettingsProvider.php (2)
src/Settings/SettingsProvider.php (1)
get(11-11)src/Settings/ArraySettingsProvider.php (1)
get(15-18)
tests/Client/Http/RequestSettingsTest.php (1)
src/Settings/ArraySettingsProvider.php (1)
ArraySettingsProvider(8-19)
src/Settings/ArraySettingsProvider.php (5)
src/Client/Http/RequestSettings.php (1)
__construct(15-20)src/Client/PsrClickHouseClient.php (1)
__construct(47-55)src/Client/PsrClickHouseAsyncClient.php (1)
__construct(31-38)src/Settings/EmptySettingsProvider.php (1)
get(9-12)src/Settings/SettingsProvider.php (1)
get(11-11)
src/Client/ClickHouseAsyncClient.php (4)
src/Settings/EmptySettingsProvider.php (1)
EmptySettingsProvider(7-13)src/Client/ClickHouseClient.php (2)
select(52-56)selectWithParams(71-76)src/Client/PsrClickHouseClient.php (2)
select(78-88)selectWithParams(90-110)src/Client/PsrClickHouseAsyncClient.php (2)
select(45-51)selectWithParams(58-79)
src/Settings/SettingsProvider.php (2)
src/Settings/EmptySettingsProvider.php (1)
get(9-12)src/Settings/ArraySettingsProvider.php (1)
get(15-18)
🪛 GitHub Check: Infection
src/Client/PsrClickHouseClient.php
[warning] 71-71:
Escaped Mutant for Mutator "MethodCallRemoval":
@@ @@
}
public function executeQueryWithParams(string $query, array $params, SettingsProvider $settings = new EmptySettingsProvider()): void
{
-
$this->executeRequest($this->sqlFactory->createWithParameters($query, $params), params: $params, settings: $settings);
- }
public function select(string $query, Format $outputFormat, SettingsProvider $settings = new EmptySettingsProvider()): Output
{
🔇 Additional comments (18)
tests/Client/SelectTest.php (1)
17-17: Import update looks correct.Importing ArraySettingsProvider aligns the test with the refactored API.
tests/Client/Http/RequestFactoryTest.php (3)
16-17: Imports aligned with new settings abstraction.ArraySettingsProvider and EmptySettingsProvider imports are correct.
40-42: LGTM: RequestSettings constructed from providers.The test asserts the resulting query string; ensure RequestSettings preserves deterministic merge order so the params appear as expected.
If helpful, add a quick assertion that the internal merged settings array order is stable:
self::assertSame(['database', 'max_block_size'], array_keys($requestSettings->settings));
88-90: LGTM: Empty settings via explicit provider.Using EmptySettingsProvider removes ambiguity vs empty arrays and matches the API contract.
src/Settings/EmptySettingsProvider.php (1)
7-13: PHP minimum version requirement satisfied
- composer.json declares
"php": "^8.3", which exceeds the PHP 8.2+ requirement forreadonlyclasses.- No occurrences of
newin default parameter values were found in the codebase.No changes are needed.
tests/Client/Http/RequestSettingsTest.php (1)
9-9: Import is correct.ArraySettingsProvider import matches the constructor changes.
src/Settings/ArraySettingsProvider.php (1)
8-18: Solid, minimal provider implementation (immutable, typed).The class is clean, immutable, and aligns with the new SettingsProvider abstraction.
src/Client/Http/RequestSettings.php (1)
15-20: Merge precedence is correct (query overrides default).Using $querySettings->get() + $defaultSettings->get() ensures query keys take precedence over defaults.
src/Client/ClickHouseAsyncClient.php (2)
20-24: Consistent interface refactor to SettingsProvider.Signatures and generics docs remain coherent; aligns with the new abstraction across the codebase.
Also applies to: 32-37
20-24: No action needed: PHP ≥ 8.1 support is guaranteed by composer.json
Composer.json requires PHP ^8.3, which fully covers the “new in parameter defaults” feature added in PHP 8.1. Usingnew EmptySettingsProvider()as a default parameter is safe—no changes required.src/Client/PsrClickHouseAsyncClient.php (3)
27-27: LGTM: making the client readonly is a good fit.The class being readonly enforces immutability of promoted properties and matches its role.
45-49: Method signatures align with interface and propagate SettingsProvider correctly.Defaults and typing are consistent; select delegates to selectWithParams cleanly.
Also applies to: 62-63
90-92: Good: executeRequest now takes SettingsProvider directly.Keeps request preparation consistent with RequestSettings merging of default + query settings.
src/Client/ClickHouseClient.php (2)
26-26: Refactor looks consistent and improves API clarity.Moving to SettingsProvider across execute/select/insert methods standardizes configuration passing.
Also applies to: 52-56, 71-76, 88-94, 103-108, 118-124
26-26: PHP ≥8.0 support fornew Class()defaults is satisfied
Your composer.json specifies^8.3, so usingnew EmptySettingsProvider()as a default parameter is fully supported. All imports ofSettingsProviderandEmptySettingsProviderare in use. No further changes required.src/Client/PsrClickHouseClient.php (3)
24-26: New SettingsProvider imports look goodImporting EmptySettingsProvider and SettingsProvider aligns with the new abstraction.
282-296: Usage of SettingsProvider in executeRequest looks correctCombining default and per-call settings via RequestSettings and passing params through RequestOptions is coherent with the new abstraction.
41-41: Platform requirement verified – no changes neededThe project’s composer.json declares
"php": "^8.3", which already satisfies the PHP 8.2+ requirement forreadonly class. No adjustment is necessary.
simPod
force-pushed
the
settings
branch
3 times, most recently
from
61027e2 to
50ce893
Compare
simPod
force-pushed
the
master
branch
2 times, most recently
from
2aae305 to
8eb584f
Compare
simPod
force-pushed
the
settings
branch
3 times, most recently
from
371c751 to
35ccb68
Compare
Summary by CodeRabbit