Custom Auth Query Params (2) #666
Conversation
googlebot
added
the
cla: yes
| @@ -29,6 +30,9 @@ namespace Google.Apis.Requests.Parameters | |||
| /// </summary> | |||
| public static class ParameterUtils | |||
| { | |||
| //TODO: Find out how to resolve the issue with setting up the Logger: | |||
| private static readonly ILogger Logger = ApplicationContext.Logger.ForType<ParameterUtils>(); | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@squid808 I'm sorry that it takes me too long. I'll try to get to it before the weekend. |
|
|
||
| /// <summary>Gets the user defined query parameters.</summary> | ||
| public IEnumerable<KeyValuePair<string, string>> UserDefinedQueryParams | ||
| { get { return userDefinedQueryParams; } } |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Awesome job, and thanks for moving from #650 to this one (huge merge.. sorry). |
…structor for RequestParameterAttribute
|
@peleyal I updated things based on your comments, thank you. I had indeed not noticed your note in the previous PR to add a new constructor, so I sorted that all out. Using the other logger method also worked, thank you. However, since I'm still on VS 2013 something wonky is happening with the Page Streamer tests for me, which seems to have killed all my other unit tests, and now VS isn't even recognizing the tests to run them. So, I didn't get the test fully fixed just yet, but I wanted to throw the rest of the code up in the meantime. I'll update once I sort out the tests. |
… to better test, fixed typo
|
Okey dokey, that should do it. I had the newer NUnit adapter installed for another project, and had disabled the older one in the VS extensions. I just had to enable both, didn't seem to be a conflict. Also, fwiw I can only compile the solution if I comment out the PageStreamer tests. I haven't looked in to it too much, but I assume there is maybe some newer syntax that I can't compile in VS2013, so I've been commenting that whole file out and not committing that change as my workaround. |
| action(attribute.Type, name, value); | ||
| if (attribute.Type == RequestParameterType.UserDefinedQueries) | ||
| { | ||
| if (typeof(IEnumerable<KeyValuePair<string, string>>).IsAssignableFrom(value.GetType())) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Awesome work! Thanks for adding this functionality and being patient with my slow review. |
…RequestParameterAttribute, updated ParameterUtilsTests
| } | ||
|
|
||
| [Test] | ||
| public void RevokeTokenAsyncTest() |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
So, like I mentioned my bad about the change to Attribute. Like I explained you should actually revert it. Regarding the broken tests, you can actually not test the Revoke method in this PR and just add a TODO to test it later on. We will focus only on your change here, and later on in another PR we will test more methods. Thanks! |
…code accordingly, added test assertion to reflect changes
|
I think that should about cover it. If you want me to add more info or a person's name in the TODO for the test, I certainly can. Once I confirm the build finishes (and that I didn't do anything stupid), I'll remove the DO NOT MERGE from the PR name. |
squid808
changed the title
| /// by the library which will be included in the resultant authentication URL. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// The name of this parameter is used only for the constructor and will not end up in the resultant query string. |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| Assert.That(result, Contains.Substring("customParam2=customVal2")); | ||
|
|
||
| //assert that the parameter name for the custom parameters does not carry through to the result | ||
| Assert.IsFalse(result.Contains("query_param_attribute_name")); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
We are really close! |
|
Glad for the nits, sorry there were so many that were simply catchable. I'm running on fumes lately. I think that should square it all away. |
|
Awesome! I'll wait for all the tests to pass, and then I'll merge it. Thanks so much for implementing it. |
|
@squid808, well done! 👍 |
Picking up where pull 650 failed and left off prior to the big changes and my bad commits, this add the ability for users to include their own custom query parameters when making an authorization URL. This is in response to issue 638 at request of @peleyal.
By providing a KeyValuePair<string, string>[] for the UserDefinedQueryParams member of the GoogleAuthorizationCodeFlow.Initializer, this collection will be used to update the resulting url via Google.Apis.Requests.Parameters.ParameterUtils.IterateParameters().
An attempt at a unit test has been provided with /GoogleApis.Tests/Apis/Requests/Parameters/ParameterUtilsTest.cs
The initial build will fail due to the issue with the logger, which I'll detail via a note on the code line.