Skip to content
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

Eagerly throw on null request parameters #1920

Merged
merged 4 commits into from
Apr 28, 2023
Merged

Eagerly throw on null request parameters #1920

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
01c2504
Eagerly throw on null request parameters
westinrm Apr 24, 2023
fcff28e
Add generated changelog entries
svc-changelog Apr 24, 2023
cbb979e
Use SafeNullPointerException
westinrm Apr 24, 2023
634b2d9
Don't check collections
westinrm Apr 25, 2023
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
8 changes: 8 additions & 0 deletions changelog/@unreleased/pr-1920.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
type: improvement
improvement:
description: Eagerly throw on null request parameters (header names, query parameter
names and values, path parameter names and values) to provide helpful stack traces
into user code when null values are inserted into the Request builder instead
of stack traces that just point to Dialogue's async threads.
links:
- https://github.com/palantir/dialogue/pull/1920
38 changes: 38 additions & 0 deletions dialogue-target/src/main/java/com/palantir/dialogue/Request.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.google.common.collect.Multimap;
import com.google.common.collect.Multimaps;
import com.palantir.logsafe.Preconditions;
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.LinkedHashMap;
Expand Down Expand Up @@ -187,6 +188,7 @@ public Request.Builder putHeaderParams(String key, String... values) {
}

public Request.Builder putHeaderParams(String key, String value) {
Preconditions.checkArgumentNotNull(key, "Header name must not be null");
schlosna marked this conversation as resolved.
Show resolved Hide resolved
mutableHeaderParams().put(key, value);
return this;
}
Expand All @@ -197,26 +199,38 @@ public Request.Builder headerParams(Multimap<String, ? extends String> entries)
}

public Request.Builder putAllHeaderParams(String key, Iterable<String> values) {
Preconditions.checkArgumentNotNull(key, "Header name must not be null");
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's match conjure builder semantics where we Preconditions.checkNotNull individual elements (like this one), but we don't iterate through collections (like putAllHeaderParams(Multimap<String, ? extends String> entries) below)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious how your bug came about, I would have expected the recent nullaway rollout to catch that sort of thing before hitting prod. Perhaps there's a case we're missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still have no idea how my bug came about, which is part of why I want to make this change.
I'm okay trying just checking non-collections for now, though I doubt there are ever enough query/path/header parameters that this will actually be a notable performance hit.

mutableHeaderParams().putAll(key, values);
return this;
}

public Request.Builder putAllHeaderParams(Multimap<String, ? extends String> entries) {
if (entries.containsKey(null)) {
throw new SafeIllegalArgumentException("Header name must not be null");
}
mutableHeaderParams().putAll(entries);
return this;
}

public Request.Builder putQueryParams(String key, String... values) {
Preconditions.checkArgumentNotNull(key, "Query parameter name must not be null");
for (String value : values) {
Preconditions.checkArgumentNotNull(value, "Query parameter value must not be null");
}
mutableQueryParams().putAll(key, Arrays.asList(values));
return this;
}

public Request.Builder putQueryParams(String key, String value) {
Preconditions.checkArgumentNotNull(key, "Query parameter name must not be null");
Preconditions.checkArgumentNotNull(value, "Query parameter value must not be null");
mutableQueryParams().put(key, value);
return this;
}

public Request.Builder putQueryParams(Map.Entry<String, ? extends String> entry) {
Preconditions.checkArgumentNotNull(entry.getKey(), "Query parameter name must not be null");
Preconditions.checkArgumentNotNull(entry.getValue(), "Query parameter value must not be null");
mutableQueryParams().put(entry.getKey(), entry.getValue());
return this;
}
Expand All @@ -227,21 +241,33 @@ public Request.Builder queryParams(Multimap<String, ? extends String> entries) {
}

public Request.Builder putAllQueryParams(String key, Iterable<String> values) {
Preconditions.checkArgumentNotNull(key, "Query parameter name must not be null");
for (String value : values) {
Preconditions.checkArgumentNotNull(value, "Query parameter value must not be null");
}
mutableQueryParams().putAll(key, values);
return this;
}

public Request.Builder putAllQueryParams(Multimap<String, ? extends String> entries) {
entries.forEach((key, value) -> {
Preconditions.checkArgumentNotNull(key, "Query parameter name must not be null");
Preconditions.checkArgumentNotNull(value, "Query parameter value must not be null");
});
mutableQueryParams().putAll(entries);
return this;
}

public Request.Builder putPathParams(String key, String value) {
Preconditions.checkArgumentNotNull(key, "Path parameter name must not be null");
Preconditions.checkArgumentNotNull(value, "Path parameter value must not be null");
mutablePathParams().put(key, value);
return this;
}

public Request.Builder putPathParams(Map.Entry<String, ? extends String> entry) {
Preconditions.checkArgumentNotNull(entry.getKey(), "Path parameter name must not be null");
Preconditions.checkArgumentNotNull(entry.getValue(), "Path parameter value must not be null");
mutablePathParams().put(entry.getKey(), entry.getValue());
return this;
}
Expand All @@ -252,16 +278,28 @@ public Request.Builder pathParams(Map<String, ? extends String> entries) {
}

public Request.Builder putAllPathParams(String key, Iterable<String> values) {
Preconditions.checkArgumentNotNull(key, "Path parameter name must not be null");
for (String value : values) {
Preconditions.checkArgumentNotNull(value, "Path parameter value must not be null");
}
mutablePathParams().putAll(key, values);
return this;
}

public Request.Builder putAllPathParams(Map<String, ? extends String> entries) {
entries.forEach((key, value) -> {
Preconditions.checkArgumentNotNull(key, "Path parameter name must not be null");
Preconditions.checkArgumentNotNull(value, "Path parameter value must not be null");
});
entries.forEach(mutablePathParams()::put);
return this;
}

public Request.Builder putAllPathParams(Multimap<String, ? extends String> entries) {
entries.forEach((key, value) -> {
Preconditions.checkArgumentNotNull(key, "Path parameter name must not be null");
Preconditions.checkArgumentNotNull(value, "Path parameter value must not be null");
});
mutablePathParams().putAll(entries);
return this;
}
Expand Down