feat: modify user agent string #683

sbansla · 2022-05-19T17:42:37Z

Fixes

Standardisation of UserAgent String as per below mentioned doc
https://docs.google.com/document/d/1dDAvu9N4wkTnZKmWx5iiMfU42b7qYLAegA_QbZoBkU0/edit#

Checklist

I acknowledge that all my contributions will be made under the project's license
I have made a material change to the repo (functionality, testing, spelling, grammar)
I have read the Contribution Guidelines and my PR follows them
I have titled the PR appropriately
I have updated my branch with the main branch
I have added tests that prove my fix is effective or that my feature works
I have added the necessary documentation about the functionality in the appropriate .md file
I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

…nd made it extensible

…ture/DII-365-Modify-user-string

…ture/Modify-UserAgent-string

childish-sambino

Coverage is a bit low. Should see where more tests would be useful.

src/test/java/com/twilio/http/HttpUtilityTest.java

shrutiburman · 2022-05-20T05:30:57Z

Could you link the issue that this PR solves, in the description ?

shrutiburman · 2022-05-20T05:46:50Z

src/main/java/com/twilio/http/NetworkHttpClient.java

                new BasicHeader(HttpHeaders.ACCEPT, "application/json"),
                new BasicHeader(HttpHeaders.ACCEPT_ENCODING, "utf-8")
        );
+        isCustomClient = true;


What's the purpose of this flag? Would it ever equal to false?

it is to know if client(developer) has created custom httpclient. I am using this flag to add " custom" String in UserAgentString.

I guess I am missing something here. Won't it be always true?

By default it will be false, It will be true when NetworkHttpClient instance is created by using constructor:
"NetworkHttpClient(HttpClientBuilder clientBuilder)" (Means by passing custom HttpClientBuilder)

Example:
Check class ProxiedTwilioClientCreator in below mentioned link:
https://www.twilio.com/docs/libraries/java/custom-http-clients-java#

Got it. Thanks.

sbansla · 2022-05-20T05:50:07Z

Could you link the issue that this PR solves, in the description ?

Added

childish-sambino

High-level feedback on the new tests: they do execute the new/changed lines and have assertions, but the assertions are overly loose. They should assert on the expected impact of the new behavior rather than the new behavior not having a negative impact.

Let me know if you'd like to sync for more discussion/clarification.

src/test/java/com/twilio/TwilioTest.java

src/test/java/com/twilio/http/NetworkHttpClientTest.java

src/test/java/com/twilio/http/TwilioRestClientTest.java

childish-sambino · 2022-05-20T14:49:04Z

src/test/java/com/twilio/http/TwilioRestClientTest.java

    private TwilioRestClient twilioRestClient;

+    @Injectable


Don't think this annotation is needed since setUp sets the value.

Not able make method call without making variable as @Injectable
This was strange to me.

sbansla · 2022-05-20T19:37:13Z

High-level feedback on the new tests: they do execute the new/changed lines and have assertions, but the assertions are overly loose. They should assert on the expected impact of the new behavior rather than the new behavior not having a negative impact.

Let me know if you'd like to sync for more discussion/clarification.

--->
Corrected the test cases wherever possible.
File: TwilioRestClientTest, I have marked variable twilioRestClientExtension as @Injectable. If I don't mark it as @Injectable, Execution of method won't happen.
twilioRestClientExtension.request(request) --> Won't succeed without @Injectable
This behaviour was strange to me.

…tString in Twilio

childish-sambino · 2022-05-20T20:45:09Z

src/test/java/com/twilio/TwilioTest.java

+        Twilio.setPassword("Password");
+        Twilio.setUserAgentExtensions(Arrays.asList("ce-appointment-reminders/1.0.0", "code-exchange"));
+        Twilio.getRestClient();
+        assertEquals(Twilio.getUserAgentExtensions(), Arrays.asList("ce-appointment-reminders/1.0.0", "code-exchange"));


nit: Should flip the arg order so it's expected, actual

childish-sambino · 2022-05-20T20:45:42Z

src/test/java/com/twilio/http/HttpUtilityTest.java

+    private List<String> userAgentExtensionsEmpty = new ArrayList<>();;
+    private List<String> userAgentExtensions = Arrays.asList("ce-appointment-reminders/1.0.0", "code-exchange");;


nit: Double ; at EOL.

childish-sambino · 2022-05-20T20:47:03Z

src/test/java/com/twilio/http/TwilioRestClientTest.java

+
+    @Test
+    public void testRequestWithExtention() {
+        Request request = new Request(
+                HttpMethod.GET,
+                Domains.API.toString(),
+                "/2010-04-01/Accounts/" + "AC123" + "/Messages/" + "MM123" + ".json"
+        );
+        twilioRestClientExtension = new TwilioRestClient.Builder("AC123", "AUTH TOKEN")
+                .userAgentExtensions(userAgentStringExtensions)
+                .build();
+        twilioRestClientExtension.request(request);
+        assertEquals(request.getUserAgentExtensions(), userAgentStringExtensions);
+    }
+
+    @Test
+    public void testRequestWithExtentionEmpty() {
+        Request request = new Request(
+                HttpMethod.GET,
+                Domains.API.toString(),
+                "/2010-04-01/Accounts/" + "AC123" + "/Messages/" + "MM123" + ".json"
+        );
+        twilioRestClientExtension = new TwilioRestClient.Builder("AC123", "AUTH TOKEN")
+                .userAgentExtensions(Arrays.asList())
+                .build();
+        twilioRestClientExtension.request(request);
+        assertEquals(request.getUserAgentExtensions(), null);
+    }
+
+    @Test
+    public void testRequestWithExtentionNull() {
+        Request request = new Request(
+                HttpMethod.GET,
+                Domains.API.toString(),
+                "/2010-04-01/Accounts/" + "AC123" + "/Messages/" + "MM123" + ".json"
+        );
+        twilioRestClientExtension = new TwilioRestClient.Builder("AC123", "AUTH TOKEN")
+                .userAgentExtensions(null)
+                .build();
+        twilioRestClientExtension.request(request);
+        assertEquals(request.getUserAgentExtensions(), null);
+    }


s/Extention/Extension/

childish-sambino · 2022-05-20T20:47:30Z

src/test/java/com/twilio/http/TwilioRestClientTest.java

+        Request request = new Request(
+                HttpMethod.GET,
+                Domains.API.toString(),
+                "/2010-04-01/Accounts/" + "AC123" + "/Messages/" + "MM123" + ".json"


nit: No need for string concatenation.

childish-sambino · 2022-05-20T20:48:18Z

src/test/java/com/twilio/http/TwilioRestClientTest.java

+                .userAgentExtensions(null)
+                .build();
+        twilioRestClientExtension.request(request);
+        assertEquals(request.getUserAgentExtensions(), null);


nit: assertNull

Recommend installing the SonarQube IDE plugin which alerts about things like this.

shrutiburman

🚀

sonarqubecloud · 2022-05-23T16:57:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

87.9% Coverage
0.0% Duplication

sbansla added 5 commits May 7, 2022 19:54

feat(DII-365): User-Agent string modified to include os information a…

df4b305

…nd made it extensible

Merge branch 'main' of https://github.com/twilio/twilio-java into fea…

73bdbe2

…ture/DII-365-Modify-user-string

feat: (dii 365) added test cases to cover extensible user agent string

4469880

feat: removed userAgentString from constructor

3a56759

Merge branch 'main' of https://github.com/twilio/twilio-java into fea…

c3f5466

…ture/Modify-UserAgent-string

sbansla requested a review from childish-sambino May 19, 2022 17:42

sbansla changed the title ~~Feature/modify user agent string~~ feat: modify user agent string May 19, 2022

childish-sambino reviewed May 19, 2022

View reviewed changes

sbansla requested a review from shrutiburman May 20, 2022 05:46

shrutiburman reviewed May 20, 2022

View reviewed changes

feat: added test cases

8e50894

childish-sambino reviewed May 20, 2022

View reviewed changes

feat: corrected test cases and added feature to reset static userAgen…

5f04311

…tString in Twilio

childish-sambino approved these changes May 20, 2022

View reviewed changes

shrutiburman approved these changes May 23, 2022

View reviewed changes

feat: corrected pmd and sonar issues

9917975

sbansla merged commit 2c29bbe into main May 25, 2022

sbansla deleted the feature/Modify-UserAgent-string branch May 25, 2022 05:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: modify user agent string #683

feat: modify user agent string #683

sbansla commented May 19, 2022 •

edited

Loading

childish-sambino left a comment

shrutiburman commented May 20, 2022

shrutiburman May 20, 2022

sbansla May 20, 2022

shrutiburman May 20, 2022

sbansla May 20, 2022

shrutiburman May 23, 2022

sbansla commented May 20, 2022

childish-sambino left a comment

childish-sambino May 20, 2022

sbansla May 20, 2022

sbansla commented May 20, 2022 •

edited

Loading

childish-sambino May 20, 2022

childish-sambino May 20, 2022

childish-sambino May 20, 2022

childish-sambino May 20, 2022

childish-sambino May 20, 2022

shrutiburman left a comment

sonarqubecloud bot commented May 23, 2022

		private List<String> userAgentExtensionsEmpty = new ArrayList<>();;
		private List<String> userAgentExtensions = Arrays.asList("ce-appointment-reminders/1.0.0", "code-exchange");;

feat: modify user agent string #683

feat: modify user agent string #683

Conversation

sbansla commented May 19, 2022 • edited Loading

Fixes

Checklist

childish-sambino left a comment

Choose a reason for hiding this comment

shrutiburman commented May 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbansla commented May 20, 2022

childish-sambino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbansla commented May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrutiburman left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 23, 2022

sbansla commented May 19, 2022 •

edited

Loading

sbansla commented May 20, 2022 •

edited

Loading