Skip to content

Commit d022d62

Browse files
authored
[ZEPPELIN-6245] Improve Repository class stability and validation
# Improve Repository Class Stability and Validation ## Summary This PR enhances the `Repository` class with comprehensive input validation, error handling, and stability improvements to ensure robust Maven repository configuration management. ## Changes Made ### 1. Enhanced Input Validation - **Repository ID validation**: Added regex pattern validation to ensure IDs contain only alphanumeric characters, dots, underscores, and hyphens - **URL validation**: Implemented URL format validation supporting HTTP, HTTPS, and FILE protocols - **Credentials validation**: Added validation to ensure both username and password are provided together - **Proxy validation**: Added comprehensive proxy configuration validation including protocol, host, and port checks ### 2. Improved Error Handling - **Custom Exception**: Created `RepositoryException` class for Repository-specific errors - **JSON parsing**: Enhanced error handling for malformed JSON with descriptive error messages - **Validation errors**: Comprehensive error messages for all validation failures ### 3. Builder Pattern Enhancements - **Method chaining**: Improved builder pattern with proper validation at each step - **Fluent API**: Enhanced usability with intuitive method names and return types ### 4. JSON Serialization Improvements - **Robust parsing**: Enhanced `fromJson()` method with comprehensive validation - **Backward compatibility**: Maintained compatibility with existing JSON formats - **Error resilience**: Graceful handling of invalid JSON inputs ### 5. Documentation and Code Quality - **Comprehensive JavaDoc**: Added detailed documentation with examples and parameter descriptions - **Usage examples**: Included code examples in class-level documentation - **Validation documentation**: Clear documentation of all validation rules ## Benefits 1. **Improved Reliability**: Input validation prevents runtime errors from malformed configurations 2. **Better Error Messages**: Clear, actionable error messages help users identify and fix configuration issues 3. **Enhanced Security**: URL and credential validation prevents potential security issues 4. **Maintainability**: Clean, well-documented code with comprehensive test coverage 5. **User Experience**: Better error handling and validation feedback ## Testing - All existing tests pass - New comprehensive test suite covers edge cases and validation scenarios - Integration tests verify backward compatibility - Manual testing confirms REST API functionality ## Example Usage ```java // Basic repository configuration Repository repo = new Repository("central") .url("https://repo.maven.apache.org/maven2/"); // Repository with authentication Repository privateRepo = new Repository("private") .url("https://private.repo/maven2/") .credentials("username", "password"); // Repository with proxy configuration Repository proxyRepo = new Repository("proxy-repo") .url("https://repo.example.com/maven2/") .proxy("HTTP", "proxy.host", 8080, "proxyUser", "proxyPass"); ``` Closes #4977 from renechoi/improve-repository-validation. Signed-off-by: Philipp Dallig <philipp.dallig@gmail.com>
1 parent 410b160 commit d022d62

File tree

8 files changed

+875
-105
lines changed

8 files changed

+875
-105
lines changed

zeppelin-interpreter/src/main/java/org/apache/zeppelin/dep/Repository.java

Lines changed: 353 additions & 42 deletions
Large diffs are not rendered by default.
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
18+
package org.apache.zeppelin.dep;
19+
20+
/**
21+
* Exception thrown when there are issues with Repository configuration or operations.
22+
*/
23+
public class RepositoryException extends RuntimeException {
24+
25+
public RepositoryException(String message) {
26+
super(message);
27+
}
28+
29+
public RepositoryException(String message, Throwable cause) {
30+
super(message, cause);
31+
}
32+
}
Lines changed: 303 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,303 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
18+
package org.apache.zeppelin.dep;
19+
20+
import org.eclipse.aether.repository.RemoteRepository;
21+
import org.junit.jupiter.api.Test;
22+
23+
import static org.junit.jupiter.api.Assertions.*;
24+
25+
class RepositoryTest {
26+
27+
@Test
28+
void testToRemoteRepository() {
29+
// Test basic repository conversion
30+
Repository repo = new Repository.Builder("test-repo")
31+
.url("https://repo.maven.apache.org/maven2/")
32+
.build();
33+
34+
RemoteRepository remoteRepo = repo.toRemoteRepository();
35+
36+
assertEquals("test-repo", remoteRepo.getId());
37+
assertEquals("https://repo.maven.apache.org/maven2/", remoteRepo.getUrl());
38+
assertEquals("default", remoteRepo.getContentType());
39+
assertNotNull(remoteRepo.getPolicy(false));
40+
assertTrue(remoteRepo.getPolicy(false).isEnabled());
41+
}
42+
43+
@Test
44+
void testToRemoteRepositoryWithSnapshot() {
45+
Repository repo = new Repository.Builder("snapshot-repo")
46+
.url("https://repo.maven.apache.org/maven2/")
47+
.snapshot()
48+
.build();
49+
50+
RemoteRepository remoteRepo = repo.toRemoteRepository();
51+
52+
assertEquals("snapshot-repo", remoteRepo.getId());
53+
assertTrue(remoteRepo.getPolicy(true).isEnabled());
54+
}
55+
56+
@Test
57+
void testToRemoteRepositoryWithAuthentication() {
58+
Repository repo = new Repository.Builder("auth-repo")
59+
.url("https://private.repo/maven2/")
60+
.credentials("user", "pass")
61+
.build();
62+
63+
RemoteRepository remoteRepo = repo.toRemoteRepository();
64+
65+
assertNotNull(remoteRepo.getAuthentication());
66+
}
67+
68+
@Test
69+
void testToRemoteRepositoryWithProxy() {
70+
Repository repo = new Repository.Builder("proxy-repo")
71+
.url("https://repo.maven.apache.org/maven2/")
72+
.proxy("HTTP", "proxy.host", 8080, "proxyUser", "proxyPass")
73+
.build();
74+
75+
RemoteRepository remoteRepo = repo.toRemoteRepository();
76+
77+
assertNotNull(remoteRepo.getProxy());
78+
assertEquals("proxy.host", remoteRepo.getProxy().getHost());
79+
assertEquals(8080, remoteRepo.getProxy().getPort());
80+
}
81+
82+
@Test
83+
void testFromRemoteRepository() {
84+
RemoteRepository remoteRepo = new RemoteRepository.Builder("central", "default",
85+
"https://repo.maven.apache.org/maven2/")
86+
.setReleasePolicy(new org.eclipse.aether.repository.RepositoryPolicy(true, null, null))
87+
.setSnapshotPolicy(new org.eclipse.aether.repository.RepositoryPolicy(false, null, null))
88+
.build();
89+
90+
Repository repo = Repository.fromRemoteRepository(remoteRepo);
91+
92+
assertEquals("central", repo.getId());
93+
assertEquals("https://repo.maven.apache.org/maven2/", repo.getUrl());
94+
assertFalse(repo.isSnapshot());
95+
}
96+
97+
@Test
98+
void testFromRemoteRepositoryWithSnapshot() {
99+
RemoteRepository remoteRepo = new RemoteRepository.Builder("snapshots", "default",
100+
"https://repo.maven.apache.org/maven2/")
101+
.setSnapshotPolicy(new org.eclipse.aether.repository.RepositoryPolicy(true, null, null))
102+
.build();
103+
104+
Repository repo = Repository.fromRemoteRepository(remoteRepo);
105+
106+
assertEquals("snapshots", repo.getId());
107+
assertTrue(repo.isSnapshot());
108+
}
109+
110+
@Test
111+
void testRoundTripConversion() {
112+
// Test that conversion is consistent (with data loss for auth/proxy)
113+
Repository original = new Repository.Builder("test")
114+
.url("https://test.repo/maven2/")
115+
.snapshot()
116+
.build();
117+
118+
RemoteRepository remote = original.toRemoteRepository();
119+
Repository converted = Repository.fromRemoteRepository(remote);
120+
121+
assertEquals(original.getId(), converted.getId());
122+
assertEquals(original.getUrl(), converted.getUrl());
123+
assertEquals(original.isSnapshot(), converted.isSnapshot());
124+
}
125+
126+
@Test
127+
void testJsonSerialization() {
128+
Repository repo = new Repository.Builder("json-test")
129+
.url("https://test.repo/")
130+
.credentials("user", "pass")
131+
.proxy("HTTP", "proxy", 8080, "puser", "ppass")
132+
.build();
133+
134+
String json = repo.toJson();
135+
Repository deserialized = Repository.fromJson(json);
136+
137+
assertEquals(repo.getId(), deserialized.getId());
138+
assertEquals(repo.getUrl(), deserialized.getUrl());
139+
// Test that credentials are preserved in JSON
140+
assertNotNull(deserialized.getAuthentication());
141+
assertNotNull(deserialized.getProxy());
142+
}
143+
144+
// Input validation tests
145+
@Test
146+
void testInvalidRepositoryId() {
147+
// Test null ID
148+
assertThrows(RepositoryException.class, () -> new Repository.Builder(null));
149+
150+
// Test empty ID
151+
assertThrows(RepositoryException.class, () -> new Repository.Builder(""));
152+
153+
// Test invalid characters in ID
154+
assertThrows(RepositoryException.class, () -> new Repository.Builder("repo@invalid"));
155+
assertThrows(RepositoryException.class, () -> new Repository.Builder("repo with spaces"));
156+
assertThrows(RepositoryException.class, () -> new Repository.Builder("repo/with/slash"));
157+
}
158+
159+
@Test
160+
void testValidRepositoryId() {
161+
// Test valid IDs
162+
assertDoesNotThrow(() -> new Repository.Builder("central").build());
163+
assertDoesNotThrow(() -> new Repository.Builder("my-repo").build());
164+
assertDoesNotThrow(() -> new Repository.Builder("repo_123").build());
165+
assertDoesNotThrow(() -> new Repository.Builder("repo.with.dots").build());
166+
assertDoesNotThrow(() -> new Repository.Builder("123-repo-456").build());
167+
}
168+
169+
@Test
170+
void testInvalidUrl() {
171+
Repository.Builder builder = new Repository.Builder("test");
172+
173+
// Test null URL
174+
assertThrows(RepositoryException.class, () -> builder.url(null));
175+
176+
// Test empty URL
177+
assertThrows(RepositoryException.class, () -> builder.url(""));
178+
179+
// Test invalid URL format
180+
assertThrows(RepositoryException.class, () -> builder.url("not-a-url"));
181+
assertThrows(RepositoryException.class, () -> builder.url("ftp://invalid-protocol"));
182+
}
183+
184+
@Test
185+
void testValidUrl() {
186+
Repository.Builder builder = new Repository.Builder("test");
187+
188+
// Test valid URLs
189+
assertDoesNotThrow(() -> builder.url("https://repo.maven.apache.org/maven2/").build());
190+
assertDoesNotThrow(() -> builder.url("http://localhost:8080/nexus/").build());
191+
assertDoesNotThrow(() -> builder.url("file:///home/user/.m2/repository/").build());
192+
}
193+
194+
@Test
195+
void testInvalidCredentials() {
196+
Repository.Builder builder = new Repository.Builder("test");
197+
198+
// Test username without password
199+
assertThrows(RepositoryException.class, () -> builder.credentials("user", null));
200+
assertThrows(RepositoryException.class, () -> builder.credentials("user", ""));
201+
202+
// Test password without username
203+
assertThrows(RepositoryException.class, () -> builder.credentials(null, "pass"));
204+
assertThrows(RepositoryException.class, () -> builder.credentials("", "pass"));
205+
}
206+
207+
@Test
208+
void testValidCredentials() {
209+
Repository.Builder builder = new Repository.Builder("test");
210+
211+
// Test valid credentials
212+
assertDoesNotThrow(() -> builder.credentials("user", "pass").build());
213+
assertDoesNotThrow(() -> builder.credentials(null, null).build());
214+
}
215+
216+
@Test
217+
void testInvalidProxy() {
218+
Repository.Builder builder = new Repository.Builder("test");
219+
220+
// Test invalid protocol
221+
assertThrows(RepositoryException.class, () -> builder.proxy("FTP", "proxy.host", 8080, null, null));
222+
assertThrows(RepositoryException.class, () -> builder.proxy(null, "proxy.host", 8080, null, null));
223+
assertThrows(RepositoryException.class, () -> builder.proxy("", "proxy.host", 8080, null, null));
224+
225+
// Test invalid host
226+
assertThrows(RepositoryException.class, () -> builder.proxy("HTTP", null, 8080, null, null));
227+
assertThrows(RepositoryException.class, () -> builder.proxy("HTTP", "", 8080, null, null));
228+
229+
// Test invalid port
230+
assertThrows(RepositoryException.class, () -> builder.proxy("HTTP", "proxy.host", 0, null, null));
231+
assertThrows(RepositoryException.class, () -> builder.proxy("HTTP", "proxy.host", -1, null, null));
232+
assertThrows(RepositoryException.class, () -> builder.proxy("HTTP", "proxy.host", 65536, null, null));
233+
}
234+
235+
@Test
236+
void testValidProxy() {
237+
Repository.Builder builder = new Repository.Builder("test");
238+
239+
// Test valid proxy configurations
240+
assertDoesNotThrow(() -> builder.proxy("HTTP", "proxy.host", 8080, null, null).build());
241+
assertDoesNotThrow(() -> new Repository.Builder("test2").proxy("HTTPS", "proxy.host", 443, "user", "pass").build());
242+
assertDoesNotThrow(() -> new Repository.Builder("test3").proxy("http", "proxy.host", 3128, null, null).build()); // case insensitive
243+
}
244+
245+
@Test
246+
void testInvalidJsonDeserialization() {
247+
// Test null JSON
248+
assertThrows(RepositoryException.class, () -> Repository.fromJson(null));
249+
250+
// Test empty JSON
251+
assertThrows(RepositoryException.class, () -> Repository.fromJson(""));
252+
253+
// Test invalid JSON format
254+
assertThrows(RepositoryException.class, () -> Repository.fromJson("not-json"));
255+
assertThrows(RepositoryException.class, () -> Repository.fromJson("{invalid json}"));
256+
257+
// Test JSON that results in null
258+
assertThrows(RepositoryException.class, () -> Repository.fromJson("null"));
259+
}
260+
261+
@Test
262+
void testValidJsonDeserialization() {
263+
String validJson = "{\"id\":\"test\",\"url\":\"https://repo.maven.apache.org/maven2/\",\"snapshot\":false}";
264+
265+
assertDoesNotThrow(() -> {
266+
Repository repo = Repository.fromJson(validJson);
267+
assertEquals("test", repo.getId());
268+
assertEquals("https://repo.maven.apache.org/maven2/", repo.getUrl());
269+
assertFalse(repo.isSnapshot());
270+
});
271+
}
272+
273+
@Test
274+
void testToRemoteRepositoryValidation() {
275+
// Test with missing URL
276+
Repository repo = new Repository.Builder("test").build();
277+
assertThrows(RepositoryException.class, () -> repo.toRemoteRepository());
278+
279+
// Test with valid repository
280+
Repository validRepo = new Repository.Builder("test")
281+
.url("https://repo.maven.apache.org/maven2/")
282+
.build();
283+
assertDoesNotThrow(() -> validRepo.toRemoteRepository());
284+
}
285+
286+
@Test
287+
void testFromRemoteRepositoryValidation() {
288+
// Test with null RemoteRepository
289+
assertThrows(RepositoryException.class, () -> Repository.fromRemoteRepository(null));
290+
}
291+
292+
@Test
293+
void testBackwardCompatibilityWithEmptyUrl() {
294+
// Test that repositories with empty URLs can be parsed (for backward compatibility)
295+
String jsonWithoutUrl = "{\"id\":\"test\",\"snapshot\":false}";
296+
297+
assertDoesNotThrow(() -> {
298+
Repository repo = Repository.fromJson(jsonWithoutUrl);
299+
assertEquals("test", repo.getId());
300+
assertNull(repo.getUrl());
301+
});
302+
}
303+
}

zeppelin-server/src/main/java/org/apache/zeppelin/rest/InterpreterRestApi.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,7 @@ public Response listRepositories() {
261261
public Response addRepository(String message) {
262262
try {
263263
Repository request = Repository.fromJson(message);
264-
interpreterSettingManager.addRepository(request.getId(), request.getUrl(),
265-
request.isSnapshot(), request.getAuthentication(), request.getProxy());
264+
interpreterSettingManager.addRepository(request);
266265
LOGGER.info("New repository {} added", request.getId());
267266
} catch (Exception e) {
268267
LOGGER.error("Exception in InterpreterRestApi while adding repository ", e);

0 commit comments

Comments
 (0)