Remaining patches all in one commit #1827
Conversation
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
kendallb
force-pushed
the
feature/kendallb/remaining-patches
branch
from
e24ffe5 to
483f4de
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
|
|
I have quite a few comments:
I plan to cherry-pick some stuff from this PR, and will keep it open meanwhile. |
|
It has been a while since I wrote this and I really need to rebase is against the current develop tree to determine what needs to stay. If you cherry pick over what you will accept now I can go back and rebase it and look at the remaining bits we changed and research why. We made all those changes to fix things we ran into using it in our code, but I am sure the approach can be changed to achieve what we needed and be acceptable upstream. |
kendallb
force-pushed
the
feature/kendallb/remaining-patches
branch
from
483f4de to
2d3a491
Compare
|
I just rebased this against develop so I can get a clean merge and will go through and revisit everything. Some stuff was already merged so I cleaned that up in the rebase. |
kendallb
force-pushed
the
feature/kendallb/remaining-patches
branch
from
2d3a491 to
bdf012d
Compare
|
Ok following up on your comments:
The authenticator must be request based if the client is shared across requests that might need different authentication (ie: different for each user of a web site for instance so different in each request). We have that case so that is what we needed it for. I removed it from the base client because I believe that is the wrong location for the authenticator to reside because it is inherently a request centric thing and not related to the client instance at all. But it is a breaking change so I would suggest leaving it in the client for now and make the request based version override the client version, and only use the client version if the request one is not present. But then I would seriously consider removing it like I have done in v109 as a breaking change as I think that is another way to drill home the concept that the client should be reused for multiple requests and not instantiated per request.
I think this was an artifact of the old code base, as now that I have rebased all that happened to StreamJsonAsync was to add the optional authenticator. The option for the headers was moved around in a prior commit which was already in develop so that bit is clean now.
Thanks. It's a bit merged in with the rest of the code but I do have a branch with mostly the cookie stuff in it (and I have rebased it) if that helps. That still does have the base URL stuff in it, and moving the options class around and making it private in the client again but that could be moved out that change. LMK if you want me to do that.
I would argue it is dangerous to let the cookie container remain at the client level because you will have cookies end up crossing over requests for any reused clients. If the client happens to be used in a multi user environment such as on a web site, you would never want cookies from the request for one user to an external service to end up in the request for another user of the same service which is exactly what will happen if the cookies are shared at the client level. So while it's plausible that it can be used in some scenarios, I would argue it is way too dangerous to allow that to happen and better to break the API and have folks update their code to do cookie handling at the request level. As I mentioned previously when it comes to cookie handling, 99% of folks using RestSharp for interacting with REST services won't be using cookies anyway, as it's is very unusual for stateless REST services to care about cookies. In our case the only time we are actually using cookies is when we are doing web scraping using RestSharp and need to process user logins, so we execute the login code to get the cookies and then requests after that are properly authenticated. It is certainly vital in that scenario that the cookies are never shared.
Yeah not sure why I did that, so I removed it.
Yes, all that is removed now as you implemented a better solution anyway a while back. |
|
BTW I think we discussed somewhere else about the public Options class in the client. In my code I made it internal from the start and exposed BaseUrl and Host at the client level as properties. That is a breaking change so perhaps something to leave out until v109? |
|
BTW, related to client level cookies, the old client always created a cookie container in the client which is always used for all requests. As mentioned 99% of RestSharp users will not be using cookies, however having a container at the client level means cookies will be cross pollinating between requests using the same client instance which is really bad (big security issue). So most of the time there probably are not any cookies that matter, but if there are it could be problematic for the user of the library. Recall Microsoft even suggests not using it at all with HttpClient: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-6.0 Cookies
The biggest annoying issue with the way cookies work in HttpClient is that they do not follow with redirects if redirects are followed within HttpClient itself (something we have run into). I don't have a clean solution for that at the moment without finding a way to process the redirects somehow at the RestSharp level but I just worked around that in our code. |
|
Created a new pull request with just the cookie handling and removed the bits changing the client Options to be internal. I still think we should do that, but perhaps that is better in another pull request? |
kendallb
force-pushed
the
feature/kendallb/remaining-patches
branch
from
bdf012d to
ffa541d
Compare
|
Oh I remember now why I removed SetBearerToken(). Because the authenticator is now per request, you do not need to set the bearer token for the authenticator attached to the client as you can just create a new one. So the reason for needing it at the authenticator level is gone (and I updated the unit test to reflect that). But it can certainly be left there so someone can re-use an authenticator and adjust the bearer token as needed. I will adjust the test to show both cases. |
kendallb
force-pushed
the
feature/kendallb/remaining-patches
branch
from
ffa541d to
9374576
Compare
|
Just remember that for M2M integration, everything is reused. Say I want to call a service on Cloud Run. I need to get the application service account, get the id token, and the authenticator would generate an access token. The access token will be used for all the requests until it expires. The scenario of forcing the authenticator to handle 401 is something I am considering, as it would be very useful. This |
Fair enough, so you can just use a shared authenticator or create a new one using the correct token for each request. Since the authenticator is now set at the request level, the need to override the token in an existing authenticator is gone as that was really only a necessary requirement when the authenticator was set on the client and could not be changed once the client was constructed. |
kendallb
force-pushed
the
feature/kendallb/remaining-patches
branch
2 times, most recently
from
23a931e to
6c9779c
Compare
Yeah, it's a design mistake, makes no sense to do it. |
Well, I think quite a lot of things are piling up as breaking changes, so 109 will be there quite soon. |
|
Perhaps for v108 the options class is marked as obsolete but remains, and we add the new property accessors for the base URL etc, then for v109 it gets removed? |
kendallb
force-pushed
the
feature/kendallb/remaining-patches
branch
from
6c9779c to
18454d8
Compare
|
Rebased against dev now that all the cookie code is in there. All that is left is moving authenticator to the request and the client options being hidden. |
…way to get the exceptions out otherwise.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
|
|
I think we need separate issues for moving/copying things like response writers and authenticator. I don't know exactly what those response writers are being used for. I'd keep the authenticator property in the client options as in many (most?) cases you'd use the same authenticator for all the requests when using machine-to-machine integrations. For multi tenancy scenarios when authentication details change between the requests, it makes sense to use an authenticator scoped for a single request. |
|
Aha, sorry, we already have a request-level authenticator, so the second part of my comment is irrelevant. |
|
Yeah the request level stuff is done. I can try to take a look and separate out what is left? |
|
I think some things like |
|
I added the CacheControl but not sure about user agent. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
|
Description
Since I have quite a few intertwined PR's left, some of those won't rebase onto each other easily. So here is one that has it all combined if you want to review that and we discuss any tweaks necessary.
Otherwise you can continue to review my other changes, but until these are accepted in some form we still have to keep using our own fork in production.