Refactor V2/NuGetServerAPICalls to use object-oriented query/filter builder#1645
Conversation
43359fe to
52477bc
Compare
|
@microsoft-github-policy-service agree company="Bungie, Inc." |
|
/azp run PowerShell.PSResourceGet |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@sean-r-williams thanks for creating this PR, it looks really thorough and the context was well explained :) The team will take a deeper look into it an get back by this week with any suggestions/questions/follow up. |
|
Thanks @anamnavi! It looks like CodeQL is falling for some reason, but I'm not authorized to view the workflow logs. I have a feeling this might be due to the new dependency on System.Web, but I'm unsure. If someone from your team is able to summarize the errors coming back from CodeQL, I might be able to resolve the errors myself. |
|
Scratch that, it sems I forgot to add a file when I renamed a method. If someone could re-run Azure Pipelines, that'd be appreciated. |
|
/azp run PowerShell.PSResourceGet |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
no worries! if that doesn't work I can take a look at the CodeQL errors/see if it has to do with things on our end. |
@anamnavi Gentle reminder on this - the current implementation of v2 API calls blocks dependency installation on Artifactory. I'd like to make sure either this PR or #1644 are included in 1.0.5 (or whatever the next patch or minor release happens to be), and that said release is made available in the near future. |
| /// </param> | ||
| internal NuGetV2QueryBuilder(Dictionary<string, string> parameters) : this() | ||
| { | ||
| AdditionalParameters = new Dictionary<string, string>(parameters); |
There was a problem hiding this comment.
Can we have AdditionalParameters = parameters to save on the new allocation of Dictionary?
PR Summary
This PR replaces the existing NuGet v2 API call-building mechanism based on string manipulation (as used in members of
V2ServerAPICallsandNuGetServerAPICalls) with a separate pair of classes (NuGetV2QueryBuilderandNuGetV2FilterBuilder) that construct a query-string by aggregating user-supplied criteria.Resolves #1643.
PR Context
The current implementation of V2 API calling classes builds API call URLs (including parameters) by directly performing string-manipulation on the URL itself. This has several drawbacks:
+=on a string means a completely new string is created (with the suffix appended), and the prior string remains in memory until GC.NuGetV2FilterBuilderusesStringBuilderto avoid costly reallocation of large strings.NuGetV2QueryBuilderuses a derivative ofSystem.Collections.Specialized.NameValueCollectionto automatically URL-encode any configured parameters.NuGetV2FilterBuilderjoins filter clauses together after the full set is available, so there's no duplicated/inappropriately-placed joining operators.There are some specific limitations in the current implementation:
System.Webto build against TFMnet472, as we needSystem.Web.HttpUtility. To my knowledge, System.Web should be in the GAC on .NET 4.5 and above, and PowerShell 7 already has System.Web.HttpUtility.dll linked as-is.I'm not an expert in MSBuild, however, so if there's a better way to ensure
HttpUtilityis available I'm all ears.NuGetV2FilterBuilderdoesn't ensure that filter clauses are properly escaping OData-relevant syntax (parentheses, quotes, etc.).I wanted to focus namely on the aggregation of clauses versus the clauses themselves in the first pass - now that we have purpose-built classes for these, it would be significantly easier to expose an interface for filter clauses (
INuGetV2FilterCriterion?) with some inheritance for unary-/binary-operator criteria.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.