-
-
Notifications
You must be signed in to change notification settings - Fork 468
perf: Replace java.net.URI with custom string parsing in Dsn #5448
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,54 +53,100 @@ URI getSentryUri() { | |
| return sentryUri; | ||
| } | ||
|
|
||
| // Avoids java.net.URI for DSN parsing, which is slow on Android. | ||
| Dsn(@Nullable String dsn) throws IllegalArgumentException { | ||
| try { | ||
| final String dsnString = Objects.requireNonNull(dsn, "The DSN is required.").trim(); | ||
| if (dsnString.isEmpty()) { | ||
| throw new IllegalArgumentException("The DSN is empty."); | ||
| } | ||
| final URI uri = new URI(dsnString).normalize(); | ||
| final String scheme = uri.getScheme(); | ||
|
|
||
| // Extract scheme | ||
| final int schemeEnd = dsnString.indexOf("://"); | ||
| if (schemeEnd < 0) { | ||
| throw new IllegalArgumentException("Invalid DSN: missing scheme."); | ||
| } | ||
| final String scheme = dsnString.substring(0, schemeEnd); | ||
| if (!("http".equalsIgnoreCase(scheme) || "https".equalsIgnoreCase(scheme))) { | ||
| throw new IllegalArgumentException("Invalid DSN scheme: " + scheme); | ||
| } | ||
|
|
||
| String userInfo = uri.getUserInfo(); | ||
| if (userInfo == null || userInfo.isEmpty()) { | ||
| // Extract userinfo (public key and optional secret key) | ||
| final int authStart = schemeEnd + 3; | ||
| final int atIndex = dsnString.indexOf('@', authStart); | ||
| if (atIndex < 0) { | ||
| throw new IllegalArgumentException("Invalid DSN: No public key provided."); | ||
| } | ||
| final String userInfo = dsnString.substring(authStart, atIndex); | ||
| if (userInfo.isEmpty()) { | ||
| throw new IllegalArgumentException("Invalid DSN: No public key provided."); | ||
| } | ||
| String[] keys = userInfo.split(":", -1); | ||
| publicKey = keys[0]; | ||
| if (publicKey == null || publicKey.isEmpty()) { | ||
| final int colonIndex = userInfo.indexOf(':'); | ||
| if (colonIndex < 0) { | ||
| publicKey = userInfo; | ||
| secretKey = null; | ||
| } else { | ||
| publicKey = userInfo.substring(0, colonIndex); | ||
| secretKey = userInfo.substring(colonIndex + 1); | ||
| } | ||
| if (publicKey.isEmpty()) { | ||
| throw new IllegalArgumentException("Invalid DSN: No public key provided."); | ||
| } | ||
| secretKey = keys.length > 1 ? keys[1] : null; | ||
| String uriPath = uri.getPath(); | ||
| if (uriPath.endsWith("/")) { | ||
| uriPath = uriPath.substring(0, uriPath.length() - 1); | ||
|
|
||
| // Extract host, optional port, and path+projectId | ||
| final int hostStart = atIndex + 1; | ||
|
|
||
| // Strip query string if present | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strip any fragment too? final int fragmentIndex = dsnString.indexOf('#', hostStart);URI would've done it. Not sure how much we care about saving folks from obvious errors they're unlikely to make, but including it will keep parity.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...ah, cursorbot beat me to it. |
||
| final int queryIndex = dsnString.indexOf('?', hostStart); | ||
| final String hostAndPath = | ||
| queryIndex < 0 | ||
| ? dsnString.substring(hostStart) | ||
| : dsnString.substring(hostStart, queryIndex); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. URI fragment not stripped, corrupts projectId silentlyLow Severity The new parser strips query strings ( Additional Locations (1)Reviewed by Cursor Bugbot for commit cacc249. Configure here. |
||
|
|
||
| final int firstSlash = hostAndPath.indexOf('/'); | ||
| if (firstSlash < 0) { | ||
| throw new IllegalArgumentException("Invalid DSN: A Project Id is required."); | ||
| } | ||
|
|
||
| final String hostPort = hostAndPath.substring(0, firstSlash); | ||
| final int portColon = hostPort.indexOf(':'); | ||
| final String host; | ||
| final int port; | ||
| if (portColon < 0) { | ||
| host = hostPort; | ||
| port = -1; | ||
| } else { | ||
| host = hostPort.substring(0, portColon); | ||
| port = Integer.parseInt(hostPort.substring(portColon + 1)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. l: We could give a more informative error message if the port is malformed / not an int: final String portStr = hostPort.substring(portColon + 1);
try {
port = Integer.parseInt(portStr);
} catch (NumberFormatException e) {
throw new IllegalArgumentException("Invalid DSN: invalid port: " + portStr);
}(Presumably rare in practice, and cleaner to do if we extract logic into static methods. Also up to you.) |
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IPv6 host parsing broken by first-colon port detectionLow Severity
Reviewed by Cursor Bugbot for commit cacc249. Configure here. |
||
|
|
||
| // Normalize the path (collapse double slashes, like URI.normalize()) | ||
| String rawPath = hostAndPath.substring(firstSlash); | ||
| while (rawPath.contains("//")) { | ||
| rawPath = rawPath.replace("//", "/"); | ||
| } | ||
|
|
||
| if (rawPath.endsWith("/")) { | ||
| rawPath = rawPath.substring(0, rawPath.length() - 1); | ||
| } | ||
| int projectIdStart = uriPath.lastIndexOf("/") + 1; | ||
| String path = uriPath.substring(0, projectIdStart); | ||
| if (!path.endsWith("/")) { | ||
| path += "/"; | ||
| final int projectIdStart = rawPath.lastIndexOf('/') + 1; | ||
| String pathSegment = rawPath.substring(0, projectIdStart); | ||
| if (!pathSegment.endsWith("/")) { | ||
| pathSegment += "/"; | ||
| } | ||
| this.path = path; | ||
| projectId = uriPath.substring(projectIdStart); | ||
| this.path = pathSegment; | ||
| projectId = rawPath.substring(projectIdStart); | ||
| if (projectId.isEmpty()) { | ||
| throw new IllegalArgumentException("Invalid DSN: A Project Id is required."); | ||
| } | ||
| sentryUri = | ||
| new URI( | ||
| scheme, null, uri.getHost(), uri.getPort(), path + "api/" + projectId, null, null); | ||
|
|
||
| sentryUri = new URI(scheme, null, host, port, pathSegment + "api/" + projectId, null, null); | ||
|
|
||
| // Extract org ID from host (e.g., "o123.ingest.sentry.io" -> "123") | ||
| String extractedOrgId = null; | ||
| final String host = uri.getHost(); | ||
| if (host != null) { | ||
| final Matcher matcher = ORG_ID_PATTERN.matcher(host); | ||
| if (matcher.find()) { | ||
| extractedOrgId = matcher.group(1); | ||
| } | ||
| final Matcher matcher = ORG_ID_PATTERN.matcher(host); | ||
| if (matcher.find()) { | ||
| extractedOrgId = matcher.group(1); | ||
| } | ||
| orgId = extractedOrgId; | ||
| } catch (Throwable e) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,4 +145,71 @@ class DsnTest { | |
| val dsn = Dsn("http://key@localhost:9000/456") | ||
| assertNull(dsn.orgId) | ||
| } | ||
|
|
||
| @Test | ||
| fun `when dsn is null, throws exception`() { | ||
| assertFailsWith<IllegalArgumentException> { Dsn(null) } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. l: My vote would be for asserting particular error messages in these tests, as they're part of our implicit API. (Looks like we sometimes do it above.) |
||
| } | ||
|
|
||
| @Test | ||
| fun `when dsn has no scheme separator, throws exception`() { | ||
| assertFailsWith<IllegalArgumentException> { Dsn("httpspublicKey@host/id") } | ||
| } | ||
|
|
||
| @Test | ||
| fun `when dsn has no slash after host, throws exception`() { | ||
| assertFailsWith<IllegalArgumentException> { Dsn("https://key@host") } | ||
| } | ||
|
|
||
| @Test | ||
| fun `dsn parsed with multiple path segments`() { | ||
| val dsn = Dsn("https://key@host/path/to/sentry/id") | ||
|
|
||
| assertEquals("https://host/path/to/sentry/api/id", dsn.sentryUri.toURL().toString()) | ||
| assertEquals("key", dsn.publicKey) | ||
| assertEquals("/path/to/sentry/", dsn.path) | ||
| assertEquals("id", dsn.projectId) | ||
| } | ||
|
|
||
| @Test | ||
| fun `dsn parsed with port and path`() { | ||
| val dsn = Dsn("http://key:secret@host:8080/path/id") | ||
|
|
||
| assertEquals("http://host:8080/path/api/id", dsn.sentryUri.toURL().toString()) | ||
| assertEquals("key", dsn.publicKey) | ||
| assertEquals("secret", dsn.secretKey) | ||
| assertEquals("/path/", dsn.path) | ||
| assertEquals("id", dsn.projectId) | ||
| } | ||
|
|
||
| @Test | ||
| fun `dsn with multiple double slashes in path is normalized`() { | ||
| val dsn = Dsn("http://key@host//path//id") | ||
| assertEquals("http://host/path/api/id", dsn.sentryUri.toURL().toString()) | ||
| } | ||
|
|
||
| @Test | ||
| fun `dsn with query string and port`() { | ||
| val dsn = Dsn("https://key@host:443/id?foo=bar&baz=1") | ||
|
|
||
| assertEquals("https://host:443/api/id", dsn.sentryUri.toURL().toString()) | ||
| assertEquals("id", dsn.projectId) | ||
| } | ||
|
|
||
| @Test | ||
| fun `dsn with empty secret key after colon`() { | ||
| val dsn = Dsn("https://publicKey:@host/id") | ||
|
|
||
| assertEquals("publicKey", dsn.publicKey) | ||
| assertEquals("", dsn.secretKey) | ||
| } | ||
|
|
||
| @Test | ||
| fun `dsn with numeric project id`() { | ||
| val dsn = Dsn("https://key@o123.ingest.sentry.io/1234567") | ||
|
|
||
| assertEquals("1234567", dsn.projectId) | ||
| assertEquals("123", dsn.orgId) | ||
| assertEquals("https://o123.ingest.sentry.io/api/1234567", dsn.sentryUri.toURL().toString()) | ||
| } | ||
| } | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: Thoughts about extracting logic into static methods for legibility? Something like:
Claude thinks HotSpot, dex2oat, etc. would inline the invocations, meaning no practical overhead.
Up to you, of course.