You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by pa...@apache.org on 2021/04/24 02:50:06 UTC
[shardingsphere] branch master updated: Replace magic values with
enums in PostgreSQLErrorResponsePacket (#10167)
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: 吴伟杰 <wu...@apache.org>
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"));