HTTPCLIENT-2343 - Enhance HttpClientContext user token management for improved compatibility

[arturobernalg]

This PR refines the way user tokens are managed within HttpClientContext. Specifically, the getUserToken() method has been enhanced to first check the internal userToken field, and only fall back to context attributes for backward compatibility. A warning log has been added to notify developers when this fallback occurs, strongly encouraging the preferred usage of setUserToken(Object) for setting user tokens.

[ok2c]

@arturobernalg I am sorry but this needs to be done differently. Context attributes are mostly "written seldom / read often". Adding a map lookup to #getUserToken is wrong and adds unnecessary overhead. Override #setAttribute instead and update context instance variables along with the map.

[arturobernalg]

setAttribute

Hi @ok2c
When ctxA.setAttribute(HttpClientContext.USER_TOKEN, "custom state") is called directly on an instance of HttpClientContext, it bypasses the overridden setAttribute method in the Delegate class and instead invokes HttpCoreContext#setAttribute. As a result, this does not synchronize with the internal userToken field as intended.

[ok2c]

@arturobernalg So what? HttpClientContext.Delegate is being in extreme cases when caller uses incorrect type of context to start with.
Alternatively we should close HTTPCLIENT-2343 as WONTFIX.

[ok2c]

@arturobernalg If both test cases pass, it would be good enough.

@SuppressWarnings("deprecation")
class TestHttpClientContext {

    @Test
    void testDeprecatedSetAttributes() {
        final HttpClientContext context1 = HttpClientContext.cast(new BasicHttpContext());
        context1.setAttribute(HttpClientContext.USER_TOKEN, "blah");
        Assertions.assertEquals("blah", context1.getUserToken());

        final HttpClientContext context2 = HttpClientContext.create();
        context2.setAttribute(HttpClientContext.USER_TOKEN, "blah");
        Assertions.assertEquals("blah", context2.getUserToken());
    }

}

[arturobernalg]

SuppressWarnings("deprecation")
class TestHttpClientContext {

@Test
void testDeprecatedSetAttributes() {
    final HttpClientContext context1 = HttpClientContext.cast(new BasicHttpContext());
    context1.setAttribute(HttpClientContext.USER_TOKEN, "blah");
    Assertions.assertEquals("blah", context1.getUserToken());

    final HttpClientContext context2 = HttpClientContext.create();
    context2.setAttribute(HttpClientContext.USER_TOKEN, "blah");
    Assertions.assertEquals("blah", context2.getUserToken());
}

@ok2c both test pass correctly.

[ok2c]

@ok2c both test pass correctly.

@arturobernalg Right, but you are still doing a map lookup in #getUserToken, don't you? The whole point of context optimization was to eliminate map lookups for frequently used attributes.

[arturobernalg]

@ok2c both test pass correctly.

@arturobernalg Right, but you are still doing a map lookup in #getUserToken, don't you? The whole point of context optimization was to eliminate map lookups for frequently used attributes.

@ok2c The test failed, confirming that the current approach doesn't resolve the issue effectively.

[arturobernalg]

@ok2c both test pass correctly.

@arturobernalg Right, but you are still doing a map lookup in #getUserToken, don't you? The whole point of context optimization was to eliminate map lookups for frequently used attributes.

@ok2c The test failed, confirming that the current approach doesn't resolve the issue effectively.

@ok2c Given the test results It might be best to consider closing the ticket as "WONTFIX" since the optimization goals aren't met with the current implementation.

[ok2c]

@arturobernalg My idea was to move the compensatory code to #setAttribute, which should be less expensive in terms of performance overhead, and as long as the said two tests pass, it would be good enough. Having said that I am fine with WONTFIX as well.