Refactor: Stop Splitting Header Values

[Johannes-Schneider]

Context

SAP/cloud-sdk-java-backlog#156.
SAP/cloud-sdk-java-backlog/discussions/343

This PR changes our RequestHeaderContainer and the ODataRequestResult to no longer split header values.

Feature scope:

• Adjust RequestHeaderContainer
  • JavaDoc
  • Implementation (DefaultRequestHeaderContainer)
• Adjust ODataRequestResult

Definition of Done

• Functionality scope stated & covered
• Tests cover the scope above
• Error handling created / updated & covered by the tests above
• Documentation updated → refactor: stop splitting header values in RequestHeaderContainer cloud-sdk#1590
• Release notes updated

[newtork]

(Comment)

I'm wondering whether it'd be worth it investigating how Spring/Apache/JDK11 HttpClient is handling white spaces in response header values-

[Johannes-Schneider]

TL;DR

• Spring does NOT process header values in any way
• Apache DOES trim header values

Spring Header Handling

public class HttpHeadersLearningTest {
    @Test
    public void testGetSimple() {
        final HttpHeaders sut = new HttpHeaders();
        sut.add("name", "value1, value2");

        assertThat(sut.get("name")).containsExactlyInAnyOrder("value1, value2");
    }

    @Test
    public void testAddEmptyName() {
        final HttpHeaders sut = new HttpHeaders();
        sut.add("", "value");

        assertThat(sut.keySet()).containsExactlyInAnyOrder("");
    }

    @Test
    public void testGetValueWithTrailingSpaces() {
        final HttpHeaders sut = new HttpHeaders();
        sut.add("name", "  value\t");

        assertThat(sut.get("name")).containsExactlyInAnyOrder("  value\t");
    }

    @Test
    public void testGetValueIgnoresKeyCasing() {
        final HttpHeaders sut = new HttpHeaders();
        sut.add("name", "value");

        assertThat(sut.get("name")).containsExactlyInAnyOrder("value");
        assertThat(sut.get("NAME")).containsExactlyInAnyOrder("value");
    }
}

Apache Header Handling

public class ApacheHeaderLearningTest {
    @Test
    public void testGetElementsSimple() {
        final BasicHeader sut = new BasicHeader("name", "value1, value2");

        assertThat(Arrays.stream(sut.getElements()).map(HeaderElement::getName))
                .containsExactlyInAnyOrder("value1", "value2");
    }

    @Test
    public void testGetElementsCookies() {
        final BasicHeader sut = new BasicHeader("Set-Cookie", "cookie1; cookie2");

        assertThat(sut.getElements())
                .containsExactly(
                        new BasicHeaderElement(
                                "cookie1",
                                null,
                                new NameValuePair[]{new BasicNameValuePair("cookie2", null)}));
    }

    @Test
    public void testGetElementsQuoted() {
        final BasicHeader sut = new BasicHeader("name", "\"value1, value2\", value3");

        assertThat(Arrays.stream(sut.getElements()).map(HeaderElement::getName))
                .containsExactlyInAnyOrder("\"value1", "value2\"", "value3");
    }

    @Test
    public void testGetElementsSingleQuoted() {
        final BasicHeader sut = new BasicHeader("name", "'value1, value2', value3");

        assertThat(Arrays.stream(sut.getElements()).map(HeaderElement::getName))
                .containsExactlyInAnyOrder("'value1", "value2'", "value3");
    }

    @Test
    public void testGetElementsWithEmptyEntry() {
        final BasicHeader sut = new BasicHeader("name", "value1, , value3");

        assertThat(Arrays.stream(sut.getElements()).map(HeaderElement::getName)).containsExactly("value1", "value3");
    }

    @Test
    public void testGetElementsWithTrailingSpaces() {
        final BasicHeader sut = new BasicHeader("name", "  value\t");

        assertThat(Arrays.stream(sut.getElements()).map(HeaderElement::getName)).containsExactly("value");
    }
}

[newtork]

(Minor)

Feels like this method can be nicely reduced by just using implementation BasicHttpResponse with respective string-based header-methods.

[newtork]

Only minor comments, otherwise LGTM

[cschubertcs]

lgtm!
When we offer the same public API we should also have a look the concrete implementation. Best to use the same kind of Map (ImmutableListMultimap with "normalized" keys vs. TreeMap with String::compareToIgnoreCase). But for the moment that should result in the same behavior.

[newtork]

(Question)

Why final?

[Johannes-Schneider]

As discussed, the constructor is private so the class cannot be inherited (in productive code).

For mocking (i.e. during tests), I'd expect customers to either mock the interface (RequestHeaderContainer) or to use an actual (i.e. non-mocked) instance of the implementation (DefaultRequestHeaderContainer).
Therefore, I'd argue this change to be fine.

---

Besides: This guide seems to hint that mocking final classes is not a problem at all.

[newtork]

LGTM

Very minor, map implementation choice as Chris mentioned. Minor code quality.