Skip to content

Commit d693427

Browse files
L-ApplinL-Applin
committed
revised isAccountIdUri logic, added unit tests
1 parent 0b0126e commit d693427

File tree

2 files changed

+174
-11
lines changed

2 files changed

+174
-11
lines changed

‎utils/src/main/java/software/amazon/awssdk/utils/uri/SdkURI.java

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@
3030
*/
3131
@SdkProtectedApi
3232
public final class SdkURI {
33+
private final String HTTPS_PREFIX = "https://";
34+
private final String HTTP_PREFIX = "http://";
3335

3436
private static final Logger log = Logger.loggerFor(SdkURI.class);
3537

@@ -105,16 +107,28 @@ public URI newURI(String scheme,
105107
return uri;
106108
}
107109

108-
// We only want to cache account-id based URI
110+
/*
111+
* Best-effort check for uri string being account-id based.
112+
*
113+
* The troublesome uris are of the form 'https://123456789012.ddb.us-east-1.amazonaws.com' The heuristic chosen to detect such
114+
* candidate URI is to check the first char after the scheme, and then the char 10 places further down the string. If both
115+
* are digits, there is a potential for that string to represent a number that would exceed the value of Integer.MAX_VALUE,
116+
* which would cause the performance degradation observed with such URIs.
117+
*/
109118
private boolean isAccountIdUri(String s) {
110-
if (s.startsWith("https://")) {
111-
return Character.isDigit(s.charAt(8));
119+
int firstCharAfterScheme = 0;
120+
int maxIntSizeBase10 = 10;
121+
if (s.startsWith(HTTPS_PREFIX)) {
122+
firstCharAfterScheme = HTTPS_PREFIX.length();
123+
} else if (s.startsWith(HTTP_PREFIX)) {
124+
firstCharAfterScheme = HTTP_PREFIX.length();
112125
}
113126

114-
if (s.startsWith("http://")) {
115-
return Character.isDigit(s.charAt(7));
127+
if (s.length() > firstCharAfterScheme + maxIntSizeBase10) {
128+
return Character.isDigit(s.charAt(firstCharAfterScheme))
129+
&& Character.isDigit(s.charAt(firstCharAfterScheme + maxIntSizeBase10));
116130
}
117-
return Character.isDigit(s.charAt(0));
131+
return false;
118132
}
119133

120134
private void logCacheUsage(boolean containsKey, URI uri) {

‎utils/src/test/java/software/amazon/awssdk/utils/SdkURITest.java

Lines changed: 154 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,14 @@ void resetCache() throws IllegalAccessException {
4343
.build());
4444
}
4545

46-
@Test
47-
void multipleCreate_withSameStringConstructor_shouldCacheOnlyOnce() {
48-
String strURI = "https://123456789012.ddb.us-east-1.amazonaws.com";
46+
@ParameterizedTest
47+
@ValueSource(strings = {"https://123456789012.ddb.us-east-1.amazonaws.com",
48+
"http://123456789012.ddb.us-east-1.amazonaws.com"})
49+
void multipleCreate_simpleURI_SameStringConstructor_ShouldCacheOnlyOnce(String strURI) {
4950
URI uri = SdkURI.getInstance().create(strURI);
51+
String scheme = strURI.startsWith("https") ? "https" : "http";
5052
assertThat(uri).hasHost("123456789012.ddb.us-east-1.amazonaws.com")
53+
.hasScheme(scheme)
5154
.hasNoParameters()
5255
.hasNoPort()
5356
.hasNoQuery();
@@ -57,6 +60,26 @@ void multipleCreate_withSameStringConstructor_shouldCacheOnlyOnce() {
5760
assertThat(uri).isSameAs(uri2);
5861
}
5962

63+
@ParameterizedTest
64+
@ValueSource(strings = {"http", "https"})
65+
void multipleCreate_FullUri_SameConstructor_ShouldCacheOnlyOne(String scheme) {
66+
String strURI = scheme + "://123456789012.ddb.us-east-1.amazonaws.com:322/some/path?foo=bar#test";
67+
URI uri = SdkURI.getInstance().create(strURI);
68+
assertThat(uri).hasHost("123456789012.ddb.us-east-1.amazonaws.com")
69+
.hasScheme(scheme)
70+
.hasNoUserInfo()
71+
.hasPort(322)
72+
.hasPath("/some/path")
73+
.hasQuery("foo=bar")
74+
.hasFragment("test");
75+
76+
assertThat(getCache().size()).isEqualTo(1);
77+
URI uri2 = SdkURI.getInstance().create(strURI);
78+
assertThat(getCache().size()).isEqualTo(1);
79+
assertThat(uri).isSameAs(uri2);
80+
81+
}
82+
6083
@Test
6184
void multipleCreate_withDifferentStringConstructor_shouldCacheOnlyOnce() {
6285
String[] strURIs = {
@@ -68,13 +91,112 @@ void multipleCreate_withDifferentStringConstructor_shouldCacheOnlyOnce() {
6891
"https://123456789017.ddb.us-east-1.amazonaws.com",
6992
"https://123456789018.ddb.us-east-1.amazonaws.com",
7093
"https://123456789019.ddb.us-east-1.amazonaws.com",
71-
};
94+
};
7295
for (String uri : strURIs) {
7396
URI u = SdkURI.getInstance().create(uri);
7497
}
7598
assertThat(getCache().size()).isEqualTo(8);
7699
}
77100

101+
@ParameterizedTest
102+
@ValueSource(strings = {"http", "https"})
103+
void multipleNewUriWithNulls_SameAuthorityConstructor_ShouldCacheOnlyOnce(String scheme) throws URISyntaxException {
104+
String strURI = "123456789012.ddb.us-east-1.amazonaws.com";
105+
URI uri = SdkURI.getInstance().newURI(scheme, strURI, null, null, null);
106+
assertThat(uri).hasHost("123456789012.ddb.us-east-1.amazonaws.com")
107+
.hasScheme(scheme)
108+
.hasNoParameters()
109+
.hasNoPort()
110+
.hasNoQuery();
111+
assertThat(getCache().size()).isEqualTo(1);
112+
URI uri2 = SdkURI.getInstance().newURI(scheme, strURI, null, null, null);
113+
assertThat(getCache().size()).isEqualTo(1);
114+
assertThat(uri).isSameAs(uri2);
115+
}
116+
117+
@ParameterizedTest
118+
@ValueSource(strings = {"http", "https"})
119+
void multipleNewUri_SameAuthorityConstructor_ShouldCacheOnlyOnce(String scheme) throws URISyntaxException {
120+
String strURI = "123456789012.ddb.us-east-1.amazonaws.com";
121+
URI uri = SdkURI.getInstance().newURI(scheme, strURI, "/somePath/to/resource", "foo=bar", "test");
122+
assertThat(uri).hasHost(strURI)
123+
.hasPath("/somePath/to/resource")
124+
.hasQuery("foo=bar")
125+
.hasFragment("test")
126+
.hasScheme(scheme);
127+
assertThat(getCache().size()).isEqualTo(1);
128+
URI uri2 = SdkURI.getInstance().newURI(scheme, strURI, "/somePath/to/resource", "foo=bar", "test");
129+
assertThat(getCache().size()).isEqualTo(1);
130+
assertThat(uri).isSameAs(uri2);
131+
}
132+
133+
@ParameterizedTest
134+
@ValueSource(strings = {"http", "https"})
135+
void multipleNewUri_DifferentAuthorityConstructor_ShouldCacheAll(String scheme) throws URISyntaxException {
136+
String strURI = "123456789012.ddb.us-east-1.amazonaws.com";
137+
URI uri = SdkURI.getInstance().newURI(scheme, strURI, "/somePath/to/resource", "foo=bar", "test");
138+
assertThat(uri).hasHost(strURI)
139+
.hasPath("/somePath/to/resource")
140+
.hasQuery("foo=bar")
141+
.hasFragment("test")
142+
.hasScheme(scheme);
143+
assertThat(getCache().size()).isEqualTo(1);
144+
URI uri2 = SdkURI.getInstance().newURI(scheme, strURI, "/some/otherPath/to/resource", null, "test2");
145+
assertThat(getCache().size()).isEqualTo(2);
146+
assertThat(uri).isNotSameAs(uri2);
147+
}
148+
149+
@ParameterizedTest
150+
@ValueSource(strings = {"http", "https"})
151+
void multipleNewUriWithNulls_SameHostConstructor_ShouldCacheOnlyOnce(String scheme) throws URISyntaxException {
152+
String strURI = "123456789012.ddb.us-east-1.amazonaws.com";
153+
URI uri = SdkURI.getInstance().newURI(scheme, null, strURI, 322, null, null, null);
154+
assertThat(uri).hasHost("123456789012.ddb.us-east-1.amazonaws.com")
155+
.hasNoParameters()
156+
.hasPort(322)
157+
.hasNoQuery();
158+
assertThat(getCache().size()).isEqualTo(1);
159+
URI uri2 = SdkURI.getInstance().newURI(scheme, null, strURI, 322, null, null, null);
160+
assertThat(getCache().size()).isEqualTo(1);
161+
assertThat(uri).isSameAs(uri2);
162+
}
163+
164+
@ParameterizedTest
165+
@ValueSource(strings = {"http", "https"})
166+
void multipleNewUri_SameHostConstructor_ShouldCacheOnlyOnce(String scheme) throws URISyntaxException {
167+
String strURI = "123456789012.ddb.us-east-1.amazonaws.com";
168+
URI uri = SdkURI.getInstance().newURI(scheme, "user1", strURI, 322, "/some/path", "foo=bar", "test");
169+
assertThat(uri).hasHost("123456789012.ddb.us-east-1.amazonaws.com")
170+
.hasScheme(scheme)
171+
.hasUserInfo("user1")
172+
.hasPort(322)
173+
.hasPath("/some/path")
174+
.hasQuery("foo=bar")
175+
.hasFragment("test");
176+
assertThat(getCache().size()).isEqualTo(1);
177+
URI uri2 = SdkURI.getInstance().newURI(scheme, "user1", strURI, 322, "/some/path", "foo=bar", "test");
178+
assertThat(getCache().size()).isEqualTo(1);
179+
assertThat(uri).isSameAs(uri2);
180+
}
181+
182+
@ParameterizedTest
183+
@ValueSource(strings = {"http", "https"})
184+
void multipleNewUri_DifferentHostConstructor_ShouldCacheOnlyOnce(String scheme) throws URISyntaxException {
185+
String strURI = "123456789012.ddb.us-east-1.amazonaws.com";
186+
URI uri = SdkURI.getInstance().newURI(scheme, "user1", strURI, 322, "/some/path", "foo=bar", "test");
187+
assertThat(uri).hasHost("123456789012.ddb.us-east-1.amazonaws.com")
188+
.hasScheme(scheme)
189+
.hasUserInfo("user1")
190+
.hasPort(322)
191+
.hasPath("/some/path")
192+
.hasQuery("foo=bar")
193+
.hasFragment("test");
194+
assertThat(getCache().size()).isEqualTo(1);
195+
URI uri2 = SdkURI.getInstance().newURI(scheme, "user1", strURI, 322, "/some/other/path", "foo=bar", "test2");
196+
assertThat(getCache().size()).isEqualTo(2);
197+
assertThat(uri).isNotSameAs(uri2);
198+
}
199+
78200
@Test
79201
void notCached_shouldCreateNewInstance() {
80202
String strURI = "https://ddb.us-east-1.amazonaws.com";
@@ -113,13 +235,40 @@ void malformedURI_shouldThrowsSameExceptionAsUriClass(String malformedURI) {
113235
.isEqualTo(0);
114236

115237
assertThatThrownBy(() -> SdkURI.getInstance().newURI("scheme", "userInfo", malformedUri,
116-
444, "path", "query", "fragment"))
238+
444, "path", "query", "fragment"))
117239
.as("Malformed uri should throw URISyntaxException using the newURI with host method")
118240
.isInstanceOf(URISyntaxException.class);
119241
assertThat(getCache().size()).as("Cache should be empty if create URI fails")
120242
.isEqualTo(0);
121243
}
122244

245+
@ParameterizedTest
246+
@ValueSource(strings = {
247+
"http://123456789.ddb.com",
248+
"https://123456789.ddb.com",
249+
"123456789.ddb.com",
250+
"http://123.ddb.com",
251+
"https://123.ddb.com",
252+
"123.ddb.com",
253+
"http://123z.ddb.com",
254+
"https://123z.ddb.com",
255+
"123z.ddb.com",
256+
"http://1",
257+
"https://1",
258+
"1",
259+
"http://z",
260+
"https://z",
261+
"z"
262+
})
263+
void shouldNotCache_whenLeadingDigitsDoNotExceedIntegerMaxValue(String strURI) {
264+
URI uri = SdkURI.getInstance().create(strURI);
265+
assertThat(getCache().size()).isEqualTo(0);
266+
URI uri2 = SdkURI.getInstance().create(strURI);
267+
assertThat(getCache().size()).isEqualTo(0);
268+
assertThat(uri).isNotSameAs(uri2);
269+
}
270+
271+
123272
private LruCache<UriConstructorArgs, URI> getCache() {
124273
Field field = getCacheField();
125274
field.setAccessible(true);

0 commit comments

Comments
 (0)