Skip to content

Conjure User-Agents supports arbitrary comment data #1286

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

Merged
merged 5 commits into from
Feb 4, 2025
Merged

Conjure User-Agents supports arbitrary comment data #1286

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f71923c
Conjure User-Agents supports arbitrary comment data
Jan 30, 2025
06e730a
Move comment to Agent
Jan 31, 2025
c22647f
experiment with allowed commas
Jan 31, 2025
bf52874
Add generated changelog entries
svc-changelog Feb 3, 2025
688a72b
annotations and javadoc
Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package com.palantir.conjure.java.api.config.service;

import com.palantir.logsafe.Preconditions;
import com.palantir.logsafe.SafeArg;
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
import java.util.List;
Expand All @@ -33,7 +32,17 @@
public interface UserAgent {

/** Identifies the node (e.g., IP address, container identifier, etc) on which this user agent was constructed. */
Optional<String> nodeId();
@Value.Lazy
default Optional<String> nodeId() {
if (comments().isEmpty()) {
// fast path to avoid stream overhead
return Optional.empty();
}
return comments().stream()
.filter(item -> item.startsWith("nodeId:"))
.map(item -> item.substring(7).trim())
.findFirst();
}

/** The primary user agent, typically the name/version of the service initiating an RPC call. */
Agent primary();
Expand All @@ -44,9 +53,14 @@ public interface UserAgent {
*/
List<Agent> informational();

List<String> comments();

/** Creates a new {@link UserAgent} with the given {@link #primary} agent and originating node id. */
static UserAgent of(Agent agent, String nodeId) {
return ImmutableUserAgent.builder().nodeId(nodeId).primary(agent).build();
return ImmutableUserAgent.builder()
.addComments("nodeId:" + nodeId)
.primary(agent)
.build();
}

/**
Expand Down Expand Up @@ -75,13 +89,17 @@ default UserAgent addAgent(Agent agent) {
return ImmutableUserAgent.builder().from(this).addInformational(agent).build();
}

default UserAgent addComment(String comment) {
UserAgents.checkComment(comment);
return ImmutableUserAgent.builder().from(this).addComments(comment).build();
}

@Value.Check
default void check() {
if (nodeId().isPresent()) {
Preconditions.checkArgument(
UserAgents.isValidNodeId(nodeId().get()),
"Illegal node id format",
SafeArg.of("nodeId", nodeId().get()));
if (!UserAgents.isValidNodeId(nodeId().get())) {
throw new SafeIllegalArgumentException("Illegal node id format", SafeArg.of("nodeId", nodeId().get()));
}
}
}

Expand Down
73 changes: 56 additions & 17 deletions service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
import com.palantir.logsafe.logger.SafeLogger;
import com.palantir.logsafe.logger.SafeLoggerFactory;
import java.util.HashMap;
import java.util.Map;
import java.util.List;
import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
Expand All @@ -47,15 +46,28 @@ public final class UserAgents {
Pattern.compile(String.format("(%s)/(%s)( \\((.+?)\\))?", NAME_REGEX, LENIENT_VERSION_REGEX));
private static final Splitter COMMA_OR_SEMICOLON_SPLITTER =
Splitter.on(CharMatcher.anyOf(",;").precomputed()).trimResults().omitEmptyStrings();
private static final CharMatcher COMMENT_VALID_CHARS = CharMatcher.inRange('a', 'z')
.or(CharMatcher.inRange('A', 'Z'))
.or(CharMatcher.inRange('0', '9'))
.or(CharMatcher.anyOf(".-:_/ "))
.precomputed();

private UserAgents() {}

/** Returns the canonical string format for the given {@link UserAgent}. */
public static String format(UserAgent userAgent) {
StringBuilder formatted = new StringBuilder(64); // preallocate larger buffer for longer agents
formatSimpleAgent(userAgent.primary(), formatted);
if (userAgent.nodeId().isPresent()) {
formatted.append(" (nodeId:").append(userAgent.nodeId().get()).append(')');
List<String> comments = userAgent.comments();
if (!comments.isEmpty()) {
formatted.append(" (");
for (int i = 0; i < comments.size(); i++) {
if (i > 0) {
formatted.append("; ");
}
formatted.append(comments.get(i));
}
formatted.append(')');
}
for (UserAgent.Agent informationalAgent : userAgent.informational()) {
formatted.append(' ');
Expand Down Expand Up @@ -146,10 +158,8 @@ private static UserAgent parseInternal(String userAgent, boolean lenient) {
// primary
builder.primary(UserAgent.Agent.of(name, version));
comments.ifPresent(c -> {
Map<String, String> parsedComments = parseComments(c);
if (parsedComments.containsKey("nodeId")) {
builder.nodeId(parsedComments.get("nodeId"));
}
List<String> parsedComments = parseComments(c);
builder.addAllComments(parsedComments);
});
} else {
// informational
Expand Down Expand Up @@ -177,17 +187,46 @@ private static UserAgent parseInternal(String userAgent, boolean lenient) {
return builder.build();
}

private static Map<String, String> parseComments(String commentsString) {
Map<String, String> comments = new HashMap<>();
for (String comment : COMMA_OR_SEMICOLON_SPLITTER.split(commentsString)) {
String[] fields = comment.split(":");
if (fields.length == 2) {
comments.put(fields[0], fields[1]);
} else {
comments.put(comment, comment);
private static List<String> parseComments(String commentsString) {
List<String> results = COMMA_OR_SEMICOLON_SPLITTER.splitToList(commentsString);
for (int i = 0; i < results.size(); ++i) {
// In most cases, all comments will be valid, so we avoid stream overhead.
if (!isValidComment(results.get(i))) {
return results.stream().filter(UserAgents::isValidComment).toList();
}
}
return comments;
return results;
}

static void checkComment(String comment) {
if (comment == null) {
throw new SafeIllegalArgumentException("Comment must not be null");
}
if (comment.isEmpty()) {
throw new SafeIllegalArgumentException("Comment must not be empty");
}
if (comment.startsWith(" ")) {
throw new SafeIllegalArgumentException(
"Comment must not start with whitespace", SafeArg.of("comment", comment));
}
if (comment.endsWith(" ")) {
throw new SafeIllegalArgumentException(
"Comment must not end with whitespace", SafeArg.of("comment", comment));
}
if (!COMMENT_VALID_CHARS.matchesAllOf(comment)) {
throw new SafeIllegalArgumentException(
"Comment contains disallowed characters",
SafeArg.of("allowed", "a-zA-Z0-9.-:_/ "),
SafeArg.of("comment", comment));
}
}

static boolean isValidComment(String comment) {
return comment != null
&& !comment.isEmpty()
&& !comment.startsWith(" ")
&& !comment.endsWith(" ")
&& COMMENT_VALID_CHARS.matchesAllOf(comment);
}

static boolean isValidName(String name) {
Expand Down
40 changes: 34 additions & 6 deletions service-config/src/test/java/com/palantir/conjure/java/api/config/service/UserAgentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static com.palantir.logsafe.testing.Assertions.assertThatLoggableExceptionThrownBy;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
Expand Down Expand Up @@ -119,14 +120,15 @@ public void parse_handlesPrimaryAgent() {
"service/10.20.30",
"service/10.20.30 (nodeId:myNode)",
}) {
assertThat(UserAgents.format(UserAgents.parse(agent)))
.withFailMessage(agent)
.isEqualTo(agent);
assertThat(UserAgents.format(UserAgents.parse(agent))).isEqualTo(agent);
}

// Formatting ignores non-nodeId comments
// Formatting ignores invalid comments
assertThat(UserAgents.format(UserAgents.parse("service/1.2.3 (())"))).isEqualTo("service/1.2.3");

// Formatting retains valid comments
assertThat(UserAgents.format(UserAgents.parse("service/1.2.3 (foo:bar)")))
.isEqualTo("service/1.2.3");
.isEqualTo("service/1.2.3 (foo:bar)");

// Finds primary agent even when there is a prefix
assertThat(UserAgents.format(UserAgents.parse(" service/1.2.3"))).isEqualTo("service/1.2.3");
Expand Down Expand Up @@ -169,7 +171,8 @@ public void parse_handlesInformationalAgents() {
public void parse_canParseBrowserAgent() {
String chrome = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) "
+ "Chrome/61.0.3163.100 Safari/537.36";
String expected = "Mozilla/5.0 AppleWebKit/537.36 Chrome/61.0.3163.100 Safari/537.36";
String expected =
"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 Chrome/61.0.3163.100 Safari/537.36";
assertThat(UserAgents.format(UserAgents.tryParse(chrome))).isEqualTo(expected);
assertThat(UserAgents.format(UserAgents.parse(chrome))).isEqualTo(expected);
}
Expand Down Expand Up @@ -263,6 +266,31 @@ public void invalid_names() {
assertThat("service_name").satisfies(UserAgentTest::isNotValidName);
}

@Test
public void invalid_comment() {
UserAgent agent = UserAgent.of(Agent.of("name", "0.0.0"));
assertThatThrownBy(() -> agent.addComment(";"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Comment contains disallowed characters");
assertThat(UserAgents.isValidComment(";")).isFalse();
assertThatThrownBy(() -> agent.addComment(null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Comment must not be null");
assertThat(UserAgents.isValidComment(null)).isFalse();
assertThatThrownBy(() -> agent.addComment(""))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Comment must not be empty");
assertThat(UserAgents.isValidComment("")).isFalse();
assertThatThrownBy(() -> agent.addComment(" leading whitespace"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Comment must not start with whitespace");
assertThat(UserAgents.isValidComment(" leading whitespace")).isFalse();
assertThatThrownBy(() -> agent.addComment("trailing whitespace "))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Comment must not end with whitespace");
assertThat(UserAgents.isValidComment("trailing whitespace ")).isFalse();
}

private static void isValidName(String name) {
assertThat(UserAgents.isValidName(name)).isTrue();
assertThat(name)
Expand Down