Merge `ColumnConfig` and `ColumnMapping` and harden them

[ppkarwasz]

The JDBC appender right now provides two ways to configure the column mapping: ColumnMapping (used also by other appenders) and the JDBC specific ColumnConfig. Neither of them is deprecated or recommended.

I would propose to:

• merge the functionality of these two configuration knobs,
• add a kill-switch that prevents users from unintentionally injecting SQL using these column mappings. A setting like literal="A great string" inserts A great string into the SQL statement without quoting it. This behavior is documented, but it would be probably safer to quote everything unless the user provides an additional property (e.g. quoteLiteral="false"). This would make the configuration more foolproof.

[vy]

I support the idea of merging the two.

I have my doubts about quoteLiteral. This implies we need to escape the typed literal, and AFAIK, escaping is a vendor-specific operation. Can we escape an SQL literal in vendor-neutral way without reaching out to extreme extents that might contain holes?

[ppkarwasz]

Since JDBC 4.3 (Java 9), there is a Statement#enquoteLiteral method that JDBC driver vendors must implement. Therefore we can delegate everything to that method.

[vy]

@ppkarwasz, thanks for the Statement#enquoteLiteral reference, I didn't know about it.

Though you state that it is available starting from Java 9, which makes it unusable for Log4j 2, which targets Java 8. Do I miss something?

[ppkarwasz]

This issue proposes backward-incompatible changes so it should target 3.x.

[garydgregory]

The JDBC appender right now provides two ways to configure the column mapping: ColumnMapping (used also by other appenders) and the JDBC specific ColumnConfig. Neither of them is deprecated or recommended.

I use ColumnMapping in a product and that's the only way it would work for our use case. So whatever you do, make sure you keep that functionality. Our config contains four (4) JDBC appenders that look like this:

    <Jdbc name="My-Appender-Name" tableName="MY_TABLE_NAME_1" ignoreExceptions="false">
      <PoolingDriver connectionString="${postgresql-url}" userName="${postgresql-user}" password="${postgresql-pass}">
        <!-- PoolableConnectionFactory default values -->
        <PoolableConnectionFactory 
          autoCommitOnReturn="true"
          cacheState="false"
          defaultAutoCommit="false"
          defaultCatalog=""
          defaultQueryTimeoutSeconds="0"
          defaultReadOnly="false"
          defaultTransactionIsolation="-1"
          fastFailValidation="true"
          maxConnLifetimeMillis="-1"
          maxOpenPreparedStatements="8"
          poolStatements="false"
          rollbackOnReturn="true"
          validationQuery=""
          validationQueryTimeoutSeconds="-1" 
        />
      </PoolingDriver>            
      <ColumnMapping name="version" />
      <ColumnMapping name="proxy_id" source="proxyId" />
      <ColumnMapping name="config_id" source="configId" />
      <ColumnMapping name="uuid" />
      <ColumnMapping name="api_key" source="apiKey" />
      <ColumnMapping name="instant_first" parameter="TO_TIMESTAMP(?::double precision / 1000)"/>
      <ColumnMapping name="instant_last" parameter="TO_TIMESTAMP(?::double precision / 1000)"/>
      <ColumnMapping name="client_local_address" source="client_localAddress" />
      <ColumnMapping name="client_local_port" source="client_localPort" />
      <ColumnMapping name="client_remote_address" source="client_remoteAddress" />
      <ColumnMapping name="client_remote_port" source="client_remotePort" />
      <ColumnMapping name="client_socket_timeout" source="client_socketTimeout" />
      <ColumnMapping name="client_ssl_protocol" source="client_ssl_protocol" />
      <ColumnMapping name="client_ssl_cipher_suite" source="client_ssl_cipher_suite" />
      <ColumnMapping name="method" source="httpRequest_method" />
      <ColumnMapping name="uri" source="httpRequest_uri" />
      <ColumnMapping name="protocol_name" source="httpRequest_protocolVersion_protocol" />
      <ColumnMapping name="protocol_major" source="httpRequest_protocolVersion_major" />
      <ColumnMapping name="protocol_minor" source="httpRequest_protocolVersion_minor" />
      <ColumnMapping name="header_accept" source="httpRequest_headers_accept" />
      <ColumnMapping name="header_host" source="httpRequest_headers_host" />
      <ColumnMapping name="header_connection" source="httpRequest_headers_connection" />
      <ColumnMapping name="header_user_agent" source="httpRequest_headers_user_agent" />
      <ColumnMapping name="header_accept_encoding" source="httpRequest_headers_accept_encoding" />
      <ColumnMapping name="header_transfer_encoding" source="httpRequest_headers_transfer_encoding" />
      <ColumnMapping name="header_content_type" source="httpRequest_headers_content_type" />
      <ColumnMapping name="header_content_length" source="httpRequest_header_content_length" parameter="cast(? as BIGINT)" />
      <ColumnMapping name="body" source="body" />
      <ColumnMapping name="throwable_class" source="throwableClass" />
      <ColumnMapping name="throwable_description" source="throwableDescription" />
      <ColumnMapping name="throwable_stack_trace" source="throwableStackTrace" />
      <ColumnMapping name="policy_violation" source="policyViolation" />
      <ColumnMapping name="policy_violation_type" source="policyViolationType" />
      <ColumnMapping name="policy_violation_description" source="policyViolationDescription" />
      <MessageLayout />
      <Property name="My.Property.Name" />
    </Jdbc>

We use both PostgreSQL and MySQL.

[garydgregory]

unintentionally injecting SQL

Uh? How does that happen when we use a prepared statement? See JdbcDatabaseManager.connectAndPrepare()

[ppkarwasz]

I use ColumnMapping in a product and that's the only way it would work for our use case. So whatever you do, make sure you keep that functionality. Our config contains four (4) JDBC appenders that look like this:

ColumnMapping is the new configuration format that currently is common with the Cassandra Appender. Column is the one that we should consider removing in 3.x. Do you have an example that needs Column?

unintentionally injecting SQL

Uh? How does that happen when we use a prepared statement? See JdbcDatabaseManager.connectAndPrepare()

The SQL PreparedStatement is dynamically created, based on the column configuration. A configuration like:

<ColumnMapping name="test" literal="lala bala"/>

will create an invalid SQL statement, whereas:

<ColumnMapping name="test" value="${ctx:appId}"/>

will require the value of appId to be quoted.

I added a warning in the documentation about this field. Is this enough?