Fixes #358 MockGeneratorPlugin errors if the response headers have a duplicate

[mosoftwareenterprises]

If a response is recorded and contains the same header key value pair more than once, then an exception is thrown.

This change removes it by only allowing distinct values of the keys.

While I was working with the mock responses file, I noticed you could not comment them out, so a small tweak to allow that too.

This can be tested by hitting the https://graph.microsoft.com/v1.0/users endpoint, and paginating through the list of users you find e.g.

       var graphServiceClient= new GraphServiceClient(requestAdapter);

            // send a get users request to the graph api; note that the results are paged
            var allUsers = new List<User>();
            var usersPage = await graphServiceClient.Users.Request().Filter("accountEnabled eq true").GetAsync();
            do
            {
                allUsers.AddRange(usersPage);
            }
            while (usersPage.NextPageRequest != null && (usersPage = await usersPage.NextPageRequest.GetAsync()).Any());

[mosoftwareenterprises]

@microsoft-github-policy-service agree

[sebastienlevert]

Thanks for the PR @mosoftwareenterprises! I'm curious what are the headers that are present twice?

[mosoftwareenterprises]

Thanks for the PR @mosoftwareenterprises! I'm curious what are the headers that are present twice?

I have just spent the last 20mins trying to replicate the issue, and could only do so via browsing in google,
It is the Set-Cookie header specifically in this case.

[sebastienlevert]

Some scenarios will need multiple headers (on Graph, for instance with Prefer). I'm wondering if our data structure here is the right one. @waldekmastykarz or @gavinbarron would you mind chiming in?

[gavinbarron]

oof. yeah, multiple headers with the same name are allowed, and is some cases required. Using a dictionary here is not really the right data structure.
We should probably be using something like List<KeyValuePair<string, string>> instead.
Or Dictionary<string,string[]> would also work but require more work to populate.

[waldekmastykarz]

Good point @gavinbarron. We should definitely change how we store headers to List<KeyValuePair<string, string>>. @mosoftwareenterprises, would you be willing to update your PR?

[mosoftwareenterprises]

Yes @waldekmastykarz I will attempt an update to the PR.

Question though, as the changes are going to be further reaching than before, how do I regression test? I dont see any automated tests in the repo?

[waldekmastykarz]

Yes @waldekmastykarz I will attempt an update to the PR.

Question though, as the changes are going to be further reaching than before, how do I regression test? I dont see any automated tests in the repo?

At this stage we'll have to test it manually. Sorry we don't have a better answer at this moment

[waldekmastykarz]

Hey @mosoftwareenterprises, are you still working on this? Anything that we could help with?

[mosoftwareenterprises]

Hey @waldekmastykarz . Sorry I have been so busy, I have not had time to finish it. I will commit the latest changes here, but I have not checked it through yet.

[waldekmastykarz]

Are we using EntityFrmeworkCore for something or is this a leftover?

[mosoftwareenterprises]

Must be a leftover. Removed

[waldekmastykarz]

It seems odd to define headers as a list of dictionaries. Shouldn't it be a list of key-value pairs instead?

[mosoftwareenterprises]

Using a Dictionary (which is in effect the same thing AFAIK), means we can use Linq to do "ToDictionary()"

[mosoftwareenterprises]

This has now changed to a List<KeyValuePair<string,string>> due to the dictionary complicating some other areas.

[waldekmastykarz]

Hey @mosoftwareenterprises, are you still working on this?

[mosoftwareenterprises]

Hey @waldekmastykarz, I will be picking this up again this week. So hopefully I will have something for you soon.

[waldekmastykarz]

Awesome! Looking forward to it! We've done some changes in the meantime, so please don't forget to pull the latest changes to avoid conflicts 😊

[waldekmastykarz]

Hey @mosoftwareenterprises, it seems like your PR includes some additional commits that aren't a part of your change. Could you please clean it up so that we can review it more easily? We appreciate your help

[mosoftwareenterprises]

@waldekmastykarz I assume you are referring to the change to correct the CONTRIBUTING.md file which is incorrect, and the changes to add the serializer options in to ignore comments etc.
I have removed them both.
Let me know if you meant something else.

[waldekmastykarz]

@waldekmastykarz I assume you are referring to the change to correct the CONTRIBUTING.md file which is incorrect, and the changes to add the serializer options in to ignore comments etc. I have removed them both. Let me know if you meant something else.

Correct, I meant those files. I'll have another look just to double check we're not including anything else that shouldn't be there. Looking forward to checking out your work!

[waldekmastykarz]

Nice! Let's do a few changes before we continue

[waldekmastykarz]

Instead of creating a separate Dictionary for each key-value pair, let's use a List<Tuple<string, string>> which seems like a less overhead. Alternatively, we could use List<HttpHeader>, assuming we can serialize them easily.

[mosoftwareenterprises]

Looking at HttpHeader, it is not very nice to serialise, so opting for the Tuple

[mosoftwareenterprises]

Changed this to a List<KeyValuePair<string,string>> instead of Tuple. Made it more readable.

[mosoftwareenterprises]

I have tested this again, using a Confluence page (private) and found it was not always working, so additional changes made to cope with that. Ready for review again.

[waldekmastykarz]

Thanks @mosoftwareenterprises! We'll do some further adjustments to streamline it across all plugins. We appreciate your help ❤️