Skip to content

Commit

Permalink
Accept no-op null arguments in ScopedLoggingContext.Builder.
Browse files Browse the repository at this point in the history 
All Flogger fluent method calls accept no-op arguments to permit conditionally ignoring a method call without splitting the method chain. While the builder can be assigned to a local variable to conditionally apply methods, this is inconsistent with other usage of Flogger, and discouraged by javadoc.

https://google.github.io/flogger/best_practice.html#no-split

RELNOTES=Accept no-op null arguments in ScopedLoggingContext.Builder.
PiperOrigin-RevId: 557923868
  • Loading branch information
@java-team-github-bot
java-team-github-bot authored and Flogger Team committed Aug 17, 2023
1 parent 0e3818b commit 5aa0649
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 13 deletions.
32 changes: 19 additions & 13 deletions api/src/main/java/com/google/common/flogger/context/ScopedLoggingContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,16 @@ public static final class ScopeList {
* the type already exists in the list, the original (potentially {@code null}) list reference
* is returned.
*/
@NullableDecl public static ScopeList addScope(
@NullableDecl ScopeList list, @NullableDecl ScopeType type) {
@NullableDecl
public static ScopeList addScope(@NullableDecl ScopeList list, @NullableDecl ScopeType type) {
return (type != null && lookup(list, type) == null)
? new ScopeList(type, type.newScope(), list)
: list;
}

/** Finds a scope instance for the given type in a possibly null scope list. */
@NullableDecl public static LoggingScope lookup(@NullableDecl ScopeList list, ScopeType type) {
@NullableDecl
public static LoggingScope lookup(@NullableDecl ScopeList list, ScopeType type) {
while (list != null) {
if (type.equals(list.key)) {
return list.scope;
Expand Down Expand Up @@ -141,22 +142,26 @@ protected Builder() {}

/**
* Sets the tags to be used with the context. This method can be called at most once per
* builder.
* builder. Calling with a null value does nothing.
*/
@CanIgnoreReturnValue
public final Builder withTags(Tags tags) {
public final Builder withTags(@NullableDecl Tags tags) {
checkState(this.tags == null, "tags already set");
checkNotNull(tags, "tags");
this.tags = tags;
if (tags != null) {
this.tags = tags;
}
return this;
}

/**
* Adds a single metadata key/value pair to the context. This method can be called multiple
* times on a builder.
* times on a builder. Calling with a null value does nothing.
*/
@CanIgnoreReturnValue
public final <T> Builder withMetadata(MetadataKey<T> key, T value) {
public final <T> Builder withMetadata(MetadataKey<T> key, @NullableDecl T value) {
if (value == null) {
return this;
}
if (metadata == null) {
metadata = ContextMetadata.builder();
}
Expand All @@ -166,13 +171,14 @@ public final <T> Builder withMetadata(MetadataKey<T> key, T value) {

/**
* Sets the log level map to be used with the context being built. This method can be called at
* most once per builder.
* most once per builder. Calling with a null value does nothing.
*/
@CanIgnoreReturnValue
public final Builder withLogLevelMap(LogLevelMap logLevelMap) {
public final Builder withLogLevelMap(@NullableDecl LogLevelMap logLevelMap) {
checkState(this.logLevelMap == null, "log level map already set");
checkNotNull(logLevelMap, "log level map");
this.logLevelMap = logLevelMap;
if (logLevelMap != null) {
this.logLevelMap = logLevelMap;
}
return this;
}

Expand Down
45 changes: 45 additions & 0 deletions api/src/test/java/com/google/common/flogger/testing/AbstractScopedLoggingContextTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,21 @@ public void testLoggedTags_areMerged() {
backend.assertLogged(0).metadata().containsUniqueEntry(TAGS, expected);
}

@Test
public void testNewContext_withNullTags_ignored() {
assertThat(getTagMap()).isEmpty();
context
.newContext()
.withTags(null)
.run(
() -> {
assertThat(getTagMap()).isEmpty();
markTestAsDone();
});
assertThat(getTagMap()).isEmpty();
checkDone();
}

@Test
public void testNewContext_withMetadata() {
assertThat(getMetadata()).hasSize(0);
Expand All @@ -144,6 +159,21 @@ public void testNewContext_withMetadata() {
checkDone();
}

@Test
public void testNewContext_withNullMetadata_ignored() {
assertThat(getMetadata()).hasSize(0);
context
.newContext()
.withMetadata(FOO_KEY, null)
.run(
() -> {
assertThat(getMetadata()).hasSize(0);
markTestAsDone();
});
assertThat(getMetadata()).hasSize(0);
checkDone();
}

@Test
public void testNewContext_withLogLevelMap() {
assertLogging("foo.bar.Bar", Level.FINE).isFalse();
Expand All @@ -160,6 +190,21 @@ public void testNewContext_withLogLevelMap() {
checkDone();
}

@Test
public void testNewContext_withNullLogLevelMap_ignored() {
assertLogging("foo.bar.Bar", Level.FINE).isFalse();
context
.newContext()
.withLogLevelMap(null)
.run(
() -> {
assertLogging("foo.bar.Bar", Level.FINE).isFalse();
markTestAsDone();
});
assertLogging("foo.bar.Bar", Level.FINE).isFalse();
checkDone();
}

@Test
public void testNewContext_withMergedTags() {
assertThat(getTagMap()).isEmpty();
Expand Down

1 comment on commit 5aa0649

@hagbard
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A regards https://google.github.io/flogger/best_practice.html#no-split

The idea of not needing to split fluent APIs does not mean they should accept null arguments if there is a perfectly good "no-op" instance that could have been passed.
For example, withStackTrace() accepts the NONE enum and rejects null, and for Tags, the Tags.empty() singleton instance is always available to be passed.

This also isn't the logging API, and the "no split" advice was written primarily for that (though it is probably good advice in general).
When making contexts I felt that there was less likelihood a user would be conditionally adding metadata values (if you are creating a context with no values, it's better to just not create the context). E.g. I was imagining that users will do:

  if (hasMetadataToAdd) {
    ScopedLoggingContexts.newContext().withMetadata(key, valueToAdd).run(() -> doThing());
  } else {
    // This does not incur the cost of processing contexts for each log statement.
    doThing();
  }

I'd be interested to know if this change came from a real situation in which a user needed the ability to provide no-op values, and which values these were?

Please sign in to comment.