Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/urlparser improve3 pr1 #2641

Merged
merged 6 commits into from Oct 17, 2022
Merged

Feature/urlparser improve3 pr1 #2641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d450878
refactoring: PGPropertyPasswordParser, PGPropertyServiceParser moved …
MarekUniq Oct 14, 2022
f5606c8
fix: loglevel -> loggerLevel (there is no property loglevel)
MarekUniq Oct 14, 2022
8635ab3
remove usage of environment variables (it eliminates need for cleanup)
MarekUniq Oct 14, 2022
18a8e20
replace hard-coded property values with references to PGProperty enum
MarekUniq Oct 14, 2022
6219a0b
rename: PGProperty.get() -> getOrDefault() because it returns value a…
MarekUniq Oct 14, 2022
c063b06
remove references to logLevel and loggerLevel
MarekUniq Oct 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 3 additions & 13 deletions appveyor.yml
View file
Open in desktop
Expand Up @@ -5,8 +5,6 @@ configuration: Release
clone_depth: 1
environment:
JAVA_HOME: 'C:\Program Files\Java\jdk1.8.0'
PGUSER: postgres
PGPASSWORD: Password12!
matrix:
- pg: 9.6.11-1
PlatformToolset: v120
Expand Down Expand Up @@ -62,29 +60,21 @@ before_build:
- ps: Add-Content -PATH "$env:pgroot\data\pg_hba.conf" "host replication all 127.0.0.1/32 trust"
- net start postgresql%x64%-%pgversion%
- path %pgroot%\bin;%PATH%
- SET PGUSER=postgres
- SET PGPASSWORD=Password12!
- mkdir %APPDATA%\postgresql
- echo *:*:*:postgres:Password12!> %APPDATA%\postgresql\pgpass.conf
- createuser -U postgres test
- psql -U postgres -c "alter user test with password 'test'" postgres
- psql -U postgres -c "alter user test with replication" postgres
- createuser -U postgres testsspi
- createdb -U postgres -O test test
- del %APPDATA%\postgresql\pgpass.conf

build_script:
- gradlew assemble

test_script:
- echo redirect escape ^> foo.bar
- echo privilegedPassword=Password12!>c:\projects\pgjdbc\build.local.properties
- set PGDATABASE=
- set PGHOST=
- set PGPASSFILE=
- set PGPASSWORD=
- set PGPORT=
- set PGSERVICE=
- set PGSERVICEFILE=
- set PGSYSCONFDIR=
- set PGUSER=
- gradlew test

cache:
Expand Down
3 changes: 0 additions & 3 deletions build.properties
View file
Open in desktop
@@ -1,6 +1,5 @@
# Default build parameters. These may be overridden by local configuration
# settings in build.local.properties.
# loggerLevel can be OFF, DEBUG, TRACE
#

server=localhost
Expand All @@ -16,7 +15,5 @@ privilegedUser=postgres
privilegedPassword=
sspiusername=testsspi
preparethreshold=5
loggerLevel=OFF
loggerFile=target/pgjdbc-tests.log
protocolVersion=0
sslpassword=sslpwd
5 changes: 2 additions & 3 deletions packaging/rpm/postgresql-jdbc.spec.tpl
View file
Open in desktop
Expand Up @@ -111,9 +111,9 @@ find -type f \( -name "*.jar" -or -name "*.class" \) | xargs rm -f
# Remove the test files depending on system-stubs-jupiter
rm src/test/java/org/postgresql/test/jdbc2/DriverTest.java \
src/test/java/org/postgresql/util/OSUtilTest.java \
src/test/java/org/postgresql/util/PGPropertyPasswordParserTest.java \
src/test/java/org/postgresql/util/StubEnvironmentAndProperties.java \
src/test/java/org/postgresql/util/PGPropertyServiceParserTest.java
src/test/java/org/postgresql/jdbcurlresolver/PgPassParserTest.java \
src/test/java/org/postgresql/jdbcurlresolver/PgServiceConfParserTest.java

# compat symlink: requested by dtardon (libreoffice), reverts part of
# 0af97ce32de877 commit.
Expand Down Expand Up @@ -145,7 +145,6 @@ password=test
privilegedUser=$PGTESTS_ADMIN
privilegedPassword=$PGTESTS_ADMINPASS
preparethreshold=5
loglevel=0
protocolVersion=0
EOF

Expand Down
1 change: 0 additions & 1 deletion pgjdbc-osgi-test/src/test/java/org/postgresql/test/osgi/DataSourceFactoryTest.java
View file
Open in desktop
Expand Up @@ -120,7 +120,6 @@ private String getUrl() {
+ p.get("server") + ":"
+ p.get("port") + "/"
+ p.get("database")
+ "?loglevel=" + p.get("loglevel")
;
}

Expand Down
24 changes: 12 additions & 12 deletions pgjdbc/src/main/java/org/postgresql/Driver.java
View file
Open in desktop
Expand Up @@ -8,11 +8,11 @@
import static org.postgresql.util.internal.Nullness.castNonNull;

import org.postgresql.jdbc.PgConnection;
import org.postgresql.jdbcurlresolver.PgPassParser;
import org.postgresql.jdbcurlresolver.PgServiceConfParser;
import org.postgresql.util.DriverInfo;
import org.postgresql.util.GT;
import org.postgresql.util.HostSpec;
import org.postgresql.util.PGPropertyPasswordParser;
import org.postgresql.util.PGPropertyServiceParser;
import org.postgresql.util.PGPropertyUtil;
import org.postgresql.util.PSQLException;
import org.postgresql.util.PSQLState;
Expand Down Expand Up @@ -629,7 +629,7 @@ public boolean jdbcCompliant() {
// load pg_service.conf
if (serviceName != null) {
LOGGER.log(Level.FINE, "Processing option [?service={0}]", serviceName);
Properties result = PGPropertyServiceParser.getServiceProperties(serviceName);
Properties result = PgServiceConfParser.getServiceProperties(serviceName);
if (result == null) {
LOGGER.log(Level.WARNING, "Definition of service [{0}] not found", serviceName);
return null;
Expand All @@ -652,8 +652,8 @@ public boolean jdbcCompliant() {
// priority 5 - PGProperty defaults for PGHOST, PGPORT, PGDBNAME
result.putIfAbsent(PGProperty.PG_PORT.getName(), castNonNull(PGProperty.PG_PORT.getDefaultValue()));
result.putIfAbsent(PGProperty.PG_HOST.getName(), castNonNull(PGProperty.PG_HOST.getDefaultValue()));
if (PGProperty.USER.get(result) != null) {
result.putIfAbsent(PGProperty.PG_DBNAME.getName(), castNonNull(PGProperty.USER.get(result)));
if (PGProperty.USER.getOrDefault(result) != null) {
result.putIfAbsent(PGProperty.PG_DBNAME.getName(), castNonNull(PGProperty.USER.getOrDefault(result)));
}

// consistency check
Expand All @@ -662,9 +662,9 @@ public boolean jdbcCompliant() {
}

// try to load .pgpass if password is missing
if (PGProperty.PASSWORD.get(result) == null) {
String password = PGPropertyPasswordParser.getPassword(
PGProperty.PG_HOST.get(result), PGProperty.PG_PORT.get(result), PGProperty.PG_DBNAME.get(result), PGProperty.USER.get(result)
if (PGProperty.PASSWORD.getOrDefault(result) == null) {
String password = PgPassParser.getPassword(
PGProperty.PG_HOST.getOrDefault(result), PGProperty.PG_PORT.getOrDefault(result), PGProperty.PG_DBNAME.getOrDefault(result), PGProperty.USER.getOrDefault(result)
);
if (password != null && !password.isEmpty()) {
PGProperty.PASSWORD.set(result, password);
Expand All @@ -688,9 +688,9 @@ public boolean jdbcCompliant() {
* @return the address portion of the URL
*/
private static HostSpec[] hostSpecs(Properties props) {
String[] hosts = castNonNull(PGProperty.PG_HOST.get(props)).split(",");
String[] ports = castNonNull(PGProperty.PG_PORT.get(props)).split(",");
String localSocketAddress = PGProperty.LOCAL_SOCKET_ADDRESS.get(props);
String[] hosts = castNonNull(PGProperty.PG_HOST.getOrDefault(props)).split(",");
String[] ports = castNonNull(PGProperty.PG_PORT.getOrDefault(props)).split(",");
String localSocketAddress = PGProperty.LOCAL_SOCKET_ADDRESS.getOrDefault(props);
HostSpec[] hostSpecs = new HostSpec[hosts.length];
for (int i = 0; i < hostSpecs.length; ++i) {
hostSpecs[i] = new HostSpec(hosts[i], Integer.parseInt(ports[i]), localSocketAddress);
Expand All @@ -702,7 +702,7 @@ private static HostSpec[] hostSpecs(Properties props) {
* @return the timeout from the URL, in milliseconds
*/
private static long timeout(Properties props) {
String timeout = PGProperty.LOGIN_TIMEOUT.get(props);
String timeout = PGProperty.LOGIN_TIMEOUT.getOrDefault(props);
if (timeout != null) {
try {
return (long) (Float.parseFloat(timeout) * 1000);
Expand Down
28 changes: 20 additions & 8 deletions pgjdbc/src/main/java/org/postgresql/PGProperty.java
View file
Open in desktop
Expand Up @@ -317,17 +317,19 @@ public enum PGProperty {

/**
* This property is no longer used by the driver and will be ignored.
* Logging is configured via java.util.logging.
* @deprecated Logging is configured via java.util.logging.
*/
@Deprecated
LOGGER_FILE(
"loggerFile",
null,
"File name output of the Logger"),

/**
* This property is no longer used by the driver and will be ignored.
* Logging is configured via java.util.logging.
* @deprecated Logging is configured via java.util.logging.
*/
@Deprecated
LOGGER_LEVEL(
"loggerLevel",
null,
Expand Down Expand Up @@ -855,10 +857,20 @@ public String getDescription() {
* @param properties properties to take actual value from
* @return evaluated value for this connection parameter
*/
public @Nullable String get(Properties properties) {
public @Nullable String getOrDefault(Properties properties) {
return properties.getProperty(name, defaultValue);
}

/**
* Returns the value of the connection parameters according to the given {@code Properties}
*
* @param properties properties to take actual value from
* @return evaluated value for this connection parameter or null
*/
public @Nullable String get(Properties properties) {
return properties.getProperty(name);
Comment on lines -858 to +871
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a severe breakage of the public API.
Previously .get did take the default value into the consideration, while now get ignores the default value.

I believe code like PGProperty.PG_HOST.get(properties) is used in the wild, and it looks like it would change the behavior after this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will revert it. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we could just fix PGProperty.PG_HOST.get(properties) so that it worked as before ? Do you have any issues with the rest of the PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I should have figured it out myself that get() could be used by external libraries.

Can I make rollback commit to the same PR here and it can be merged again? (or new PR is required)

What could be the name of another method:

  • getWithoutDefault()
  • getNoDefault()
  • ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the only issue with this PR is that get should be reverted we can just fix that as per PR # 2644
After contemplating this a bit, I'm not sure of the value of using getOrDefault ?
@vlsi do you have any other issues with the PR ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we can probably keep getOrDefault so it explicitly conveys the behavior.

We can keep get() which would delegate to getOrDefault, and mark it as @Deprecated (without intention to remove the method), so the clients who use the methods could either migrate to the "new" getOrDefault or getWithoutDefault, etc.

I think getOrNull(...) would be better alternative than getWithoutDefault and getIgnoringDefault.
AFAIK, Properties (and Hashtable) does not support null values, and orNull in the method name gives an explicit warning that the method can return null.

I don't like getNoDefault.
Yet another alternative could be getIgnoringDefault, however, I think getOrNull is better for "get value or return null if it is missing in the properties".

Copy link
Contributor Author

@MarekUniq MarekUniq Oct 19, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davecramer

if the only issue with this PR is that get should be reverted we can just fix that as per PR #2644

yes, that's good

@vlsi

I think getOrNull is better ...

I like it.

}

/**
* Set the value for this connection parameter in the given {@code Properties}.
*
Expand All @@ -880,7 +892,7 @@ public void set(Properties properties, @Nullable String value) {
* @return evaluated value for this connection parameter converted to boolean
*/
public boolean getBoolean(Properties properties) {
return Boolean.parseBoolean(get(properties));
return Boolean.parseBoolean(getOrDefault(properties));
}

/**
Expand All @@ -893,7 +905,7 @@ public boolean getBoolean(Properties properties) {
*/
@SuppressWarnings("nullness:argument.type.incompatible")
public int getIntNoCheck(Properties properties) {
String value = get(properties);
String value = getOrDefault(properties);
//noinspection ConstantConditions
return Integer.parseInt(value);
}
Expand All @@ -907,7 +919,7 @@ public int getIntNoCheck(Properties properties) {
*/
@SuppressWarnings("nullness:argument.type.incompatible")
public int getInt(Properties properties) throws PSQLException {
String value = get(properties);
String value = getOrDefault(properties);
try {
//noinspection ConstantConditions
return Integer.parseInt(value);
Expand All @@ -925,7 +937,7 @@ public int getInt(Properties properties) throws PSQLException {
* @throws PSQLException if unable to parse property as integer
*/
public @Nullable Integer getInteger(Properties properties) throws PSQLException {
String value = get(properties);
String value = getOrDefault(properties);
if (value == null) {
return null;
}
Expand Down Expand Up @@ -975,7 +987,7 @@ public boolean isPresent(Properties properties) {
* @return a DriverPropertyInfo representing this connection parameter
*/
public DriverPropertyInfo toDriverPropertyInfo(Properties properties) {
DriverPropertyInfo propertyInfo = new DriverPropertyInfo(name, get(properties));
DriverPropertyInfo propertyInfo = new DriverPropertyInfo(name, getOrDefault(properties));
propertyInfo.required = required;
propertyInfo.description = description;
propertyInfo.choices = choices;
Expand Down
2 changes: 1 addition & 1 deletion pgjdbc/src/main/java/org/postgresql/core/ConnectionFactory.java
View file
Open in desktop
Expand Up @@ -42,7 +42,7 @@ public abstract class ConnectionFactory {
*/
public static QueryExecutor openConnection(HostSpec[] hostSpecs,
Properties info) throws SQLException {
String protoName = PGProperty.PROTOCOL_VERSION.get(info);
String protoName = PGProperty.PROTOCOL_VERSION.getOrDefault(info);

if (protoName == null || protoName.isEmpty() || "3".equals(protoName)) {
ConnectionFactory connectionFactory = new ConnectionFactoryImpl();
Expand Down
10 changes: 5 additions & 5 deletions pgjdbc/src/main/java/org/postgresql/core/QueryExecutorBase.java
View file
Open in desktop
Expand Up @@ -69,17 +69,17 @@ public abstract class QueryExecutorBase implements QueryExecutor {
@SuppressWarnings({"assignment.type.incompatible", "argument.type.incompatible"})
protected QueryExecutorBase(PGStream pgStream, int cancelSignalTimeout, Properties info) throws SQLException {
this.pgStream = pgStream;
this.user = PGProperty.USER.get(info);
this.database = PGProperty.PG_DBNAME.get(info);
this.user = PGProperty.USER.getOrDefault(info);
this.database = PGProperty.PG_DBNAME.getOrDefault(info);
this.cancelSignalTimeout = cancelSignalTimeout;
this.reWriteBatchedInserts = PGProperty.REWRITE_BATCHED_INSERTS.getBoolean(info);
this.columnSanitiserDisabled = PGProperty.DISABLE_COLUMN_SANITISER.getBoolean(info);
String callMode = PGProperty.ESCAPE_SYNTAX_CALL_MODE.get(info);
String callMode = PGProperty.ESCAPE_SYNTAX_CALL_MODE.getOrDefault(info);
this.escapeSyntaxCallMode = EscapeSyntaxCallMode.of(callMode);
this.quoteReturningIdentifiers = PGProperty.QUOTE_RETURNING_IDENTIFIERS.getBoolean(info);
String preferMode = PGProperty.PREFER_QUERY_MODE.get(info);
String preferMode = PGProperty.PREFER_QUERY_MODE.getOrDefault(info);
this.preferQueryMode = PreferQueryMode.of(preferMode);
this.autoSave = AutoSave.of(PGProperty.AUTOSAVE.get(info));
this.autoSave = AutoSave.of(PGProperty.AUTOSAVE.getOrDefault(info));
this.logServerErrorDetail = PGProperty.LOG_SERVER_ERROR_DETAIL.getBoolean(info);
// assignment.type.incompatible, argument.type.incompatible
this.cachedQueryCreateAction = new CachedQueryCreateAction(this);
Expand Down
8 changes: 4 additions & 4 deletions pgjdbc/src/main/java/org/postgresql/core/SocketFactoryFactory.java
View file
Open in desktop
Expand Up @@ -31,13 +31,13 @@ public class SocketFactoryFactory {
*/
public static SocketFactory getSocketFactory(Properties info) throws PSQLException {
// Socket factory
String socketFactoryClassName = PGProperty.SOCKET_FACTORY.get(info);
String socketFactoryClassName = PGProperty.SOCKET_FACTORY.getOrDefault(info);
if (socketFactoryClassName == null) {
return SocketFactory.getDefault();
}
try {
return ObjectFactory.instantiate(SocketFactory.class, socketFactoryClassName, info, true,
PGProperty.SOCKET_FACTORY_ARG.get(info));
PGProperty.SOCKET_FACTORY_ARG.getOrDefault(info));
} catch (Exception e) {
throw new PSQLException(
GT.tr("The SocketFactory class provided {0} could not be instantiated.",
Expand All @@ -54,15 +54,15 @@ public static SocketFactory getSocketFactory(Properties info) throws PSQLExcepti
* @throws PSQLException if something goes wrong
*/
public static SSLSocketFactory getSslSocketFactory(Properties info) throws PSQLException {
String classname = PGProperty.SSL_FACTORY.get(info);
String classname = PGProperty.SSL_FACTORY.getOrDefault(info);
if (classname == null
|| "org.postgresql.ssl.jdbc4.LibPQFactory".equals(classname)
|| "org.postgresql.ssl.LibPQFactory".equals(classname)) {
return new LibPQFactory(info);
}
try {
return ObjectFactory.instantiate(SSLSocketFactory.class, classname, info, true,
PGProperty.SSL_FACTORY_ARG.get(info));
PGProperty.SSL_FACTORY_ARG.getOrDefault(info));
} catch (Exception e) {
throw new PSQLException(
GT.tr("The SSLSocketFactory class provided {0} could not be instantiated.", classname),
Expand Down
6 changes: 3 additions & 3 deletions pgjdbc/src/main/java/org/postgresql/core/v3/AuthenticationPluginManager.java
View file
Open in desktop
Expand Up @@ -55,11 +55,11 @@ public static <T> T withPassword(AuthenticationRequestType type, Properties info
PasswordAction<char @Nullable [], T> action) throws PSQLException, IOException {
char[] password = null;

String authPluginClassName = PGProperty.AUTHENTICATION_PLUGIN_CLASS_NAME.get(info);
String authPluginClassName = PGProperty.AUTHENTICATION_PLUGIN_CLASS_NAME.getOrDefault(info);

if (authPluginClassName == null || authPluginClassName.equals("")) {
// Default auth plugin simply pulls password directly from connection properties
String passwordText = PGProperty.PASSWORD.get(info);
String passwordText = PGProperty.PASSWORD.getOrDefault(info);
if (passwordText != null) {
password = passwordText.toCharArray();
}
Expand Down Expand Up @@ -108,7 +108,7 @@ public static <T> T withEncodedPassword(AuthenticationRequestType type, Properti
if (password == null) {
throw new PSQLException(
GT.tr("The server requested password-based authentication, but no password was provided by plugin {0}",
PGProperty.AUTHENTICATION_PLUGIN_CLASS_NAME.get(info)),
PGProperty.AUTHENTICATION_PLUGIN_CLASS_NAME.getOrDefault(info)),
PSQLState.CONNECTION_REJECTED);
}
ByteBuffer buf = StandardCharsets.UTF_8.encode(CharBuffer.wrap(password));
Expand Down