Upgrade .NET 10 #60
Upgrade .NET 10 #60
Conversation
📝 WalkthroughWalkthroughUpgrades backend to .NET 10 (SDK and runtime), bumps numerous NuGet packages, removes an SQL schema-compare file, deletes HTTP client logging extension and its DI registration, adds logger IsEnabled guards, converts solution descriptor from Changes
Sequence Diagram(s)sequenceDiagram
participant Startup as Program.cs (Startup)
participant DI as ServiceCollection
participant HttpClient as HttpClient pipeline
participant Telemetry as TelemetryTrigger
note right of Startup `#DDEBF7`: Previously registered\nAddHttpClientLogger()
Startup->>DI: services.AddHttpClientLogger() [removed]
DI--x HttpClient: HttpClient logger pipeline (was present)
note right of HttpClient `#FCE8D6`: HttpClientLogger removed\n(no logging hook)
Telemetry->>Telemetry: logger.IsEnabled(LogLevel.Information)?
alt enabled
Telemetry->>Telemetry: Log Information
else disabled
Telemetry-->>Telemetry: skip logging
end
opt exception path
Telemetry->>Telemetry: logger.IsEnabled(LogLevel.Error)?
Telemetry->>Telemetry: Log Error (if enabled)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas to focus on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/backend/Functions/Ts.Functions/TelemetryTrigger/TelemetryTrigger.cs (2)
35-38: Logger guard could be optimized for better performance gains.The
IsEnabledguard prevents theLogInformationcall when Information logging is disabled. However, thejsonstring is already computed on line 31 before this guard, so the guard doesn't prevent that potentially expensive UTF8 decoding operation.Since
jsonis also used on line 41, you could consider a different approach: only decode and log in non-production when Information logging is enabled, and always decode for the telemetry processing. However, this would complicate the flow, so the current implementation is acceptable.
48-51: Error logging guard is uncommon but acceptable.Guarding error logs with
IsEnabledchecks is unusual since error-level logging is typically always enabled in production. The guard adds a small overhead for a check that will almost always pass. However, this may be useful in specific test scenarios or as part of a consistent logging pattern across the codebase.src/backend/Database/Ts.TelemetryDatabase.Sql/Ts.TelemetryDatabase.Sql.sqlproj (2)
2-3: New Microsoft.Build.Sql SDK reference looks reasonable; confirm preview choice.Using the
<Sdk Name="Microsoft.Build.Sql" Version="2.1.0-preview.1" />element is consistent with multi‑SDK style; just make sure you actually want a preview build here (and that your build agents/VS version support this specific SDK version).
36-37: Security script inclusions look good; ensure files exist in source.Including
Security\create_user.sqlandSecurity\db_execute_procedure_role.sqlis fine; just confirm these files are present in the repo and don’t rely on environment‑specific assumptions (e.g., pre‑existing roles) during deployment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/backend/Database/Ts.TelemetryDatabase.Sql/Compares/SqlSchemaCompare.Dev.scmp(0 hunks)src/backend/Database/Ts.TelemetryDatabase.Sql/Ts.TelemetryDatabase.Sql.sqlproj(1 hunks)src/backend/Database/Ts.TelemetryDatabase/Ts.TelemetryDatabase.csproj(0 hunks)src/backend/Directory.Build.props(1 hunks)src/backend/Directory.Packages.props(1 hunks)src/backend/Functions/Ts.Functions.Tests/CacheProvider.cs(0 hunks)src/backend/Functions/Ts.Functions.Tests/Ts.Functions.Tests.csproj(0 hunks)src/backend/Functions/Ts.Functions/Extensions/HttpClientLogger.cs(0 hunks)src/backend/Functions/Ts.Functions/Program.cs(0 hunks)src/backend/Functions/Ts.Functions/TelemetryTrigger/TelemetryTrigger.cs(2 hunks)src/backend/Functions/Ts.Functions/Ts.Functions.csproj(0 hunks)src/backend/Infrastructure/Ts.Infrastructure/parameter-types.bicep(2 hunks)src/backend/Infrastructure/Ts.Infrastructure/ts.dev.bicepparam(2 hunks)src/backend/Infrastructure/Ts.Infrastructure/ts.prod.bicepparam(2 hunks)src/backend/TelemetryStash.Backend.sln(0 hunks)src/backend/TelemetryStash.Backend.slnx(1 hunks)src/backend/global.json(1 hunks)
💤 Files with no reviewable changes (8)
- src/backend/Functions/Ts.Functions/Program.cs
- src/backend/Database/Ts.TelemetryDatabase/Ts.TelemetryDatabase.csproj
- src/backend/TelemetryStash.Backend.sln
- src/backend/Functions/Ts.Functions.Tests/CacheProvider.cs
- src/backend/Functions/Ts.Functions/Ts.Functions.csproj
- src/backend/Functions/Ts.Functions.Tests/Ts.Functions.Tests.csproj
- src/backend/Database/Ts.TelemetryDatabase.Sql/Compares/SqlSchemaCompare.Dev.scmp
- src/backend/Functions/Ts.Functions/Extensions/HttpClientLogger.cs
🧰 Additional context used
🪛 GitHub Actions: CI/CD Backend Functions - Telemetry Stash
src/backend/Database/Ts.TelemetryDatabase.Sql/Ts.TelemetryDatabase.Sql.sqlproj
[error] 20-20: dotnet test failed. Ts.TelemetryDatabase.Sql.sqlproj could not be loaded: Name cannot begin with the '$' character, hexadecimal value 0x24. Line 20, position 36.
🔇 Additional comments (13)
src/backend/TelemetryStash.Backend.slnx (2)
1-35: LGTM: Migration to .slnx solution format.The migration from the legacy .sln format to the newer XML-based .slnx format is a good modernization step that provides better readability and source control friendliness.
25-25: Path verification complete—no issues found.The relative path
../../.github/Ts.Pipelines.GitHub.msbuildprojcorrectly navigates fromsrc/backend/to the project file, which exists at the referenced location.src/backend/Directory.Build.props (1)
3-3: LGTM: Target framework updated to .NET 10.The TargetFramework update to net10.0 is correct and aligns with the PR objectives.
src/backend/Infrastructure/Ts.Infrastructure/ts.dev.bicepparam (1)
28-28: LGTM: Runtime version consistent with production.The linuxFxVersion updates match the production configuration. Refer to the verification request in ts.prod.bicepparam to confirm the runtime string validity.
Also applies to: 39-39
src/backend/Infrastructure/Ts.Infrastructure/parameter-types.bicep (1)
42-42: LGTM: Type constraints updated for .NET 10.The linuxFxVersion type definitions now enforce
DOTNET-ISOLATED|10.0exclusively, which prevents accidental use of older runtime versions. This is appropriate for a complete framework upgrade.Also applies to: 61-61
src/backend/Directory.Packages.props (3)
20-25: LGTM: Microsoft.Extensions packages updated for .NET 10.The Microsoft.Extensions.* packages have been correctly updated from version 9.x to 10.0.0, aligning with the .NET 10 framework upgrade. Note that Microsoft.Extensions.Caching.Hybrid has also moved from preview to a stable 10.0.0 release.
9-9: Azure.Identity 1.17.1 and Microsoft.Data.SqlClient 6.1.3 are secure.Verification against GitHub Security Advisories confirms both packages are free from known vulnerabilities at their specified versions:
- Azure.Identity 1.17.1 is patched against all known HIGH and MODERATE severity advisories (RCE, information disclosure, elevation of privilege).
- Microsoft.Data.SqlClient 6.1.3 is patched against all known HIGH and MODERATE severity advisories (security feature bypasses, information disclosure).
No action required.
12-12: No issues found — both packages are compatible with .NET 10 and release notes confirm no breaking changes.Microsoft.Azure.Functions.Worker 2.51.0 has no breaking changes reported, and Microsoft.Azure.Functions.Worker.ApplicationInsights 2.50.0 similarly has no breaking changes listed. The version increments consist of feature additions and dependency updates only. The changes are safe to deploy.
src/backend/global.json (1)
3-3: .NET SDK version 10.0.100 is confirmed as a valid, stable GA release.The SDK version 10.0.100 is the official .NET 10 GA release from November 11, 2025, which is appropriate for production use.
src/backend/Infrastructure/Ts.Infrastructure/ts.prod.bicepparam (1)
29-29: Runtime string confirmed correct.The linuxFxVersion value
DOTNET-ISOLATED|10.0is the correct runtime stack identifier for .NET 10 isolated worker on Azure Functions. The code at lines 29 and 40 is accurate and requires no changes.src/backend/Database/Ts.TelemetryDatabase.Sql/Ts.TelemetryDatabase.Sql.sqlproj (3)
5-6: Verify new DSP/provider and Name/Guid settings match your target environment.
<Name>and the newProjectGuidare fine, but switchingDSPtoSqlAzureV12DatabaseSchemaProviderchanges the model provider semantics; double‑check this is intentional for on‑prem vs Azure deployments and matches how the DB is actually hosted.Also applies to: 9-9
28-29: Folders for Tables and UDTs included correctly.Adding
TablesandUser Defined Typesfolders keeps the project tree aligned with the Build items; no issues here.
41-41: Telemetry stored procedure and UDT Build items are consistent.Re‑adding
Stored Procedures\Telemetry\UpsertTelemetry.sqlandUser Defined Types\TelemetriesType.sqlaligns procedures with the UDT definitions and table layout; looks correct.Also applies to: 43-43
| <!-- This is necessary to ensure that VS properly restores the packages for the sql project, which is necessary for Microsoft.Build.Sql database projects. --> | ||
| <Target Name="EnsureNuGetRestore" BeforeTargets="PrepareForBuild" Condition="!Exists('$(MSBuildProjectDirectory)\obj\project.assets.json')"> | ||
| <Message Importance="high" Text="Running NuGet restore for SQL project $(MSBuildProjectName)..." /> | ||
| <Exec Command="dotnet restore "$(MSBuildProjectFile)"" ContinueOnError="false" /> | ||
| </Target> |
There was a problem hiding this comment.
🧩 Analysis chain
Fix Exec Command quoting; this causes the $ XML parse error and breaks CI.
The inner quotes in the Command attribute terminate the XML attribute early, so $(MSBuildProjectFile) is parsed as an attribute name starting with $, matching the pipeline error (“Name cannot begin with the '$' character… Line 20”). Use XML‑escaped quotes around the MSBuild property:
- <Exec Command="dotnet restore "$(MSBuildProjectFile)"" ContinueOnError="false" />
+ <Exec Command="dotnet restore "$(MSBuildProjectFile)"" ContinueOnError="false" />This should allow the project to load and dotnet test to run.
🏁 Script executed:
#!/bin/bash
# Check if the file exists and show full context around lines 17-21
fd -t f "\.sqlproj$" | head -20Length of output: 78
🏁 Script executed:
#!/bin/bash
# Verify the exact XML syntax issue with unescaped quotes
cat << 'EOF' > /tmp/test_xml.xml
<?xml version="1.0"?>
<Project>
<!-- This should fail XML parsing -->
<Exec Command="dotnet restore "$(MSBuildProjectFile)"" />
</Project>
EOF
# Try to parse it
python3 << 'PYTHON'
import xml.etree.ElementTree as ET
try:
tree = ET.parse('/tmp/test_xml.xml')
print("✗ XML parsed successfully (unexpected)")
except ET.ParseError as e:
print(f"✓ XML parsing failed as expected: {e}")
PYTHONLength of output: 84
🌐 Web query:
MSBuild Exec command proper quoting syntax for properties like $(MSBuildProjectFile)
💡 Result:
Short answer: quote the property with either XML-escaped quotes or by using single quotes for the attribute so you can use normal double-quotes inside.
Examples
- Using XML attribute with escaped quotes:
- Using single-quoted attribute (easier/readable):
Notes
- If the value can contain a trailing backslash it can interact badly with a closing quote — consider appending a dot or otherwise normalizing the path.
- If your command contains % signs or other MSBuild special chars you may need MSBuild escapes (e.g. %25 for %).
Sources: StackOverflow examples and MSBuild escaping notes. [1][2]
🏁 Script executed:
#!/bin/bash
# Extract and display lines 17-21 from the actual sqlproj file
sed -n '17,21p' 'src/backend/Database/Ts.TelemetryDatabase.Sql/Ts.TelemetryDatabase.Sql.sqlproj'Length of output: 507
🏁 Script executed:
#!/bin/bash
# Extract and display lines 17-21 from the actual sqlproj file
sed -n '17,21p' 'src/backend/Database/Ts.TelemetryDatabase.Sql/Ts.TelemetryDatabase.Sql.sqlproj'Length of output: 507
Fix Exec Command quoting to resolve XML parse error blocking CI.
Line 20 has unescaped quotes in the Command attribute that terminate the XML attribute early, causing the parser to fail on $(MSBuildProjectFile) ("Name cannot begin with the '$' character"). Use XML-escaped quotes around the MSBuild property:
- <Exec Command="dotnet restore "$(MSBuildProjectFile)"" ContinueOnError="false" />
+ <Exec Command="dotnet restore "$(MSBuildProjectFile)"" ContinueOnError="false" />🧰 Tools
🪛 GitHub Actions: CI/CD Backend Functions - Telemetry Stash
[error] 20-20: dotnet test failed. Ts.TelemetryDatabase.Sql.sqlproj could not be loaded: Name cannot begin with the '$' character, hexadecimal value 0x24. Line 20, position 36.
🤖 Prompt for AI Agents
In
src/backend/Database/Ts.TelemetryDatabase.Sql/Ts.TelemetryDatabase.Sql.sqlproj
around lines 17 to 21, the Exec Command attribute contains unescaped double
quotes that break XML parsing; fix the Command attribute so the inner quotes
around $(MSBuildProjectFile) are XML-safe (for example replace the inner double
quotes with "$(MSBuildProjectFile)" or switch the attribute to single
quotes and keep the inner double quotes), ensuring the resulting XML is
well-formed and the dotnet restore command runs correctly.
mikmog
force-pushed
the
upgrade-framework
branch
from
1315937 to
f474045
Compare
mikmog
force-pushed
the
upgrade-framework
branch
from
f474045 to
b9f143a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/backend/Functions/Ts.Functions/TelemetryTrigger/TelemetryTrigger.cs (1)
41-42: Improve deserialization failure message for easier diagnosticsThe exception currently interpolates
body(abyte[]), which will render asSystem.Byte[]instead of the actual payload. UsingjsonoreventData.Bodywould make the error far more actionable.- var telemetry = JsonSerializer.Deserialize<TelemetryRequest>(json, JsonOptions) - ?? throw new Exception($"Telemetry deserialization failed: {body}"); + var telemetry = JsonSerializer.Deserialize<TelemetryRequest>(json, JsonOptions) + ?? throw new Exception($"Telemetry deserialization failed. Payload: {json}");src/backend/Database/Ts.TelemetryDatabase.Sql/Ts.TelemetryDatabase.Sql.sqlproj (1)
8-10: Confirm LangVersion pinning in the .NET 10 world
ProjectGuidandRunSqlCodeAnalysislook fine, but explicitly pinning<LangVersion>9.0</LangVersion>here may leave this project on an older C# version than the rest of the backend now that you're on .NET 10. If there's no strong reason to lock this to 9.0, consider either dropping the property (to inherit the solution default) or bumping it to a matching/later version.The current CI error about
NuGet.Build.Tasks.Pack.targetsbeing missing doesn’t appear to originate from this sqlproj and is more likely coming from an.msbuildprojthat imports the NuGet pack SDK, so the fix for that will probably live outside this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/backend/Database/Ts.TelemetryDatabase.Sql/Compares/SqlSchemaCompare.Dev.scmp(0 hunks)src/backend/Database/Ts.TelemetryDatabase.Sql/Ts.TelemetryDatabase.Sql.sqlproj(1 hunks)src/backend/Database/Ts.TelemetryDatabase/Ts.TelemetryDatabase.csproj(0 hunks)src/backend/Directory.Build.props(1 hunks)src/backend/Directory.Packages.props(1 hunks)src/backend/Functions/Ts.Functions.Tests/CacheProvider.cs(0 hunks)src/backend/Functions/Ts.Functions.Tests/Ts.Functions.Tests.csproj(0 hunks)src/backend/Functions/Ts.Functions/Extensions/HttpClientLogger.cs(0 hunks)src/backend/Functions/Ts.Functions/Program.cs(0 hunks)src/backend/Functions/Ts.Functions/TelemetryTrigger/TelemetryTrigger.cs(2 hunks)src/backend/Functions/Ts.Functions/Ts.Functions.csproj(0 hunks)src/backend/Infrastructure/Ts.Infrastructure/parameter-types.bicep(2 hunks)src/backend/Infrastructure/Ts.Infrastructure/ts.dev.bicepparam(2 hunks)src/backend/Infrastructure/Ts.Infrastructure/ts.prod.bicepparam(2 hunks)src/backend/TelemetryStash.Backend.sln(0 hunks)src/backend/TelemetryStash.Backend.slnx(1 hunks)src/backend/global.json(1 hunks)
💤 Files with no reviewable changes (8)
- src/backend/Functions/Ts.Functions.Tests/Ts.Functions.Tests.csproj
- src/backend/Functions/Ts.Functions/Ts.Functions.csproj
- src/backend/Functions/Ts.Functions.Tests/CacheProvider.cs
- src/backend/Functions/Ts.Functions/Extensions/HttpClientLogger.cs
- src/backend/Database/Ts.TelemetryDatabase/Ts.TelemetryDatabase.csproj
- src/backend/Database/Ts.TelemetryDatabase.Sql/Compares/SqlSchemaCompare.Dev.scmp
- src/backend/TelemetryStash.Backend.sln
- src/backend/Functions/Ts.Functions/Program.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/backend/global.json
- src/backend/Directory.Build.props
- src/backend/Infrastructure/Ts.Infrastructure/parameter-types.bicep
- src/backend/Infrastructure/Ts.Infrastructure/ts.dev.bicepparam
- src/backend/Infrastructure/Ts.Infrastructure/ts.prod.bicepparam
🧰 Additional context used
🪛 GitHub Actions: CI/CD Backend Functions - Telemetry Stash
src/backend/Database/Ts.TelemetryDatabase.Sql/Ts.TelemetryDatabase.Sql.sqlproj
[error] MSBuild import failed: The imported project '/usr/share/dotnet/sdk/10.0.100/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets' was not found (MSB4019). This prevents NuGet.Build.Tasks.Pack from loading during restore/build.
[error] MSBuild import failed: The imported project '/usr/share/dotnet/sdk/10.0.100/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets' was not found (MSB4019). This prevents NuGet.Build.Tasks.Pack from loading during restore/build.
🔇 Additional comments (4)
src/backend/Functions/Ts.Functions/TelemetryTrigger/TelemetryTrigger.cs (2)
33-38: Info logging guard is clear and low-overheadThe additional
logger.IsEnabled(LogLevel.Information)guard inside the non-production block is fine and keeps the Info telemetry logging cheap when disabled, without changing behavior.
46-51: Error logging guard is consistent with the Info patternWrapping the Error log in an
IsEnabled(LogLevel.Error)check is consistent with the new pattern and keeps the hot path simple; behavior remains driven by the configured log level.src/backend/Directory.Packages.props (1)
9-27: Package bumps look aligned with .NET 10; please validate runtime and test behaviorThe Azure Functions isolated worker stack here (
Microsoft.Azure.Functions.Worker2.51.0,Worker.Sdk2.0.7,Worker.ApplicationInsights2.50.0) meets the current guidance for targeting .NET 10 isolated apps, which calls for Worker ≥ 2.50.0 and Worker.Sdk ≥ 2.0.5. (learn.microsoft.com)Upgrading
Microsoft.Extensions.*to 10.0.0 plus newerMicrosoft.Data.SqlClient,Microsoft.SqlServer.DacFx, andTestcontainers.MsSqlis a sensible match for the framework upgrade, but these jumps can change defaults (e.g., diagnostics and SQL connection behavior). It’s worth running full integration tests against a real database after this lands.On the test side,
xunit.runner.visualstudio3.1.5 is designed to run xUnit.net 2.x tests, so it should be compatible with your existingxunit2.9.3 projects alongside the newerMicrosoft.NET.Test.Sdk18.0.1. (nuget.org)Also applies to: 33-33, 35-35
src/backend/TelemetryStash.Backend.slnx (1)
23-26: The review comment is based on incorrect assumptions about CI build behaviorThe actual CI workflows do not build the slnx file. The CI only builds specific project directories directly (e.g.,
src/backend/Functions/Ts.Functions). Since the slnx file is not invoked in any active CI workflow, the presence or absence of<Build Project="false" />onTs.Pipelines.GitHub.msbuildprojhas no impact on CI success or failure.The slnx file appears to be a local development convenience, not part of the CI/CD pipeline. The suggested fix to exclude the Pipelines project from the solution descriptor would not resolve any CI issues because the CI never builds this solution file.
Likely an incorrect or invalid review comment.
Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.