Skip to content

Commit 95e40c0

Browse files
zhabbazhabba
committed
migrated to http5, tests are green
Signed-off-by: Dmitrii Puzikov <dmitrii.puzikov@gooddata.com>
1 parent 07612c0 commit 95e40c0

27 files changed

Lines changed: 1801 additions & 1470 deletions

‎.github/prompts/deprecation-plan.md‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
# Deprecation Replacement Plan
2+
3+
## Overview
4+
This document outlines the systematic approach to replace deprecated methods in the HttpClient 4 to 5 migration.
5+
6+
## Deprecated Methods Identified
7+
8+
### 1. Apache Commons Lang3 Validation (4 instances)
9+
**Location**: `GoodDataHttpClient.java`
10+
- Lines 166, 168: `notNull(httpClient)`, `notNull(sstStrategy)`
11+
- Line 124: `notNull(authHost, "HTTP host cannot be null")`
12+
- Line 330: `notNull(request, "Request can't be null")`
13+
14+
**Replacement Strategy**:
15+
```java
16+
// Current (deprecated):
17+
notNull(object, "message")
18+
19+
// Option 1 - Objects.requireNonNull():
20+
Objects.requireNonNull(object, "message")
21+
22+
// Option 2 - Manual validation:
23+
if (object == null) {
24+
throw new IllegalArgumentException("message");
25+
}
26+
```
27+
28+
**Recommendation**: Use Objects.requireNonNull() for consistency with modern Java practices.
29+
30+
### 2. HttpClient Execute Methods (6 instances)
31+
**Locations**:
32+
- `GoodDataHttpClient.java` lines 260, 355: `httpClient.execute(host, request, context)`
33+
- `LoginSSTRetrievalStrategy.java` lines 100, 142: `httpClient.execute(host, request)`
34+
- `TestUtils.java` lines 56, 68: `httpClient.execute(host, request)`
35+
36+
**Replacement Strategy**:
37+
```java
38+
// Current (deprecated):
39+
httpClient.execute(host, request, null)
40+
httpClient.execute(host, request, context)
41+
42+
// Replacement:
43+
// For null context:
44+
httpClient.execute(host, request)
45+
46+
// For non-null context:
47+
// Use response handlers or context-aware patterns
48+
```
49+
50+
### 3. Test Framework Deprecations
51+
52+
#### Mockito (5 instances)
53+
```java
54+
// Current (deprecated):
55+
MockitoAnnotations.initMocks(this)
56+
verifyZeroInteractions(mock)
57+
58+
// Replacement:
59+
MockitoAnnotations.openMocks(this)
60+
verifyNoInteractions(mock)
61+
```
62+
63+
#### JUnit Assertions (3 instances)
64+
```java
65+
// Current (deprecated):
66+
assertThat("message", actual, matcher)
67+
68+
// Replacement:
69+
assertThat(actual, matcher)
70+
```
71+
72+
#### JUnit ExpectedException (1 instance)
73+
```java
74+
// Current (deprecated):
75+
@Rule ExpectedException expectedException = ExpectedException.none()
76+
expectedException.expect(ExceptionType.class)
77+
78+
// Replacement:
79+
assertThrows(ExceptionType.class, () -> {
80+
// code that should throw
81+
})
82+
```
83+
84+
## Implementation Priority
85+
86+
### Priority 1: Critical (Breaks Compilation)
87+
- [x] Fix BasicHttpEntity usage in tests
88+
- [x] Update HttpResponse API usage
89+
- [x] Fix type mismatches (HttpRequest -> ClassicHttpRequest)
90+
- [x] Replace removed methods (getStatusLine())
91+
92+
### Priority 2: High (Deprecation Warnings)
93+
- [ ] Update Mockito usage
94+
- [ ] Fix JUnit assertion patterns
95+
- [ ] Replace ExpectedException with assertThrows
96+
97+
### Priority 3: Medium (Deprecated but Functional)
98+
- [ ] Replace HttpClient execute methods with context
99+
- [ ] Update Commons Lang validation methods
100+
101+
### Priority 4: Low (Code Quality)
102+
- [ ] Add proper imports
103+
- [ ] Remove unused imports
104+
- [ ] Optimize method signatures
105+
106+
## Rollout Strategy
107+
108+
### Phase 1: Make Compilable ✅
109+
- Fix all compilation errors
110+
- Update critical API usage
111+
112+
### Phase 2: Remove Deprecation Warnings
113+
- Update test framework usage
114+
- Replace deprecated method calls
115+
116+
### Phase 3: Optimize
117+
- Implement modern patterns
118+
- Add HttpClient 5 enhancements
119+
- Performance optimizations
120+
121+
## Risk Assessment
122+
123+
### Low Risk
124+
- Commons Lang validation replacement
125+
- Mockito method updates
126+
- JUnit assertion patterns
127+
128+
### Medium Risk
129+
- HttpClient execute method changes
130+
- HttpContext handling updates
131+
132+
### High Risk
133+
- Major API pattern changes
134+
- Response handler implementations
135+
136+
## Testing Strategy
137+
138+
1. **Unit Tests**: Ensure all existing tests pass
139+
2. **Integration Tests**: Verify authentication flows work
140+
3. **Regression Tests**: Check backwards compatibility
141+
4. **Performance Tests**: Validate no performance degradation

‎.github/prompts/exception-analysis.md‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
# Exception Analysis and Recommendations for GoodDataHttpClient
2+
3+
## Overview
4+
This document analyzes the three TODO comments in `GoodDataHttpClient.java` related to exception handling and provides specific recommendations for appropriate exceptions to throw.
5+
6+
## Current TODO Locations and Analysis
7+
8+
### 1. TODO #1 (Line 299): HttpException in Response Handler
9+
```java
10+
public <T> T execute(ClassicHttpRequest request, HttpContext context,
11+
HttpClientResponseHandler<? extends T> responseHandler) throws IOException {
12+
final HttpResponse resp = execute(request, context);
13+
try {
14+
return responseHandler.handleResponse((ClassicHttpResponse) resp);
15+
} catch (HttpException e) {
16+
throw new RuntimeException(e); //TODO:: clarify which exception to use
17+
}
18+
}
19+
```
20+
21+
**Context Analysis:**
22+
- Method declares `throws IOException`
23+
- `HttpException` is thrown by `responseHandler.handleResponse()`
24+
- This is a wrapper method implementing the `HttpClient` interface
25+
26+
**Recommendation: Wrap in IOException**
27+
```java
28+
} catch (HttpException e) {
29+
throw new IOException("Failed to handle HTTP response", e);
30+
}
31+
```
32+
33+
**Rationale:**
34+
- The method signature only allows `IOException` to be thrown
35+
- `HttpException` represents HTTP protocol-level issues
36+
- Wrapping preserves the original cause while conforming to the interface
37+
- This follows the principle of translating checked exceptions at API boundaries
38+
39+
### 2. TODO #2 (Line 309): URISyntaxException in URI Parsing
40+
```java
41+
public HttpResponse execute(ClassicHttpRequest request, HttpContext context) throws IOException {
42+
final URI uri;
43+
try {
44+
uri = request.getUri();
45+
} catch (URISyntaxException e) {
46+
throw new RuntimeException(e); //TODO:: clarify which exception to use
47+
}
48+
final HttpHost httpHost = new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort());
49+
return execute(httpHost, request, context);
50+
}
51+
```
52+
53+
**Context Analysis:**
54+
- Method declares `throws IOException`
55+
- `URISyntaxException` indicates malformed URI in the request
56+
- This is a client-side configuration error, not a network I/O issue
57+
58+
**Recommendation: Use IllegalArgumentException**
59+
```java
60+
} catch (URISyntaxException e) {
61+
throw new IllegalArgumentException("Invalid URI in request: " + e.getMessage(), e);
62+
}
63+
```
64+
65+
**Rationale:**
66+
- `URISyntaxException` represents a programming error - invalid input
67+
- `IllegalArgumentException` is the standard Java exception for invalid arguments
68+
- The method signature only allows `IOException`, but `IllegalArgumentException` is unchecked
69+
- This clearly indicates a client-side error rather than a server/network issue
70+
71+
### 3. TODO #3 (Line 367): HttpException in Response Handler (duplicate pattern)
72+
```java
73+
public <T> T execute(HttpHost target, ClassicHttpRequest request, HttpContext context,
74+
HttpClientResponseHandler<? extends T> responseHandler) throws IOException {
75+
HttpResponse resp = execute(target, request, context);
76+
try {
77+
return responseHandler.handleResponse((ClassicHttpResponse) resp);
78+
} catch (HttpException e) {
79+
throw new RuntimeException(e); //TODO:: clarify which one to use here
80+
}
81+
}
82+
```
83+
84+
**Context Analysis:**
85+
- Identical pattern to TODO #1
86+
- Method declares `throws IOException`
87+
- Same `HttpException` handling issue
88+
89+
**Recommendation: Same as TODO #1 - Wrap in IOException**
90+
```java
91+
} catch (HttpException e) {
92+
throw new IOException("Failed to handle HTTP response", e);
93+
}
94+
```
95+
96+
**Rationale:**
97+
- Consistency with TODO #1 resolution
98+
- Same underlying issue and constraints
99+
100+
## Summary of Recommended Changes
101+
102+
### Exception Mapping Strategy
103+
104+
1. **HttpException → IOException**
105+
- Used for: Response handling failures
106+
- Reason: Protocol-level errors that should be treated as I/O issues
107+
- Pattern: `new IOException("Failed to handle HTTP response", cause)`
108+
109+
2. **URISyntaxException → IllegalArgumentException**
110+
- Used for: Invalid URI in requests
111+
- Reason: Client-side configuration error
112+
- Pattern: `new IllegalArgumentException("Invalid URI in request: " + cause.getMessage(), cause)`
113+
114+
### Implementation Priority
115+
116+
**High Priority (Breaking Changes):**
117+
- All three TODOs should be resolved together
118+
- These are runtime behavior changes that could affect error handling
119+
120+
**Benefits of These Changes:**
121+
1. **Clearer Error Semantics**: Different exception types clearly indicate different error categories
122+
2. **Better Debugging**: More specific exception messages help identify root causes
123+
3. **Interface Compliance**: Proper adherence to method signatures
124+
4. **Consistency**: Uniform approach to exception translation
125+
126+
### Alternative Considerations
127+
128+
**Option 1: Custom GoodData Exceptions**
129+
Could create `GoodDataHttpException` extending `IOException`, but this adds complexity without clear benefits since the existing exceptions adequately represent the error conditions.
130+
131+
**Option 2: Just Use IOException for All**
132+
Could wrap `URISyntaxException` in `IOException` for consistency, but `IllegalArgumentException` better represents the nature of URI syntax errors.
133+
134+
**Recommended Approach: Mixed Strategy**
135+
Use the most semantically appropriate exception type for each case while respecting method signature constraints.
136+
137+
## Code Quality Impact
138+
139+
These changes will:
140+
- ✅ Improve error handling clarity
141+
- ✅ Provide better debugging information
142+
- ✅ Follow Java exception handling best practices
143+
- ✅ Maintain backward compatibility (exception types, not RuntimeException)
144+
- ✅ Reduce generic RuntimeException usage
145+
146+
## Testing Considerations
147+
148+
After implementing these changes:
149+
1. Update tests to expect the new exception types
150+
2. Verify error messages are descriptive
151+
3. Ensure exception causes are preserved
152+
4. Test both success and failure scenarios

‎.github/prompts/phase1-analysis.md‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
# Phase 1: HttpClient 4 to 5 Migration Analysis
2+
3+
## Overview
4+
This document outlines the analysis and plan for completing the HttpClient 4 to 5 migration in GoodDataHttpClient.
5+
6+
## Current Status
7+
- **Migration Progress**: ~80% complete
8+
- **Main Code**: Mostly functional with deprecation warnings
9+
- **Test Code**: Multiple compilation failures
10+
- **Dependencies**: Already updated to HttpClient 5.5.1
11+
12+
## Critical Issues Blocking Compilation
13+
14+
### 1. Test File Compilation Failures
15+
16+
#### GoodDataHttpClientTest.java
17+
- **BasicHttpEntity Constructor**: No-arg constructor doesn't exist in HttpClient 5
18+
- **HttpResponse.setEntity()**: Method doesn't exist, use ClassicHttpResponse instead
19+
- **HttpResponse.getStatusLine()**: Method removed, use getCode() and getReasonPhrase()
20+
- **HttpRequestBase**: Class doesn't exist, replaced with ClassicHttpRequest
21+
- **Type Mismatches**: HttpRequest vs ClassicHttpRequest in mocks
22+
23+
#### LoginSSTRetrievalStrategyTest.java
24+
- **ExpectedException**: Deprecated JUnit 4 pattern
25+
- **Deprecated Assertions**: 3-parameter assertThat() usage
26+
- **Mock Methods**: verifyZeroInteractions() deprecated
27+
28+
#### Other Test Files
29+
- **TokenUtilsTest.java**: Deprecated assertThat() usage
30+
- **TestUtils.java**: Deprecated execute() method calls
31+
32+
### 2. Deprecated Method Usage (Warnings Only)
33+
34+
#### Main Code
35+
- **Validate.notNull()**: Deprecated in Apache Commons Lang3
36+
- **HttpClient.execute()**: Some overloads deprecated in HttpClient 5
37+
38+
## Phase 1 Tasks
39+
40+
### Phase 1a: Make Project Buildable ✅
41+
- [x] Analyze current state
42+
- [x] Identify compilation blockers
43+
- [x] Document deprecated usage without fixing
44+
45+
### Phase 1b: Fix Tests and Imports ✅
46+
- [x] Fix BasicHttpEntity usage in tests
47+
- [x] Update HttpResponse API usage
48+
- [x] Fix type mismatches in mocks
49+
- [x] Update test framework usage
50+
- [x] Fix imports and assertions
51+
52+
### Phase 1c: Create Deprecation Plan ✅
53+
- [x] Document all deprecated method usage
54+
- [x] Create replacement strategy
55+
- [x] Prioritize by impact and complexity
56+
57+
### Phase 1d: Implement Deprecation Fixes
58+
- [ ] Replace deprecated validation methods
59+
- [ ] Update HttpClient execute method calls
60+
- [ ] Implement proper HttpContext handling
61+
62+
## Success Criteria
63+
- [x] Project compiles without errors
64+
- [x] All tests pass
65+
- [x] Documentation of deprecation plan created
66+
- [ ] Deprecation warnings addressed
67+
68+
## Next Steps
69+
1. Fix test compilation issues
70+
2. Ensure all tests pass
71+
3. Create detailed deprecation replacement plan
72+
4. Implement deprecation fixes systematically

0 commit comments

Comments
 (0)