Support command level URL query string parameters #510
Conversation
|
Hi @DarkWanderer , just checking in to see if you're still interested in these changes. The pull request is ready for review whenever you have the time. |
| public static string DefaultFormat => "RowBinaryWithNamesAndTypes"; | ||
|
|
||
| public IDictionary<string, object> CustomParameters { get; set; } | ||
| public IDictionary<string, object> ConnectionQueryStringParameters { get; set; } |
There was a problem hiding this comment.
This class doesn't need to know about existence of Command or ConnectionString. The custom parameters should be fed from outside - overriding the values as needed
There was a problem hiding this comment.
Good point. However, considering that this class is internal, I believe this implementation is easier to maintain.
There was a problem hiding this comment.
Having N properties instead of 1 is definitely not easier to maintain. Merging the dictionary can well be done at builder population stage rather than at URI build time.
There was a problem hiding this comment.
Merging this into a single dictionary would complicate unit tests. To properly test the override of command-level settings over connection-level ones, I'd need to create a connection with a command and mock an HttpClient. Since there is no Moq reference yet, what do you think is the most suitable approach here?
(sorry, didn’t realize the PR had already been merged)
| /// Gets collection of custom settings which will be passed as URL query string parameters. | ||
| /// </summary> | ||
| /// <remarks>Not thread-safe.</remarks> | ||
| public IDictionary<string, object> CustomSettings => customSettings ??= new Dictionary<string, object>(); |
There was a problem hiding this comment.
An autoproperty would be more efficient and idiomatic here:
public IDictionary<string, object> CustomSettings { get; } = new Dictionary<string, object>();
There was a problem hiding this comment.
You're right, the auto-property is more idiomatic. But is it truly more efficient here? Since commands are created frequently and settings are used only occasionally, I chose not to allocate the dictionary every time to reduce command initialization time.
There was a problem hiding this comment.
The cost of having an NRE at the wrong time will definitely be more than the cost of creating an extra Dictionary object
|
Greetings. Sorry for slow response - real life gets in the way. Left a few minor comments |
|
I feel some things can be polished further, but these should not prevent the feature to be available. Thank you for your contribution! |
Discussed in #509 , closes #86
This PR introduces
CustomSettingsattribute forClickHouseCommandclass.CustomSettingsare appended as URL query string parameters.ClickHouseConnectionfor consistency.CustomSettingswill take precedence over analogous connection-level settings.ClickHouseUriBuildertests were added. Some of them demonstrate that both command-level and connection-level custom settings can override common parameters, such asquery,session_id, etc. This behavior might be unintended.