Removing accept header previews

[JonruAlveus]

Making a first stab. Removing everything I think I can, but tests aren't working yet. Need some more advice.

fixes #2497

[JonruAlveus]

This isn't quite ready yet, the tests don't pass. It's more for visibility that it's on the way.

[nickfloyd]

I think instead of passing null, now that previews are gone and will be able to use the default content type / accept headers set by the REST API we should consider creating an overload for

Task<IApiResponse<T>> Get<T>(Uri uri, IDictionary<string, string> parameters);

Then in the implementation we simply pass in null there for the accepts parameter:

     public Task<IApiResponse<T>> Get<T>(Uri uri, IDictionary<string, string> parameters)
        {
            Ensure.ArgumentNotNull(uri, nameof(uri));

            return SendData<T>(uri.ApplyParameters(parameters), HttpMethod.Get, null, null, null, CancellationToken.None);
        }

That way, we give users the ability to use the defaults and the option to pass in headers if they'd like. This aspect of the code could be refactored and allow a collection of headers to be passed in, but that's orthogonal to what you're doing here.

It also cleans up call sites a bit so that we're not passing nulls around until we absolutely have to.

Thoughts?

[JonruAlveus]

I've made some tweaks, can you have another skim over?

[nickfloyd]

@JonruAlveus This is looking good, thanks for putting in the effort here ❤️ . What type of things are you running into? Please let me know how I can help!

[JonruAlveus]

@nickfloyd Thanks, there were three questions (one was the nulls thing)

• What to do with nulls - I'll see about creating some overloads to avoid having to handle nulls
• There are a few remaining headers which are more about content types. Would it be worth merging them into the code a little further down the stack or just leave them as they are?
• I'm not 100% sure how to ensure everything still works - the integration tests are the only ones that will really prove it and I don't really know how many of them worked before these changes (nor how long it would take to run them!). I wonder if we should consider writing a smoke test pack of a few integration tests that prove some of the primary journeys still work without having to test the entire api?

[JonruAlveus]

@nickfloyd Another question - how far down the rabbit hole do we want to go? Removing accept headers entirely? I feel like it would be nice to build something different rather than having accepts headers as overloads. Perhaps we could use a pattern on the connection itself, perhaps Connection.WithHeaders("blah").Get()? That way the Get is simpler.

[nickfloyd]

@JonruAlveus sorry for the delay in getting back to you.

What to do with nulls - I'll see about creating some overloads to avoid having to handle nulls
✅

There are a few remaining headers which are more about content types. Would it be worth merging them into the code a little further down the stack or just leave them as they are?

I'd like to leave those in place. While previews are no longer supported, the intention of accept and content-type headers will always be present in a REST API given things like HATEOAS. We need to be able to support that type of thing - I'd like to leave what is implemented intact and possibly document what is supported with what remains.

I'm not 100% sure how to ensure everything still works - the integration tests are the only ones that will really prove it and I don't really know how many of them worked before these changes (nor how long it would take to run them!). I wonder if we should consider writing a smoke test pack of a few integration tests that prove some of the primary journeys still work without having to test the entire API?

We could write those tests as you describe - I feel like we need a little more robustness around testing anyway. We ended up promoting all REST API previews back on October 2021 - which means that, regardless of the headers we pass, they have been used in this SDK for close to 9 months now. I know it's not as tidy as some smoke or functional tests, but it does prove that things are working as expected.

how far down the rabbit hole do we want to go? Removing accept headers entirely? I feel like it would be nice to build something different rather than having accepts headers as overloads. Perhaps we could use a pattern on the connection itself, perhaps Connection.WithHeaders("blah").Get()? That way the Get is simpler.

Great question, I think, for now, we isolate this change to the original intent - remove previews. We've got some other clean-up to do as well - and once the dust settles on those changes I believe we'll be able to more clearly see the next steps we need to take for things like headers now that we are not tied to the constraints of previews.

Let me know what you think!

[JonruAlveus]

Hey @nickfloyd, thanks for the feedback! I agree with everything you said. Very easy to veer off course with this kind of change and over complicate things. In that case, I believe I’m done, but I just noticed the merge conflicts so I’ll try to fix those on Monday.

[nickfloyd]

A huge thanks to you, @JonruAlveus for getting this knocked out! This is huge for the quality and dev sanity (for everyone) of this SDK! ❤️

[nickfloyd]

release_notes: Removes all REST API previews logic from the SDK