Harden DateOnly/TimeOnly version detection, modernize NuGet APIs, and add E2E coverage - #451
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves how the VS extension and CLI decide whether to emit native System.DateOnly/System.TimeOnly (vs legacy Microsoft.OData.Edm.Date/TimeOfDay) by making OData client version resolution more robust, modernizing NuGet/VS APIs, and adding end-to-end regression coverage.
Changes:
- Harden OData client version detection (VS: query installed/restored packages via NuGet services; CLI: read
project.assets.json, CPM,VersionOverride, property-evaluated versions, and version ranges). - Modernize NuGet APIs for VS2022+ (
INuGetProjectService) while keeping legacy behavior for older VS versions. - Add/expand E2E + unit tests for DateOnly/TimeOnly generation across version sources, and bump CLI package version to
0.3.2.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ODataConnectedService.Tests/ODataConnectedService.Tests.csproj | Enables VS2022PLUS in tests and adds a new EDMX metadata file as test content. |
| test/ODataConnectedService.Tests/FileHandling/ConnectedServiceFileHandlerTests.cs | Updates unit tests to validate version detection via installed-package provider + warning behavior. |
| test/ODataConnectedService.Tests/CodeGeneration/SampleServiceV4WithDateOnlyAndTimeOnly.xml | Adds a metadata document containing Edm.Date/Edm.TimeOfDay for DateOnly/TimeOnly E2E generation tests. |
| test/ODataConnectedService.Tests/CodeGeneration/CodeGenDescriptorTest.cs | Adds V4 E2E generation tests validating native vs legacy date/time emission by simulated package version. |
| test/Microsoft.OData.Cli.Tests/Microsoft.OData.Cli.Tests.csproj | Adjusts MSBuild package reference to avoid runtime asset conflicts in tests. |
| test/Microsoft.OData.Cli.Tests/FileHandling/ODataCliFileHandlerTests.cs | Expands CLI unit tests for CPM, overrides, MSBuild property versions, version ranges, and assets file precedence. |
| test/Microsoft.OData.Cli.Tests/CodeGeneration/ODataCliCodeGenerationTests.cs | Adds CLI E2E tests covering CPM, overrides, MSBuild properties, ranges, and restored assets-based resolution. |
| src/ODataConnectedService.Shared/ConnectedServicePackageInstaller.cs | Switches package detection on VS2022+ to INuGetProjectService while preserving legacy behavior elsewhere. |
| src/ODataConnectedService.Shared/ConnectedServiceFileHandler.cs | Switches VS-side version detection to installed/restored package query with caching + one-time warning on parse failures. |
| src/ODataConnectedService_VS2022Plus/ODataConnectedService_VS2022Plus.csproj | Adds NuGet.VisualStudio.Contracts dependency and defines VS2022PLUS. |
| src/Microsoft.OData.CodeGen/Microsoft.OData.CodeGen.csproj | Includes the new shared version-checking helper in the build. |
| src/Microsoft.OData.CodeGen/Common/ODataClientVersionChecker.cs | Centralizes version parsing and “supports native date/time types” capability checks. |
| src/Microsoft.OData.Cli/ProjectHelper.cs | Implements robust CLI-side version resolution (assets file, CPM, overrides, ranges) with warnings on failure. |
| src/Microsoft.OData.Cli/ODataCliMessageLogger.cs | Ensures warnings are surfaced on stderr (with a prefix) in addition to errors. |
| src/Microsoft.OData.Cli/ODataCliFileHandler.cs | Moves DateOnly/TimeOnly decision to the new async version-check logic. |
| src/Microsoft.OData.Cli/Microsoft.OData.Cli.csproj | Bumps CLI NuGet/package version to 0.3.2. |
Suppressed comments (1)
src/ODataConnectedService.Shared/ConnectedServicePackageInstaller.cs:88
- After awaiting IsPackageInstalledAsync(...).ConfigureAwait(false), execution may resume off the UI thread. IVsPackageInstaller.InstallPackage is a VS service call that is generally expected to run on the main thread; calling it from a thread-pool thread can cause threading violations or hangs. Switch back to the main thread before invoking InstallPackage.
if (!await this.IsPackageInstalledAsync(packageName).ConfigureAwait(false))
{
PackageInstaller.InstallPackage(packageSource, this.Project, packageName, (string)null, false);
await (this.MessageLogger?.WriteMessageAsync(LogMessageCategory.Information, $"Nuget Package \"{packageName}\" for OData client was added.")).ConfigureAwait(false);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| using Microsoft.VisualStudio.ConnectedServices; | ||
| using NuGet.VisualStudio; | ||
| using Shell = Microsoft.VisualStudio.Shell; | ||
| #if VS2022PLUS |
There was a problem hiding this comment.
why do we need this VS2022PLUS?
I mean why we wait now to add such? If it's needed, it should be added at the beginning?
There was a problem hiding this comment.
It is added now as a workaround to support the legacy ODataConnectedService targeting older VS SDK to keep building even after modernizing the ODataConnectedService_VS2022PLUS by replacing the obsolete IVsPackageInstallerServices.GetInstalledPackages (that causes UI-thread delays) with INuGetProjectService.GetInstalledPackagesAsync from NuGet.VisualStudio.Contracts. However, the NuGet.VisualStudio.Contracts service API and its dependencies only exist in the VS 2022+ SDK.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/ODataConnectedService.Shared/ConnectedServicePackageInstaller.cs:82
- In !VS2022PLUS builds, CheckAndInstallNuGetPackageAsync now runs whenever PackageInstaller is available, but IsPackageInstalledAsync returns false when PackageInstallerServices is null. That combination will attempt to install packages even though the code can’t determine whether they’re already installed (behavior changed from the previous guard on PackageInstallerServices), potentially causing redundant installs/errors.
public async Task CheckAndInstallNuGetPackageAsync(string packageSource, string packageName)
{
if (PackageInstaller != null)
{
try
src/Microsoft.OData.CodeGen/Common/ODataClientVersionChecker.cs:49
- TryParseVersion returns a System.Version directly from Version.TryParse. For typical NuGet versions like "7.6.4" this produces a Version with Revision == -1, and comparisons against 4-part thresholds (e.g., "7.6.4.0") will incorrectly treat 7.6.4 as LESS than 7.6.4.0. Normalizing missing build/revision parts to 0 avoids feature checks accidentally failing when the input is a 2- or 3-part NuGet version.
int suffixIndex = version.IndexOfAny(new[] { '-', '+' });
string normalizedVersion = suffixIndex >= 0 ? version.Substring(0, suffixIndex) : version;
return Version.TryParse(normalizedVersion, out parsedVersion);
}
Follow-up to #442. The DateOnly/TimeOnly feature only read the
Microsoft.OData.Clientversion from a direct<PackageReference>, so it silently emitted legacyEdm.Date/Edm.TimeOfDaywhenever the version came from anywhere else. This PR makes detection robust, modernizes the NuGet API, and adds E2E tests to prevent regressions.Version detection - resolve the client version from the restored
project.assets.json, Central Package Management,VersionOverride, MSBuild property expressions, and version ranges (not just the literal<PackageReference Version>).NuGet API - replace the obsolete
IVsPackageInstallerServices.GetInstalledPackages(UI-thread delays) withINuGetProjectService.GetInstalledPackagesAsync, gated behind#if VS2022PLUS; the legacy VSIX keeps the old API.Bump
Microsoft.OData.Cliversion to0.3.2Tests - CLI + VS end-to-end tests that generate real proxies for each version source (9.0.0 → native
System.DateOnly/System.TimeOnly, 8.0.0 → legacyEdmtypes), plus expanded unit coverage. Also fixes an MSBuildLocator engine-mismatch that broke the in-process generation tests on multi-SDK machines (ExcludeAssets="runtime"onMicrosoft.Build).