-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor V2/NuGetServerAPICalls to use object-oriented query/filter builder #1645
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
V2ServerAPICalls
andNuGetServerAPICalls
) with a separate pair of classes (NuGetV2QueryBuilder
andNuGetV2FilterBuilder
) 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.NuGetV2FilterBuilder
usesStringBuilder
to avoid costly reallocation of large strings.NuGetV2QueryBuilder
uses a derivative ofSystem.Collections.Specialized.NameValueCollection
to automatically URL-encode any configured parameters.NuGetV2FilterBuilder
joins 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.Web
to 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
HttpUtility
is available I'm all ears.NuGetV2FilterBuilder
doesn'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
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.