Changing Permit redirection #1499
Conversation
This is based on https://jira.mariadb.org/browse/MDEV-15935: first OK_Packet can contain variable `redirect_url` using "mariadb/mysql://[{user}[:{password}]@]{host}[:{port}]/[{db}[?{opt1}={value1}[&{opt2}={value2}]]]']" format. Signed-off-by: rusher <diego.dupin@gmail.com>
|
This is done for either pool and single connection, contrary to previous implementation. I don't know why there was a restriction like this. Logger messages from previous pool/single connection are keeped, but it might be a good idea to have a simplier implemenation |
src/MySqlConnector/Logging/Log.cs
Outdated
| [LoggerMessage(EventIds.HasServerRedirectionHeader, LogLevel.Trace, "{poolPrefix}Session {SessionId} has server redirection header {Header}")] | ||
| public static partial void HasServerRedirectionHeader(ILogger logger, string poolPrefix, string sessionId, string header); |
There was a problem hiding this comment.
The log field names are significant, and are used for "structured logging" within Microsoft.Extensions.Logging; see https://learn.microsoft.com/en-us/aspnet/core/fundamentals/logging/?view=aspnetcore-8.0#log-message-template.
We can't just change them for better code reuse (thinking of them like string concatenation).
I don't remember why, either. Based on the commit message for 48c6754 it looks like a bugfix introduced this change, and before that redirection was implemented for pooled and non-pooled. |
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
|
|
||
| namespace IntegrationTests; | ||
|
|
||
| public class RedirectionTests : IClassFixture<DatabaseFixture>, IDisposable |
| try | ||
| { | ||
| m_database.Connection.Execute( | ||
| $"set @@global.redirect_url=\"mariadb://{initialServer}:{initialPort}\""); |
There was a problem hiding this comment.
Looks like this fails with MariaDB 11.4 since it does accept this global variable, but doesn't include it in the initial OK packet.
There was a problem hiding this comment.
Usually I handle server-specific functionality by adding a new enum to ServerFeatures and skipping the test based on whether that's set in config.json.
change redirection logging Signed-off-by: rusher <diego.dupin@gmail.com>
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
|
I'm getting errors in the |
| { | ||
| OwningConnection = new WeakReference<MySqlConnection>(this), | ||
| }; | ||
| var session = await ServerSession.ConnectAndRedirectAsync(() => new ServerSession(m_logger), m_logger, null, connectionSettings, loadBalancer, this, null, startingTimestamp, null, actualIOBehavior, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
| var session = await ServerSession.ConnectAndRedirectAsync(() => new ServerSession(m_logger), m_logger, null, connectionSettings, loadBalancer, this, null, startingTimestamp, null, actualIOBehavior, cancellationToken).ConfigureAwait(false); | |
| var session = await ServerSession.ConnectAndRedirectAsync(() => new ServerSession(m_logger), m_logger, null, connectionSettings, loadBalancer, this, null, startingTimestamp, null, actualIOBehavior, connectToken).ConfigureAwait(false); |
See line 1073 below.
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
| @@ -628,6 +628,8 @@ public override string ConnectionString | |||
| } | |||
| } | |||
|
|
|||
| public string? SessionConnectionString => m_session?.ConnectionString; | |||
There was a problem hiding this comment.
Is this just for tests? We shouldn't add a new public property just to make testing easier; that will imply a commitment to support this.
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
Introduce IConnectionPoolMetadata to unify the code between pooled and non-pooled sessions. Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
This standardises all redirection logging to use the MySqlConnection logger, whether pooled or non-pooled. Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
Use ServerFeatures.Redirection to ignore this test on servers that don't support it. Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
|
Fixed in 2.4.0. |
Permit redirection, based on mariadb implementation https://jira.mariadb.org/browse/MDEV-15935: first OK_Packet can contain variable
redirect_urlusing "mariadb/mysql://[{user}[:{password}]@]{host}[:{port}]/[{db}[?{opt1}={value1}[&{opt2}={value2}]]]']" format.