You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "CzyerChen (via GitHub)" <gi...@apache.org> on 2023/11/16 11:13:41 UTC
[PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
CzyerChen opened a new pull request, #649:
URL: https://github.com/apache/skywalking-java/pull/649
### Fix #11552
- [x] Add a unit test to verify that the fix works.
- [x] Explain briefly why the bug exists and how to fix it.
### Fix multi hosts JDBC url parsing exception
- [x] Add a unit test to verify that the fix works.
- [x] Explain briefly why the bug exists and how to fix it.
----
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #11552.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #649:
URL: https://github.com/apache/skywalking-java/pull/649#issuecomment-1815559089
> > I am not sure why, but the UTs on the Windows keep failing.
>
> all the new UTs added in URLParserTest?
Don't worry about this. They are passed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #649:
URL: https://github.com/apache/skywalking-java/pull/649#discussion_r1397274355
##########
apm-sniffer/apm-agent-core/src/test/java/org/apache/skywalking/apm/agent/core/context/IgnoredTracerContextTest.java:
##########
@@ -73,9 +74,8 @@ public void ignoredTraceContextWithSampling() {
ContextManager.createLocalSpan("/test4");
ContextManager.stopSpan();
- assertThat(storage.getIgnoredTracerContexts().size(), is(3));
- assertThat(storage.getTraceSegments().size(), is(1));
-
+ Assert.assertTrue(storage.getIgnoredTracerContexts().size() < 4);
+ Assert.assertTrue(storage.getTraceSegments().size() > 0);
Review Comment:
The original tests are not stable. Try to fix through this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #649:
URL: https://github.com/apache/skywalking-java/pull/649#issuecomment-1814386089
I am not sure why, but the UTs on the Windows keep failing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #649:
URL: https://github.com/apache/skywalking-java/pull/649#discussion_r1395734278
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/connectionurl/parser/PostgreSQLURLParser.java:
##########
@@ -37,15 +37,29 @@ public PostgreSQLURLParser(String url) {
protected URLLocation fetchDatabaseHostsIndexRange() {
int hostLabelStartIndex = url.indexOf("//");
int hostLabelEndIndex = url.indexOf("/", hostLabelStartIndex + 2);
+ if (hostLabelEndIndex == -1) {
+ hostLabelEndIndex = url.length();
+ }
return new URLLocation(hostLabelStartIndex + 2, hostLabelEndIndex);
}
@Override
protected URLLocation fetchDatabaseNameIndexRange() {
- int databaseStartTag = url.lastIndexOf("/");
- int databaseEndTag = url.indexOf("?", databaseStartTag);
+ int databaseLabelStartIndex = url.indexOf("//");
+ int databaseStartTag = url.indexOf("/", databaseLabelStartIndex + 2);
+ int databaseEndTag = url.indexOf("?", databaseLabelStartIndex + 2);
+ if (databaseEndTag < databaseStartTag && databaseEndTag != -1) {
+ //database parse fail
+ return new URLLocation(0, 0);
+ }
+ if (databaseStartTag == -1) {
+ //database empty
+ return new URLLocation(0, 0);
+ }
Review Comment:
If we fail into this, what is the database name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #649:
URL: https://github.com/apache/skywalking-java/pull/649#discussion_r1396538667
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/connectionurl/parser/PostgreSQLURLParser.java:
##########
@@ -37,15 +37,29 @@ public PostgreSQLURLParser(String url) {
protected URLLocation fetchDatabaseHostsIndexRange() {
int hostLabelStartIndex = url.indexOf("//");
int hostLabelEndIndex = url.indexOf("/", hostLabelStartIndex + 2);
+ if (hostLabelEndIndex == -1) {
+ hostLabelEndIndex = url.length();
+ }
return new URLLocation(hostLabelStartIndex + 2, hostLabelEndIndex);
}
@Override
protected URLLocation fetchDatabaseNameIndexRange() {
- int databaseStartTag = url.lastIndexOf("/");
- int databaseEndTag = url.indexOf("?", databaseStartTag);
+ int databaseLabelStartIndex = url.indexOf("//");
+ int databaseStartTag = url.indexOf("/", databaseLabelStartIndex + 2);
+ int databaseEndTag = url.indexOf("?", databaseLabelStartIndex + 2);
+ if (databaseEndTag < databaseStartTag && databaseEndTag != -1) {
+ //database parse fail
+ return new URLLocation(0, 0);
+ }
+ if (databaseStartTag == -1) {
+ //database empty
+ return new URLLocation(0, 0);
+ }
Review Comment:
Are there `jdbc:postgresql://localhost` and `jdbc:postgresql://` legal connection URLs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #649:
URL: https://github.com/apache/skywalking-java/pull/649#discussion_r1396551680
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/connectionurl/parser/PostgreSQLURLParser.java:
##########
@@ -37,15 +37,29 @@ public PostgreSQLURLParser(String url) {
protected URLLocation fetchDatabaseHostsIndexRange() {
int hostLabelStartIndex = url.indexOf("//");
int hostLabelEndIndex = url.indexOf("/", hostLabelStartIndex + 2);
+ if (hostLabelEndIndex == -1) {
+ hostLabelEndIndex = url.length();
+ }
return new URLLocation(hostLabelStartIndex + 2, hostLabelEndIndex);
}
@Override
protected URLLocation fetchDatabaseNameIndexRange() {
- int databaseStartTag = url.lastIndexOf("/");
- int databaseEndTag = url.indexOf("?", databaseStartTag);
+ int databaseLabelStartIndex = url.indexOf("//");
+ int databaseStartTag = url.indexOf("/", databaseLabelStartIndex + 2);
+ int databaseEndTag = url.indexOf("?", databaseLabelStartIndex + 2);
+ if (databaseEndTag < databaseStartTag && databaseEndTag != -1) {
+ //database parse fail
+ return new URLLocation(0, 0);
+ }
+ if (databaseStartTag == -1) {
+ //database empty
+ return new URLLocation(0, 0);
+ }
Review Comment:
Several things I noticed from the doc
- `postgresql://`, we should check what is the default path, and the peer can't be null for analysis
- `postgresql:///mydb?host=localhost&port=5433`, host and port could be given by parameters. We should support and connect them to make the peer.
- Please recheck on IPv6 case `postgresql://[2001:db8::1234]/database`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
Posted by "CzyerChen (via GitHub)" <gi...@apache.org>.
CzyerChen commented on code in PR #649:
URL: https://github.com/apache/skywalking-java/pull/649#discussion_r1396537060
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/connectionurl/parser/PostgreSQLURLParser.java:
##########
@@ -37,15 +37,29 @@ public PostgreSQLURLParser(String url) {
protected URLLocation fetchDatabaseHostsIndexRange() {
int hostLabelStartIndex = url.indexOf("//");
int hostLabelEndIndex = url.indexOf("/", hostLabelStartIndex + 2);
+ if (hostLabelEndIndex == -1) {
+ hostLabelEndIndex = url.length();
+ }
return new URLLocation(hostLabelStartIndex + 2, hostLabelEndIndex);
}
@Override
protected URLLocation fetchDatabaseNameIndexRange() {
- int databaseStartTag = url.lastIndexOf("/");
- int databaseEndTag = url.indexOf("?", databaseStartTag);
+ int databaseLabelStartIndex = url.indexOf("//");
+ int databaseStartTag = url.indexOf("/", databaseLabelStartIndex + 2);
+ int databaseEndTag = url.indexOf("?", databaseLabelStartIndex + 2);
+ if (databaseEndTag < databaseStartTag && databaseEndTag != -1) {
+ //database parse fail
+ return new URLLocation(0, 0);
+ }
+ if (databaseStartTag == -1) {
+ //database empty
+ return new URLLocation(0, 0);
+ }
Review Comment:
It will call method `fetchDatabaseNameFromURL` in `AbstractURLParser` next, the database name will be ""
```java
protected String fetchDatabaseNameFromURL() {
URLLocation hostsLocation = fetchDatabaseNameIndexRange(); // --> return new URLLocation(0, 0);
return url.substring(hostsLocation.startIndex(), hostsLocation.endIndex()); // --> substring will return ""
}
```
Test cases matched:
```java
@Test
public void testParsePostgresqlJDBCURLWithHostNoPortAndDb() {
ConnectionInfo connectionInfo = new URLParser().parser("jdbc:postgresql://localhost");
assertThat(connectionInfo.getDBType(), is("PostgreSQL"));
assertThat(connectionInfo.getDatabaseName(), is(""));
assertThat(connectionInfo.getDatabasePeer(), is("localhost:5432"));
}
@Test
public void testParsePostgresqlJDBCURLEmpty() {
ConnectionInfo connectionInfo = new URLParser().parser("jdbc:postgresql://");
assertThat(connectionInfo.getDBType(), is("PostgreSQL"));
assertThat(connectionInfo.getDatabaseName(), is(""));
assertThat(connectionInfo.getDatabasePeer(), is(":5432"));
}
```
The MysqlUrlParser may be better, but it rewrite `fetchDatabaseNameFromURL` itself that not controlled by `AbstractURLParser`.
```java
protected String fetchDatabaseNameFromURL(int startSize) {
URLLocation hostsLocation = fetchDatabaseNameIndexRange(startSize);
if (hostsLocation == null) {
return "";
}
return url.substring(hostsLocation.startIndex(), hostsLocation.endIndex());
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
Posted by "CzyerChen (via GitHub)" <gi...@apache.org>.
CzyerChen commented on PR #649:
URL: https://github.com/apache/skywalking-java/pull/649#issuecomment-1815553029
> I am not sure why, but the UTs on the Windows keep failing.
all the new UTs added in URLParserTest?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #649:
URL: https://github.com/apache/skywalking-java/pull/649
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix PostgreSQL Jdbc URL parsing exception [skywalking-java]
Posted by "CzyerChen (via GitHub)" <gi...@apache.org>.
CzyerChen commented on code in PR #649:
URL: https://github.com/apache/skywalking-java/pull/649#discussion_r1396542859
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/connectionurl/parser/PostgreSQLURLParser.java:
##########
@@ -37,15 +37,29 @@ public PostgreSQLURLParser(String url) {
protected URLLocation fetchDatabaseHostsIndexRange() {
int hostLabelStartIndex = url.indexOf("//");
int hostLabelEndIndex = url.indexOf("/", hostLabelStartIndex + 2);
+ if (hostLabelEndIndex == -1) {
+ hostLabelEndIndex = url.length();
+ }
return new URLLocation(hostLabelStartIndex + 2, hostLabelEndIndex);
}
@Override
protected URLLocation fetchDatabaseNameIndexRange() {
- int databaseStartTag = url.lastIndexOf("/");
- int databaseEndTag = url.indexOf("?", databaseStartTag);
+ int databaseLabelStartIndex = url.indexOf("//");
+ int databaseStartTag = url.indexOf("/", databaseLabelStartIndex + 2);
+ int databaseEndTag = url.indexOf("?", databaseLabelStartIndex + 2);
+ if (databaseEndTag < databaseStartTag && databaseEndTag != -1) {
+ //database parse fail
+ return new URLLocation(0, 0);
+ }
+ if (databaseStartTag == -1) {
+ //database empty
+ return new URLLocation(0, 0);
+ }
Review Comment:
I learn from PostgreSQL documentation here `https://www.postgresql.org/docs/16/libpq-connect.html`
The following examples illustrate valid URI syntax:
```
postgresql://
postgresql://localhost
postgresql://localhost:5433
postgresql://localhost/mydb
postgresql://user@localhost
postgresql://user:secret@localhost
postgresql://other@localhost/otherdb?connect_timeout=10&application_name=myapp
postgresql://host1:123,host2:456/somedb?target_session_attrs=any&application_name=myapp
```
and I tested 6 of them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org