This is an automated email from the ASF dual-hosted git repository.
panjuan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/shardingsphere.git
The following commit(s) were added to refs/heads/master by this push:
new 29bbb99 Replace magic values with enums in
PostgreSQLErrorResponsePacket (#10167)
29bbb99 is described below
commit 29bbb992ae0ca17f8da677fcb815e770988c73ee
Author: 吴伟杰 <[email protected]>
AuthorDate: Sat Apr 24 10:49:24 2021 +0800
Replace magic values with enums in PostgreSQLErrorResponsePacket (#10167)
* Refactor PostgreSQLErrorResponsePacket
* Checkstyle and complete testcases
---
.../codec/PostgreSQLPacketCodecEngine.java | 4 ++-
.../generic/PostgreSQLErrorResponsePacket.java | 37 ++++++++++++----------
.../generic/PostgreSQLErrorResponsePacketTest.java | 14 ++++----
.../PostgreSQLUnsupportedCommandExecutor.java | 3 +-
.../postgresql/err/PostgreSQLErrPacketFactory.java | 18 +++++------
.../err/PostgreSQLErrPacketFactoryTest.java | 5 +--
6 files changed, 44 insertions(+), 37 deletions(-)
diff --git
a/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/main/java/org/apache/shardingsphere/db/protocol/postgresql/codec/PostgreSQLPacketCodecEngine.java
b/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/main/java/org/apache/shardingsphere/db/protocol/postgresql/codec/PostgreSQLPacketCodecEngine.java
index 85af5a1..fbdf12b 100644
---
a/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/main/java/org/apache/shardingsphere/db/protocol/postgresql/codec/PostgreSQLPacketCodecEngine.java
+++
b/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/main/java/org/apache/shardingsphere/db/protocol/postgresql/codec/PostgreSQLPacketCodecEngine.java
@@ -21,6 +21,7 @@ import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelHandlerContext;
import org.apache.shardingsphere.db.protocol.codec.DatabasePacketCodecEngine;
import
org.apache.shardingsphere.db.protocol.postgresql.constant.PostgreSQLErrorCode;
+import
org.apache.shardingsphere.db.protocol.postgresql.constant.PostgreSQLMessageSeverityLevel;
import
org.apache.shardingsphere.db.protocol.postgresql.packet.identifier.PostgreSQLIdentifierPacket;
import
org.apache.shardingsphere.db.protocol.postgresql.packet.PostgreSQLPacket;
import
org.apache.shardingsphere.db.protocol.postgresql.packet.generic.PostgreSQLErrorResponsePacket;
@@ -70,7 +71,8 @@ public final class PostgreSQLPacketCodecEngine implements
DatabasePacketCodecEng
// CHECKSTYLE:ON
payload.getByteBuf().resetWriterIndex();
// TODO consider what severity to use
- PostgreSQLErrorResponsePacket errorResponsePacket =
PostgreSQLErrorResponsePacket.newBuilder("ERROR",
PostgreSQLErrorCode.SYSTEM_ERROR.getErrorCode(), ex.getMessage()).build();
+ PostgreSQLErrorResponsePacket errorResponsePacket =
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.ERROR,
PostgreSQLErrorCode.SYSTEM_ERROR, ex.getMessage())
+ .build();
errorResponsePacket.write(payload);
} finally {
if (message instanceof PostgreSQLIdentifierPacket) {
diff --git
a/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/main/java/org/apache/shardingsphere/db/protocol/postgresql/packet/generic/PostgreSQLErrorResponsePacket.java
b/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/main/java/org/apache/shardingsphere/db/protocol/postgresql/packet/generic/PostgreSQLErrorResponsePacket.java
index 955be1b..9366ce2 100644
---
a/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/main/java/org/apache/shardingsphere/db/protocol/postgresql/packet/generic/PostgreSQLErrorResponsePacket.java
+++
b/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/main/java/org/apache/shardingsphere/db/protocol/postgresql/packet/generic/PostgreSQLErrorResponsePacket.java
@@ -19,6 +19,8 @@ package
org.apache.shardingsphere.db.protocol.postgresql.packet.generic;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
+import
org.apache.shardingsphere.db.protocol.postgresql.constant.PostgreSQLErrorCode;
+import
org.apache.shardingsphere.db.protocol.postgresql.constant.PostgreSQLMessageSeverityLevel;
import
org.apache.shardingsphere.db.protocol.postgresql.packet.identifier.PostgreSQLIdentifierPacket;
import
org.apache.shardingsphere.db.protocol.postgresql.packet.identifier.PostgreSQLIdentifierTag;
import
org.apache.shardingsphere.db.protocol.postgresql.packet.identifier.PostgreSQLMessagePacketType;
@@ -105,12 +107,25 @@ public final class PostgreSQLErrorResponsePacket
implements PostgreSQLIdentifier
* Create PostgreSQL error response packet builder with required arguments.
*
* @param severity severity
+ * @param postgreSQLErrorCode PostgreSQL error code
+ * @param message message
+ * @return PostgreSQL error response packet builder
+ * @see <a
href="https://www.postgresql.org/docs/12/protocol-error-fields.html";>52.8.
Error and Notice Message Fields</a>
+ */
+ public static Builder newBuilder(final PostgreSQLMessageSeverityLevel
severity, final PostgreSQLErrorCode postgreSQLErrorCode, final String message) {
+ return newBuilder(severity, postgreSQLErrorCode.getErrorCode(),
message);
+ }
+
+ /**
+ * Create PostgreSQL error response packet builder with required arguments.
+ *
+ * @param severity severity
* @param code code
* @param message message
* @return PostgreSQL error response packet builder
* @see <a
href="https://www.postgresql.org/docs/12/protocol-error-fields.html";>52.8.
Error and Notice Message Fields</a>
*/
- public static Builder newBuilder(final String severity, final String code,
final String message) {
+ public static Builder newBuilder(final PostgreSQLMessageSeverityLevel
severity, final String code, final String message) {
return new Builder(severity, code, message);
}
@@ -118,29 +133,17 @@ public final class PostgreSQLErrorResponsePacket
implements PostgreSQLIdentifier
private final Map<Character, String> fields = new LinkedHashMap<>(16,
1);
- private Builder(final String severity, final String code, final String
message) {
- Preconditions.checkArgument(!Strings.isNullOrEmpty(severity), "The
severity is always present!");
+ private Builder(final PostgreSQLMessageSeverityLevel severity, final
String code, final String message) {
+ Preconditions.checkArgument(null != severity, "The severity is
always present!");
Preconditions.checkArgument(!Strings.isNullOrEmpty(code), "The
SQLSTATE code is always present!");
Preconditions.checkArgument(!Strings.isNullOrEmpty(message), "The
message is always present!");
- fields.put(FIELD_TYPE_SEVERITY, severity);
+ fields.put(FIELD_TYPE_SEVERITY, severity.name());
+ fields.put(FIELD_TYPE_SEVERITY_NON_LOCALIZED, severity.name());
fields.put(FIELD_TYPE_CODE, code);
fields.put(FIELD_TYPE_MESSAGE, message);
}
/**
- * Set severity non localized.
- *
- * @param severityNonLocalized severity non localized
- * @return PostgreSQL error response packet builder
- */
- public Builder severityNonLocalized(final String severityNonLocalized)
{
- if (!Strings.isNullOrEmpty(severityNonLocalized)) {
- fields.put(FIELD_TYPE_SEVERITY_NON_LOCALIZED,
severityNonLocalized);
- }
- return this;
- }
-
- /**
* Set detail.
*
* @param detail detail
diff --git
a/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/test/java/org/apache/shardingsphere/db/protocol/postgresql/packet/generic/PostgreSQLErrorResponsePacketTest.java
b/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/test/java/org/apache/shardingsphere/db/protocol/postgresql/packet/generic/PostgreSQLErrorResponsePacketTest.java
index 885afd3..04c46fb 100644
---
a/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/test/java/org/apache/shardingsphere/db/protocol/postgresql/packet/generic/PostgreSQLErrorResponsePacketTest.java
+++
b/shardingsphere-db-protocol/shardingsphere-db-protocol-postgresql/src/test/java/org/apache/shardingsphere/db/protocol/postgresql/packet/generic/PostgreSQLErrorResponsePacketTest.java
@@ -17,6 +17,8 @@
package org.apache.shardingsphere.db.protocol.postgresql.packet.generic;
+import
org.apache.shardingsphere.db.protocol.postgresql.constant.PostgreSQLErrorCode;
+import
org.apache.shardingsphere.db.protocol.postgresql.constant.PostgreSQLMessageSeverityLevel;
import
org.apache.shardingsphere.db.protocol.postgresql.payload.PostgreSQLPacketPayload;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -25,6 +27,7 @@ import org.mockito.junit.MockitoJUnitRunner;
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
@RunWith(MockitoJUnitRunner.class)
@@ -36,7 +39,7 @@ public final class PostgreSQLErrorResponsePacketTest {
@Test
public void assertToServerErrorMessage() {
PostgreSQLErrorResponsePacket responsePacket =
createErrorResponsePacket();
- String expectedMessage = "SFATAL\0C3D000\0Mdatabase \"test\" does not
exist\0VERROR\0Ddetail\0Hhint\0P1\0p2\0qinternal query\0"
+ String expectedMessage = "SFATAL\0VFATAL\0C3D000\0Mdatabase \"test\"
does not exist\0Ddetail\0Hhint\0P1\0p2\0qinternal query\0"
+ "Wwhere\0stest\0ttable\0ccolumn\0ddata
type\0nconstraint\0Ffile\0L3\0Rroutine";
assertThat(responsePacket.toServerErrorMessage(), is(expectedMessage));
}
@@ -46,13 +49,12 @@ public final class PostgreSQLErrorResponsePacketTest {
PostgreSQLErrorResponsePacket responsePacket =
createErrorResponsePacket();
responsePacket.write(payload);
verify(payload).writeInt1(PostgreSQLErrorResponsePacket.FIELD_TYPE_SEVERITY);
- verify(payload).writeStringNul("FATAL");
+
verify(payload).writeInt1(PostgreSQLErrorResponsePacket.FIELD_TYPE_SEVERITY_NON_LOCALIZED);
+ verify(payload, times(2)).writeStringNul("FATAL");
verify(payload).writeInt1(PostgreSQLErrorResponsePacket.FIELD_TYPE_CODE);
verify(payload).writeStringNul("3D000");
verify(payload).writeInt1(PostgreSQLErrorResponsePacket.FIELD_TYPE_MESSAGE);
verify(payload).writeStringNul("database \"test\" does not exist");
-
verify(payload).writeInt1(PostgreSQLErrorResponsePacket.FIELD_TYPE_SEVERITY_NON_LOCALIZED);
- verify(payload).writeStringNul("ERROR");
verify(payload).writeInt1(PostgreSQLErrorResponsePacket.FIELD_TYPE_DETAIL);
verify(payload).writeStringNul("detail");
verify(payload).writeInt1(PostgreSQLErrorResponsePacket.FIELD_TYPE_HINT);
@@ -85,8 +87,8 @@ public final class PostgreSQLErrorResponsePacketTest {
}
private PostgreSQLErrorResponsePacket createErrorResponsePacket() {
- return PostgreSQLErrorResponsePacket.newBuilder("FATAL", "3D000",
"database \"test\" does not
exist").severityNonLocalized("ERROR").detail("detail").hint("hint").position(1)
- .internalQueryAndInternalPosition("internal query",
2).where("where").schemaName("test").tableName("table").columnName("column").dataTypeName("data
type")
+ return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.FATAL,
PostgreSQLErrorCode.INVALID_CATALOG_NAME, "database \"test\" does not
exist").detail("detail")
+
.hint("hint").position(1).internalQueryAndInternalPosition("internal query",
2).where("where").schemaName("test").tableName("table").columnName("column").dataTypeName("data
type")
.constraintName("constraint").file("file").line(3).routine("routine").build();
}
}
diff --git
a/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/generic/PostgreSQLUnsupportedCommandExecutor.java
b/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/generic/PostgreSQLUnsupportedCommandExecutor.java
index 615aa55..cd90feb 100644
---
a/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/generic/PostgreSQLUnsupportedCommandExecutor.java
+++
b/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/generic/PostgreSQLUnsupportedCommandExecutor.java
@@ -19,6 +19,7 @@ package
org.apache.shardingsphere.proxy.frontend.postgresql.command.generic;
import org.apache.shardingsphere.db.protocol.packet.DatabasePacket;
import
org.apache.shardingsphere.db.protocol.postgresql.constant.PostgreSQLErrorCode;
+import
org.apache.shardingsphere.db.protocol.postgresql.constant.PostgreSQLMessageSeverityLevel;
import
org.apache.shardingsphere.db.protocol.postgresql.packet.generic.PostgreSQLErrorResponsePacket;
import
org.apache.shardingsphere.proxy.frontend.command.executor.CommandExecutor;
@@ -33,7 +34,7 @@ public final class PostgreSQLUnsupportedCommandExecutor
implements CommandExecut
@Override
public Collection<DatabasePacket<?>> execute() {
// TODO consider what severity and error code to use
- PostgreSQLErrorResponsePacket packet =
PostgreSQLErrorResponsePacket.newBuilder("ERROR",
PostgreSQLErrorCode.FEATURE_NOT_SUPPORTED.getErrorCode(),
+ PostgreSQLErrorResponsePacket packet =
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.ERROR,
PostgreSQLErrorCode.FEATURE_NOT_SUPPORTED,
PostgreSQLErrorCode.FEATURE_NOT_SUPPORTED.getConditionName()).build();
return Collections.singletonList(packet);
}
diff --git
a/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/err/PostgreSQLErrPacketFactory.java
b/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/err/PostgreSQLErrPacketFactory.java
index e1a3392..5be6ee3 100644
---
a/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/err/PostgreSQLErrPacketFactory.java
+++
b/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/err/PostgreSQLErrPacketFactory.java
@@ -49,27 +49,25 @@ public final class PostgreSQLErrPacketFactory {
}
if (cause instanceof SQLException) {
// TODO consider what severity to use
- return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.ERROR.name(),
((SQLException) cause).getSQLState(), cause.getMessage()).build();
+ return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.ERROR,
((SQLException) cause).getSQLState(), cause.getMessage()).build();
}
if (cause instanceof InvalidAuthorizationSpecificationException) {
- return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.FATAL.name(),
PostgreSQLErrorCode.INVALID_AUTHORIZATION_SPECIFICATION.getErrorCode(),
cause.getMessage())
-
.severityNonLocalized(PostgreSQLMessageSeverityLevel.FATAL.name()).build();
+ return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.FATAL,
PostgreSQLErrorCode.INVALID_AUTHORIZATION_SPECIFICATION,
cause.getMessage()).build();
}
if (cause instanceof PostgreSQLProtocolViolationException) {
- return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.FATAL.name(),
PostgreSQLErrorCode.PROTOCOL_VIOLATION.getErrorCode(),
+ return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.FATAL,
PostgreSQLErrorCode.PROTOCOL_VIOLATION,
String.format("expected %s response, got message type %s",
((PostgreSQLProtocolViolationException) cause).getExpectedMessageType(),
- ((PostgreSQLProtocolViolationException)
cause).getActualMessageType())).severityNonLocalized(PostgreSQLMessageSeverityLevel.FATAL.name()).build();
+ ((PostgreSQLProtocolViolationException)
cause).getActualMessageType())).build();
}
if (cause instanceof PostgreSQLAuthenticationException) {
- return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.FATAL.name(),
((PostgreSQLAuthenticationException) cause).getErrorCode().getErrorCode(),
cause.getMessage())
- .build();
+ return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.FATAL,
((PostgreSQLAuthenticationException) cause).getErrorCode(),
cause.getMessage()).build();
}
return createErrorResponsePacketForUnknownException(cause);
}
private static PostgreSQLErrorResponsePacket
createErrorResponsePacket(final ServerErrorMessage serverErrorMessage) {
- return
PostgreSQLErrorResponsePacket.newBuilder(serverErrorMessage.getSeverity(),
serverErrorMessage.getSQLState(), serverErrorMessage.getMessage())
-
.severityNonLocalized(serverErrorMessage.getSeverity()).detail(serverErrorMessage.getDetail()).hint(serverErrorMessage.getHint()).position(serverErrorMessage.getPosition())
+ return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.valueOf(serverErrorMessage.getSeverity()),
serverErrorMessage.getSQLState(), serverErrorMessage.getMessage())
+
.detail(serverErrorMessage.getDetail()).hint(serverErrorMessage.getHint()).position(serverErrorMessage.getPosition())
.internalQueryAndInternalPosition(serverErrorMessage.getInternalQuery(),
serverErrorMessage.getInternalPosition()).where(serverErrorMessage.getWhere())
.schemaName(serverErrorMessage.getSchema()).tableName(serverErrorMessage.getTable()).columnName(serverErrorMessage.getColumn()).dataTypeName(serverErrorMessage.getDatatype())
.constraintName(serverErrorMessage.getConstraint()).file(serverErrorMessage.getFile()).line(serverErrorMessage.getLine()).routine(serverErrorMessage.getRoutine()).build();
@@ -78,6 +76,6 @@ public final class PostgreSQLErrPacketFactory {
private static PostgreSQLErrorResponsePacket
createErrorResponsePacketForUnknownException(final Exception cause) {
// TODO add FIELD_TYPE_CODE for common error and consider what
severity to use
String message = Strings.isNullOrEmpty(cause.getLocalizedMessage()) ?
cause.toString() : cause.getLocalizedMessage();
- return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.ERROR.name(),
PostgreSQLErrorCode.SYSTEM_ERROR.getErrorCode(), message).build();
+ return
PostgreSQLErrorResponsePacket.newBuilder(PostgreSQLMessageSeverityLevel.ERROR,
PostgreSQLErrorCode.SYSTEM_ERROR, message).build();
}
}
diff --git
a/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/test/java/org/apache/shardingsphere/proxy/frontend/postgresql/err/PostgreSQLErrPacketFactoryTest.java
b/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/test/java/org/apache/shardingsphere/proxy/frontend/postgresql/err/PostgreSQLErrPacketFactoryTest.java
index 6bb7e94..9410ec9 100644
---
a/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/test/java/org/apache/shardingsphere/proxy/frontend/postgresql/err/PostgreSQLErrPacketFactoryTest.java
+++
b/shardingsphere-proxy/shardingsphere-proxy-frontend/shardingsphere-proxy-frontend-postgresql/src/test/java/org/apache/shardingsphere/proxy/frontend/postgresql/err/PostgreSQLErrPacketFactoryTest.java
@@ -17,6 +17,7 @@
package org.apache.shardingsphere.proxy.frontend.postgresql.err;
+import
org.apache.shardingsphere.db.protocol.postgresql.constant.PostgreSQLMessageSeverityLevel;
import
org.apache.shardingsphere.db.protocol.postgresql.packet.generic.PostgreSQLErrorResponsePacket;
import org.junit.Test;
import org.postgresql.util.PSQLException;
@@ -36,7 +37,7 @@ public final class PostgreSQLErrPacketFactoryTest {
@Test
public void assertPSQLExceptionWithServerErrorMessageNotNull() throws
NoSuchFieldException, IllegalAccessException {
ServerErrorMessage serverErrorMessage = mock(ServerErrorMessage.class);
- when(serverErrorMessage.getSeverity()).thenReturn("severity");
+
when(serverErrorMessage.getSeverity()).thenReturn(PostgreSQLMessageSeverityLevel.FATAL.name());
when(serverErrorMessage.getSQLState()).thenReturn("sqlState");
when(serverErrorMessage.getMessage()).thenReturn("message");
when(serverErrorMessage.getPosition()).thenReturn(1);
@@ -44,7 +45,7 @@ public final class PostgreSQLErrPacketFactoryTest {
Field packetField =
PostgreSQLErrorResponsePacket.class.getDeclaredField("fields");
packetField.setAccessible(true);
Map<Character, String> fields = (Map<Character, String>)
packetField.get(actual);
-
assertThat(fields.get(PostgreSQLErrorResponsePacket.FIELD_TYPE_SEVERITY),
is("severity"));
+
assertThat(fields.get(PostgreSQLErrorResponsePacket.FIELD_TYPE_SEVERITY),
is(PostgreSQLMessageSeverityLevel.FATAL.name()));
assertThat(fields.get(PostgreSQLErrorResponsePacket.FIELD_TYPE_CODE),
is("sqlState"));
assertThat(fields.get(PostgreSQLErrorResponsePacket.FIELD_TYPE_MESSAGE),
is("message"));
assertThat(fields.get(PostgreSQLErrorResponsePacket.FIELD_TYPE_POSITION),
is("1"));
