Migrate org.apache.http.HttpResponse to org.apache.hc.core5.http.ClassicHttpResponse
#100
Conversation
bmuschko
requested review from
Copilot and
steve-aom-elliott
and removed request for
Copilot
bmuschko
changed the title
org.apache.http.HttpResponse to org.apache.hc.core5.http.ClassicHttpResponse
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/test/java/org/openrewrite/apache/httpclient5/MigrateHttpResponseTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/apache/httpclient5/MigrateHttpResponseTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/apache/httpclient5/MigrateHttpResponseTest.java
Outdated
Show resolved
Hide resolved
…sponseTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| - org.openrewrite.java.ChangeType: | ||
| oldFullyQualifiedTypeName: org.apache.http.HttpRequest | ||
| newFullyQualifiedTypeName: org.apache.hc.core5.http.ClassicHttpRequest | ||
| - org.openrewrite.java.ChangeType: | ||
| oldFullyQualifiedTypeName: org.apache.http.HttpResponse | ||
| newFullyQualifiedTypeName: org.apache.hc.core5.http.ClassicHttpResponse | ||
| - org.openrewrite.java.ChangeType: | ||
| oldFullyQualifiedTypeName: org.apache.http.client.ResponseHandler | ||
| newFullyQualifiedTypeName: org.apache.hc.core5.http.io.HttpClientResponseHandler |
There was a problem hiding this comment.
The placement for these unfortunately still feel a bit strange (not sure that a blank line above these would really help, but it would at least separate it from the block of ChangeTypes that are trying to fix things that ended up in org.apache.hc.client5.http that shouldn't have been there).
There was a problem hiding this comment.
Add a new line and comment.
| methodPatternChain: ['org.apache.hc.core5.http.HttpResponse getStatusLine()', 'org.apache.hc.core5.http.message.StatusLine getStatusCode()'] | ||
| methodPatternChain: ['org.apache.http.HttpResponse getStatusLine()', 'org.apache.http.StatusLine getStatusCode()'] | ||
| newMethodName: getCode | ||
| - org.openrewrite.java.SimplifyMethodChain: | ||
| methodPatternChain: ['org.apache.hc.core5.http.HttpResponse getStatusLine()', 'org.apache.hc.core5.http.message.StatusLine getReasonPhrase()'] | ||
| methodPatternChain: ['org.apache.http.HttpResponse getStatusLine()', 'org.apache.http.StatusLine getReasonPhrase()'] | ||
| newMethodName: getReasonPhrase | ||
| - org.openrewrite.java.SimplifyMethodChain: | ||
| methodPatternChain: [ 'org.apache.hc.core5.http.HttpResponse getStatusLine()', 'org.apache.hc.core5.http.message.StatusLine getProtocolVersion()' ] | ||
| methodPatternChain: [ 'org.apache.http.HttpResponse getStatusLine()', 'org.apache.http.StatusLine getProtocolVersion()' ] |
There was a problem hiding this comment.
Unfortunate that visually it's taking it from a "working" state with a method it knows to a "broken" state with an unknown method until lines 642-644 run at some point later
| - org.openrewrite.apache.httpclient5.UpgradeApacheHttpClient_5_DeprecatedMethods | ||
| - org.openrewrite.apache.httpclient5.UpgradeApacheHttpClient_5_TimeUnit | ||
| - org.openrewrite.apache.httpclient5.StatusLine | ||
| - org.openrewrite.apache.httpclient5.NewRequestLine |
There was a problem hiding this comment.
Might it make sense to give NewRequestLine a similar treatment for consistency? You changed NewStatusLine to operate on 4.x class name to bring it to 5.x class: should NewRequestLine operate on 4.x class name and bring it to 5.x class as well? I know the description would have to be altered a bit, but I feel like we should try to be consistent either direction: either migrate the package and then fix the class and methods or fix the methods and then later migrate the class.
There was a problem hiding this comment.
I pushed another change. From your comments, here's what I understood.
NewRequestLineandNewStatusLineshould both work based on the HttpClient 4.x API.org.openrewrite.apache.httpclient5.StatusLineneeds to fully work based on the HttpClient 4.x API.- Move the migration logic of
org.openrewrite.apache.httpclient5.StatusLinebeforeorg.openrewrite.apache.httpclient5.UpgradeApacheHttpClient_5_ClassMappingso that it can work based on the HttpClient 4.x API before packages are migrated to the HttpClient 5.x.
Let me know if I am missing something.
github-project-automation
bot
moved this from In Progress
to Ready to Review
in OpenRewrite
|
@timtebeek Looks like we are good with merging the change? |
|
Yes all good; and know that you're welcome to merge after approval; sometimes is isn't clear if any further work is planned or expected beyond the approval, so then I hold off on an immediate merge. You'd have been welcome to merge as well. :) |
What's changed?
The
org.apache.http.HttpResponsein HttpClient 4.x needs to be migrated toorg.apache.hc.core5.http.ClassicHttpResponsein HttpClient 5.x. TheClassicHttpResponseprovides some backward-compatibility to the HttpClient 4.x API.We already wanted to make that switch in our recipe list via
but that was listed after
so it didn't have any more effect as
org.apache.http.HttpResponsewas already changed toorg.apache.hc.core5.http.HttpResponse.What's your motivation?
The recipe used to change org.apache.http.HttpResponse to org.apache.hc.core5.http.HttpResponse via the following rule:
The new class doesn't provide the method
getEntity()anymore. It has been moved to HttpEntityContainer#getEntity(). Accessing the entity requires aClassicHttpResponseorClosableHttpResponse.As a result the end user experiences a compilation issues, as the method cannot be resolved.
Anything in particular you'd like reviewers to focus on?
Given the changed order of execution in the long list of recipes had an impact on other recipes, namely
NewStatusLineandNewRequestLine. The description ofNewStatusLinesays that it works based on the HttpClient 4.x API so the method matcher needs to run beforeorg.openrewrite.apache.httpclient5.UpgradeApacheHttpClient_5_ClassMapping. TheNewRequestLinechanges a HttpClient 5.x API so it needs to run afterorg.openrewrite.apache.httpclient5.UpgradeApacheHttpClient_5_ClassMapping.I noticed that we sometimes conflate recipes for HttpClient 4.x to HttpClient 5.x migration with changes that just apply to the HttpClient 5.x API but aims to fix deprecated APIs. Would it make sense to separate those concerns?