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.

Sign up for GitHub

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

Jump to bottom

feat: Introduce google-http-client-apache-v5 (Apache Client/Core 5.x) #1960

Merged

gcf-merge-on-green merged 54 commits into main from google-http-client-apache-v3-httpclient-5.x

Aug 21, 2024

Contributor

diegomarquezp commented Jun 27, 2024 •

edited

Loading

Adds a new module google-http-client-apache-v5, which is based in Apache Client/Core 5.x

The implementation was meant to keep the same functionality prescribed by the tests of google-http-client-apache-v2.

Before merging

Remove convenience integration tests from unit tests file

diegomarquezp added 4 commits

June 26, 2024 11:18


          chore: add base google-http-client-apache-v3

8efb1b1


          exclude core 4.x

6c0b7ed


          chore: demo usage of Apache HTTP Client 5.x

a4d5c06


          implement transport using classic implementation

7d7dd3a

product-auto-label bot added the size: xl label


          remove author comments

413ccae

diegomarquezp added do not merge and removed size: xl labels

suztomo reviewed

View reviewed changes

...client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpTransport.java Outdated Show resolved Hide resolved

suztomo reviewed

View reviewed changes

...client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpTransport.java Outdated Show resolved Hide resolved

suztomo reviewed

View reviewed changes

google-http-client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/Main.java Outdated Show resolved Hide resolved

diegomarquezp added 2 commits

June 27, 2024 15:34


          move test code to test class

6cde3cb


          remove redundant instanceof

27836c7

product-auto-label bot added the size: xl label

burkedavison reviewed

View reviewed changes

...p-client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpRequest.java Outdated Show resolved Hide resolved

burkedavison reviewed

View reviewed changes

...p-client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpRequest.java Outdated Show resolved Hide resolved

diegomarquezp commented

View reviewed changes

...nt-apache-v3/src/test/java/com/google/api/client/http/apache/v3/ApacheHttpTransportTest.java Outdated Show resolved Hide resolved

diegomarquezp added 2 commits

August 12, 2024 15:04


          use interface

1b17559


          prepare for production

6b6b7e3

diegomarquezp changed the title ~~chore: demo of classic API of Apache Core+Client 5.x~~ feat: Introduce google-http-client-apache-v3 (Apache Client/Core 5.3)

diegomarquezp changed the title ~~feat: Introduce google-http-client-apache-v3 (Apache Client/Core 5.3)~~ feat: Introduce google-http-client-apache-v3 (Apache Client/Core 5.x)


          expose interfaces instead of implementation classes

06971ce

burkedavison reviewed

View reviewed changes

...p-client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpRequest.java Outdated Show resolved Hide resolved

...p-client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpRequest.java Outdated Show resolved Hide resolved

...client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpTransport.java Outdated Show resolved Hide resolved

...client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpTransport.java Outdated Show resolved Hide resolved

...client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpTransport.java Outdated Show resolved Hide resolved

...p-client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpRequest.java Outdated Show resolved Hide resolved

...p-client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpRequest.java Outdated Show resolved Hide resolved

...client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpTransport.java Outdated Show resolved Hide resolved

...p-client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpRequest.java Outdated Show resolved Hide resolved

...ient-apache-v3/src/test/java/com/google/api/client/http/apache/v3/ApacheHttpRequestTest.java Outdated Show resolved Hide resolved


          use executeOpen()

cb9c2c8

burkedavison reviewed

View reviewed changes

...p-client-apache-v3/src/main/java/com/google/api/client/http/apache/v3/ApacheHttpRequest.java Outdated Show resolved Hide resolved

diegomarquezp added 5 commits

August 13, 2024 13:48


          rename to google-http-client-apache-v5

68a4c62


          format

e831876


          format

b23f1f7


          remove @SInCE annotations

46128b2


          remove unused beta annotatoins

f9216de

Contributor Author

diegomarquezp commented Aug 19, 2024

I excluded this module from the java 7 tests

google-http-java-client/.github/workflows/ci-java7.yaml

Lines 57 to 58 in fe1ef0a

    
                   mvn --batch-mode --show-version -ntp test \ 
        
                       --projects '!google-http-client-jackson2,!google-http-client-appengine,!samples/dailymotion-simple-cmdline-sample,!google-http-client-assembly,!google-http-client-apache-v5' \


          Merge remote-tracking branch 'origin/google-http-client-apache-v3-htt…

e87f1fb

…pclient-5.x' into google-http-client-apache-v3-httpclient-5.x

diegomarquezp commented

View reviewed changes

...ent-apache-v5/src/main/java/com/google/api/client/http/apache/v5/Apache5ResponseContent.java Show resolved Hide resolved

ldetmer reviewed

View reviewed changes

...ent-apache-v5/src/main/java/com/google/api/client/http/apache/v5/Apache5ResponseContent.java Show resolved Hide resolved

burkedavison reviewed

View reviewed changes

...client-apache-v5/src/main/java/com/google/api/client/http/apache/v5/Apache5HttpResponse.java Outdated Show resolved Hide resolved

burkedavison reviewed

View reviewed changes

...client-apache-v5/src/main/java/com/google/api/client/http/apache/v5/Apache5HttpResponse.java Outdated Show resolved Hide resolved

diegomarquezp added 3 commits

August 19, 2024 17:20


          add javadoc for Apache5ResponseContent

ff5c209


          close() response only, emit getContent() exception

b4049e0


          format

de87435

ldetmer reviewed

View reviewed changes

...ent-apache-v5/src/main/java/com/google/api/client/http/apache/v5/Apache5ResponseContent.java

+                }
+                @Override
+                public void close() throws IOException {

Contributor

ldetmer Aug 20, 2024

I don't see a test that verifies that the response gets closed if we close the stream. Can you point me to it, or add one if it doesn't exist?

Contributor Author

diegomarquezp Aug 20, 2024

I just added one test to verify that the response is closed when closing the content stream.

google-http-java-client/google-http-client-apache-v5/src/test/java/com/google/api/client/http/apache/v5/Apache5HttpRequestTest.java

Lines 126 to 129 in d8c7f67

    
           // we close the response's content stream and confirm the response is also closed 
        
           assertEquals(0, closedResponseCounter.get()); 
        
           response.getContent().close(); 
        
           assertEquals(1, closedResponseCounter.get());

ldetmer reviewed

View reviewed changes

...ent-apache-v5/src/test/java/com/google/api/client/http/apache/v5/Apache5HttpRequestTest.java

+              import org.apache.hc.core5.http.protocol.HttpContext;
+              import org.junit.Test;
+              public class Apache5HttpRequestTest {

Contributor

ldetmer Aug 20, 2024 •

edited

Loading

can we add a test here , or in TransportTest to check that the response content stream contains the apache 5 response?

Contributor Author

diegomarquezp Aug 20, 2024

yes, added a @VisibleForTesting method that will expose the response and a test to confirm it.

google-http-java-client/google-http-client-apache-v5/src/test/java/com/google/api/client/http/apache/v5/Apache5HttpRequestTest.java

Lines 121 to 124 in d8c7f67

    
           // we confirm that the classic response we prepared in this test is the same as the content's 
        
           // response 
        
           assertTrue(response.getContent() instanceof Apache5ResponseContent); 
        
           assertEquals(classicResponse, ((Apache5ResponseContent) response.getContent()).getResponse());

Contributor

ldetmer commented Aug 20, 2024 •

edited

Loading

I think this readme has to be updated: https://github.com/googleapis/google-http-java-client/blob/afd6afc1dbfaba403d67469431d7ac8db5630e86/docs/http-transport.md,

google-http-java-client/docs/component-modules.md

Line 4 in afd6afc

does that happen as part of this PR? or some other process?

emindeniz99 mentioned this pull request

feat: Add HTTP/2 enabled transport as default transport firebase/firebase-admin-java#979

Merged

diegomarquezp added 4 commits

August 20, 2024 15:41


          implement all methods of InputStream

5485b46


          improve javadoc of wrapped stream

86e70aa


          add test to confirm the response is close()d

e21f993


          update docs to offer Apache5Http.*

d8c7f67

Contributor Author

diegomarquezp commented Aug 20, 2024

I think this readme has to be updated: https://github.com/googleapis/google-http-java-client/blob/afd6afc1dbfaba403d67469431d7ac8db5630e86/docs/http-transport.md,

google-http-java-client/docs/component-modules.md

Line 4 in afd6afc

does that happen as part of this PR? or some other process?

@ldetmer I modified the docs to offer our new Apache5HttpTransport. One follow up is to update the link to one hosted in googleapis.dev. See example for Apache4HttpTransport.

google-http-java-client/docs/http-transport.md

Line 126 in afd6afc

    
           [apache-http-transport]: https://googleapis.dev/java/google-http-client/latest/index.html?com/google/api/client/http/apache/v2/ApacheHttpTransport.html


          exclude apache 4 from test dependencies

ec0ebb3

ldetmer approved these changes

View reviewed changes

Contributor

ldetmer left a comment

LGTM


          throw IOException when disconnect()ing

1cd91ea

Member

burkedavison commented Aug 21, 2024 •

edited by diegomarquezp

Loading

Please make sure to resolve the "Before merge" task listed in the PR description.

Remove convenience integration tests from unit tests file

burkedavison approved these changes

View reviewed changes


          remove convenience integration tests

5da2b32

diegomarquezp added the automerge label


          fix deps

5fd5dd1

gcf-merge-on-green bot merged commit 5d527dc into main

19 of 21 checks passed

gcf-merge-on-green bot removed the automerge label

gcf-merge-on-green bot deleted the google-http-client-apache-v3-httpclient-5.x branch

August 21, 2024 18:02

release-please bot mentioned this pull request

chore(main): release 1.45.0 #1946

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels