feat: add support for agent cache

[winstxnhdw]

Closes #142.

• Add support for agent cache
• Applied some lint suggestions
• Add tests for agent cache

[marcin-krystianc]

Hi @winstxnhdw,

Thanks for your contribution. I see that you included multiple, unrelated changes into the PR. I think it is better to keep the scope of this PR just to caching mechanism and add those other features in next PRs.

[marcin-krystianc]

From reading about [Background refresh Caching]{https://developer.hashicorp.com/consul/api-docs/features/caching#background-refresh-caching} I conclude that the Agent_UseCache seems to be sufficient and a dedicated test for it is not necessary.

[marcin-krystianc]

I'm not sure about exposing the HttpResponseHeaders in the response object. It changes the contract of this class dramatically, and the HttpResponseHeaders class is very implementation specific. I think it is better to use simpler types and add two extra fields for X-Cache and Age headers.
@mfkl @jgiannuzzi Any thoughts?

[winstxnhdw]

It changes the contract of this class dramatically, and the HttpResponseHeaders class is very implementation specific.

From what the intellisense tells me, the base request method always returns a HttpResponseMessage which always contains a Headers property with HttpResponseHeaders. Exposing this property will also allow users full access to the raw Headers where they would not have before.

In fact I am even of the opinion to keep the entire original response in a RawResponse property because IMO, as a library, I'd want to provide the user an easier experience when interfacing with the Consul API, while not blocking one of the valid ways a user may want to inspect the original response.

[marcin-krystianc]

I agree that it gives the user much flexibility. So I understand your motivation, but I believe that exposing the HttpResponseHeaders would be a bad API design practice.
It is an implementation-specific class. Once we would expose it we would never be able to change our implementation. I'm not saying that we are ever going to do that, but I'm saying that it is going to strongly couple implementation and the API together which is a bad practice.
The number of different HTTP headers that are going to be used by the consul is very limited, so we can parse them in our library for the user. If the number of possible headers was very high, we would need more flexibility as you suggest.

[winstxnhdw]

I see, that makes a lot of sense.

Okay, in that case, we can declare it as a private property and retrieve the value from our test using reflection?

[marcin-krystianc]

I see, that makes a lot of sense.

Okay, in that case, we can declare it as a private property and retrieve the value from our test using reflection?

I don't think we need reflection, I think the best way to do this is by adding two new extra public properties (XCache and Age) in the QueryResult class. Then update the ParseQueryHeaders method to parse those headers. It will make it possible to implement the test, but also it will make it possible for the end user to know whether the result comes from the cache.

[winstxnhdw]

I am wondering if we need to update the following constructor overload for QueryResult.

public QueryResult(QueryResult other) : base(other)
{
    LastIndex = other.LastIndex;
    LastContact = other.LastContact;
    KnownLeader = other.KnownLeader;
}

to

public QueryResult(QueryResult other) : base(other)
{
    LastIndex = other.LastIndex;
    LastContact = other.LastContact;
    KnownLeader = other.KnownLeader;
    AddressTranslationEnabled = other.AddressTranslationEnabled;
    XCache = other.XCache;
    Age = Age;
}

[marcin-krystianc]

Generally, it is good to be merged now. I just noticed that we do unnecessary validation of arguments, It neither adds any value nor does much harm - but I still would remove it.

[marcin-krystianc]

Looks good 👍 Thank you for your patience!