Closed sean-r-williams closed 2 months ago
@microsoft-github-policy-service agree company="Bungie, Inc."
/azp run PowerShell.PSResourceGet
@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
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.
@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.
@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.
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.