Eagerly throw on null request parameters #1920

westinrm · 2023-04-24T17:08:56Z

Before this PR

Null header names, query parameter names/values, and path parameter names/values will throw exceptions during request execution or URL rendering respectively without any useful stack trace, making debugging the root cause of these issues difficult.

For example, this is the error when a null query parameter value is used:

java.lang.NullPointerException: Cannot invoke "String.getBytes(java.nio.charset.Charset)" because "source" is null
    at com.palantir.dialogue.core.BaseUrl$UrlEncoder.encode(BaseUrl.java:279)
    at com.palantir.dialogue.core.BaseUrl$UrlEncoder.encodeQueryNameOrValue(BaseUrl.java:271)
    at com.palantir.dialogue.core.BaseUrl$DefaultUrlBuilder.queryParam(BaseUrl.java:164)
    ...
    at com.palantir.dialogue.core.BaseUrl.render(BaseUrl.java:58)
    at com.palantir.dialogue.hc5.ApacheHttpClientBlockingChannel.execute(ApacheHttpClientBlockingChannel.java:99)
    ...

After this PR

==COMMIT_MSG==
Eagerly throw on null request parameters

By throwing when the null values are inserted into the Request builder it's possible to get a helpful stack trace instead of one that just traces back into one of Dialogue's async threads.
==COMMIT_MSG==

Possible downsides?

Slightly worse performance due to having to check all entries inserted against null

Null header names, query parameter names/values, and path parameter names/values will throw exceptions during request execution or URL rendering respectively without any useful stack trace, making debugging the root cause of these issues difficult. By throwing when the null values are inserted into the Request builder it's possible to get a more accurate stack trace.

changelog-app · 2023-04-24T17:09:04Z

Generate changelog in `changelog/@unreleased`

Type

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.

Check the box to generate changelog(s)

Generate changelog entry

dialogue-target/src/main/java/com/palantir/dialogue/Request.java

carterkozak · 2023-04-24T17:28:18Z

dialogue-target/src/main/java/com/palantir/dialogue/Request.java

@@ -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");


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)

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?

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.

carterkozak · 2023-04-28T18:22:26Z

dialogue-target/src/main/java/com/palantir/dialogue/Request.java

            mutableQueryParams().putAll(key, Arrays.asList(values));
            return this;
        }

        public Request.Builder putQueryParams(String key, String value) {
+            Preconditions.checkNotNull(key, "Query parameter name must not be null");
+            Preconditions.checkNotNull(value, "Query parameter value must not be null");


Why do we check key+value here, but only key in public Request.Builder putHeaderParams(String key, String value) {?

headers actually permit null values from what I could determine, while query and path parameters don't

if null headers don't result in failures, I'd consider that a bug, but reasonable not to make it throw in this change 👍

svc-autorelease · 2023-04-28T18:37:29Z

Released 3.83.0

probot-autolabeler bot added the autorelease label Apr 24, 2023

Add generated changelog entries

fcff28e

pkoenig10 reviewed Apr 24, 2023

View reviewed changes

dialogue-target/src/main/java/com/palantir/dialogue/Request.java Outdated Show resolved Hide resolved

carterkozak reviewed Apr 24, 2023

View reviewed changes

westinrm added 2 commits April 24, 2023 11:30

Use SafeNullPointerException

cbb979e

Don't check collections

634b2d9

westinrm added the merge when ready label Apr 26, 2023

carterkozak reviewed Apr 28, 2023

View reviewed changes

carterkozak approved these changes Apr 28, 2023

View reviewed changes

bulldozer-bot bot merged commit 9c78ef0 into develop Apr 28, 2023

bulldozer-bot bot deleted the westinm/eagerly-throw-on-null branch April 28, 2023 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eagerly throw on null request parameters #1920

Eagerly throw on null request parameters #1920

westinrm commented Apr 24, 2023 •

edited

Loading

changelog-app bot commented Apr 24, 2023 •

edited by westinrm

Loading

carterkozak Apr 24, 2023

carterkozak Apr 24, 2023

westinrm Apr 24, 2023

carterkozak Apr 28, 2023

westinrm Apr 28, 2023

carterkozak Apr 28, 2023

svc-autorelease commented Apr 28, 2023

Eagerly throw on null request parameters #1920

Eagerly throw on null request parameters #1920

Conversation

westinrm commented Apr 24, 2023 • edited Loading

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Apr 24, 2023 • edited by westinrm Loading

Generate changelog in changelog/@unreleased

carterkozak Apr 24, 2023

Choose a reason for hiding this comment

carterkozak Apr 24, 2023

Choose a reason for hiding this comment

westinrm Apr 24, 2023

Choose a reason for hiding this comment

carterkozak Apr 28, 2023

Choose a reason for hiding this comment

westinrm Apr 28, 2023

Choose a reason for hiding this comment

carterkozak Apr 28, 2023

Choose a reason for hiding this comment

svc-autorelease commented Apr 28, 2023

westinrm commented Apr 24, 2023 •

edited

Loading

changelog-app bot commented Apr 24, 2023 •

edited by westinrm

Loading

Generate changelog in `changelog/@unreleased`