Configurable user agent

[cezarykluczynski]

Hi,

it seems UA string is hardcoded here:

jcabi-github/src/main/java/com/jcabi/github/RtGithub.java

Line 83 in 361bb23

private static final String USER_AGENT = String.format(

Would you consider making it configurable?

It seems, per documentation, that it should be an application user agent, rather than a user agent of client used by application.

https://developer.github.com/v3/#user-agent-required

[pecko]

@cezarykluczynski

It will probably be changed with #1162
You will be able to add the jcabi.properties in classpath (before jcabi-github library) and define your UA.
See https://github.com/pecko/jcabi-github/blob/issue932/src/main/java/com/jcabi/github/RtGithub.java@L107

[cezarykluczynski]

@pecko You changes give the ability to dynamically specify Jcabi version, build, and date using properties file, but UA string would still be built using a fixed template.

What i meant instead was to give the ability to specify entire UA string.

The logic behind this is that GitHub would probably like to have to ability to identify an application based on UA, and not the library used by application. It more likely that the abusive or other invalid behaviour lays within the application itself, not the library.

If your PR gets merged, I would probably follow with a PR based on your work, to allow totally custom UA strings too.

[pecko]

@cezarykluczynski
It is already custom. You can add the following jcabi.properties:

JCabi-Version = Cezary Kluczyński & Friends
JCabi-Build = 
JCabi-Date = 

or what ever you want.
However, there is better way. I just have solved the issue in Android or similar framework that rebuilds dependencies.

[cezarykluczynski]

@pecko Wouldn't the resulting string be "jcabi-github Cezary Kluczyński & Friends "?

I'm looking at pecko@14dbcb0#diff-bba83786024a91e3e967174c102169e8R111

I'm thinking of something like this:

if (valid) {
    if (prop.getProperty("JCabi-User-Agent-String")) {
        USER_AGENT = prop.getProperty("JCabi-User-Agent-String");
    } else {
        USER_AGENT = JCABI_GITHUB + BLANK
            + prop.getProperty("JCabi-Version") + BLANK
            + prop.getProperty("JCabi-Build") + BLANK
            + prop.getProperty("JCabi-Date");
    }
} else {
    USER_AGENT = JCABI_GITHUB + BLANK
        + "? "
        + new Date().toString();
}

[pecko]

@cezarykluczynski

You are right. What you mean that is better:

• I can add a new commit (the JCabi_Client property, for example)
  or
• you solve it when (if) this PR be applied

[cezarykluczynski]

@pecko It would be great if you could add those changes to your PR. I would have create by PR based on yours, so there's no point in waiting.

[pecko]

@cezarykluczynski
Will do it asap

[pecko]

@cezarykluczynski

I have added pecko@4ecb4ba

It still uses more properties (client, build number, version and date), but you can use only one.

[cezarykluczynski]

@pecko Thank you.

Joining string with null value can be tricky, so I would tweak it further, to both avoid converting null values to "null" string, and to avoid having multiple or untrimmed spaces:

    private buildUserAgentFromProperties(Properties properties) {
        ArrayList<String> keys = new ArrayList<String>() {{
            add("JCabi-User-Agent")
            add("JCabi-Version")
            add("JCabi-Build")
            add("JCabi-Date")
        }};

        String ua = "";

        for(String key : keys) {
            String value = properties.getProperty(key);
            if (null != value) {
                ua += BLANK + value;
            }
        }

        return ua.length() ? ua.substring(1) : ua;
    }

And then call it like this:

    if (value) {
        USER_AGENT = buildUserAgentFromProperties(prop);
    }

[pecko]

The PR is ready to merge.
I have changed slightly your change (remove the method because it should be static and fix a trivial compile errors), but not sure it is too late to add commit to the PR because it would be merge.
Maybe we should enter a new issue after committing or make these change in this issue.

[cezarykluczynski]

@pecko OK, when #1162 is merged, I will create a separate PR.

[pecko]

@cezarykluczynski
This diff adds the qulice checks (hope it helps):

diff --git a/src/main/java/com/jcabi/github/RtGithub.java b/src/main/java/com/jcabi/github/RtGithub.java
index 80bf147..78190f8 100644
--- a/src/main/java/com/jcabi/github/RtGithub.java
+++ b/src/main/java/com/jcabi/github/RtGithub.java
@@ -35,7 +35,9 @@ import com.jcabi.http.Request;
 import com.jcabi.http.request.ApacheRequest;
 import com.jcabi.http.response.JsonResponse;
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Date;
+import java.util.List;
 import java.util.Properties;
 import javax.json.JsonObject;
 import javax.validation.constraints.NotNull;
@@ -109,6 +111,7 @@ public final class RtGithub implements Github {
      */
     private final transient Request request;

+    // @checkstyle ExecutableStatementCountCheck (30 lines)
     static {
         final Properties prop = new Properties();
         boolean valid;
@@ -121,10 +124,26 @@ public final class RtGithub implements Github {
             valid = false;
         }
         if (valid) {
-            USER_AGENT = prop.getProperty(JCABI_CLIENT) + BLANK
-                    + prop.getProperty("JCabi-Version") + BLANK
-                    + prop.getProperty("JCabi-Build") + BLANK
-                    + prop.getProperty("JCabi-Date");
+            final List<String> keys = new ArrayList<String>(4);
+            keys.add(JCABI_CLIENT);
+            keys.add("JCabi-Version");
+            keys.add("JCabi-Build");
+            keys.add("JCabi-Date");
+            final StringBuilder buffer = new StringBuilder();
+            boolean first = true;
+            for (final String key : keys) {
+                final String value = prop.getProperty(key);
+                if (null != value) {
+                    // @checkstyle NestedIfDepthCheck (2 lines)
+                    if (first) {
+                        first = false;
+                    } else {
+                        buffer.append(BLANK);
+                    }
+                    buffer.append(value);
+                }
+            }
+            USER_AGENT = buffer.toString();
         } else {
             USER_AGENT = JCABI_GITHUB + BLANK
                 + "? "

[dmarkov]

@yegor256 please dispatch this issue

[yegor256]

@cezarykluczynski thanks for the ticket, I totally understand that problem. But you can solve it without any modifications to the library:

Github github = new RtGithub(
  new RtGithub().entry()
    .reset("User-Agent")
    .header("User-Agent", "something else")
)

Make sense?

[cezarykluczynski]

@yegor256 This doesn't do the trick.

Whenever BaseRequest.reset() or BaseRequest.header() is called, a new instance of BaseRequest is created with the updated data, and this new instance is no longed bound to RtGithub.REQUEST.

[yegor256]

@cezarykluczynski why should it be "bound" to RtGithub.REQUEST? That's right, a new version of Request is created - that's how it should be. When you instantiate RtGithub, it contains a default instance of Request. Then, you need to create another instance of RtGithub, with a new instance of request, which has a different HTTP header. That's exactly what my example is doing.

[cezarykluczynski]

@yegor256 Sorry for the confusion. That's right, it's working fine.

It seems a little excesive to create two instances, but it's works fine.

[yegor256]

@cezarykluczynski please close the ticket if it's solved, thanks for using the lib!