Implement trigger payload type validation diagnostics (CSDK200-204)#49
Closed
daviburg wants to merge 35 commits into
Closed
Implement trigger payload type validation diagnostics (CSDK200-204)#49daviburg wants to merge 35 commits into
daviburg wants to merge 35 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new diagnostics validator to the LSP server to validate trigger callback payload deserialization types against the indexed Connectors SDK (CSDK200–CSDK204), and wires it into the existing diagnostics pipeline with unit test coverage.
Changes:
- Added diagnostic code constants for CSDK200–CSDK204.
- Implemented
TriggerPayloadValidatorto analyzeDeserialize<T>calls within trigger methods and emit the new diagnostics. - Extended
SdkIndex.CreateForTesting(...)to accept optionaltypeNamesfor validator unit tests, and added a comprehensive new test suite.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Server/Diagnostics/DiagnosticCodes.cs | Defines CSDK200–CSDK204 constants for trigger payload validation diagnostics. |
| Server/Diagnostics/Validators/TriggerPayloadValidator.cs | New validator implementing trigger payload type validation and emitting CSDK200–CSDK204. |
| Server/Program.cs | Registers the new TriggerPayloadValidator in DI so it runs in the diagnostics pipeline. |
| Server/SdkIndex.cs | Allows tests to supply TypeNames via CreateForTesting(...) to simulate SDK type availability. |
| Server.Tests/TriggerPayloadValidatorTests.cs | Adds test coverage for correct usage, each diagnostic code, and key edge cases. |
- NormalizeQualifiedName now uses string prefix strip for deeply nested global:: - Add ValidateAsync_GlobalQualifiedCorrectType_NoDiagnostic test Co-authored-by: Dobby <dobby@microsoft.com>
…nown limitations - Iterate MethodDeclarationSyntax first, filter by trigger attribute, then scan invocations - Pass TriggerMetadataInfo directly to ValidateDeserializeInvocation (no more ancestor walk) - Document syntax-only limitation for same-named types in different namespaces Co-authored-by: Dobby <dobby@microsoft.com>
- Require using System.Text.Json or using Newtonsoft.Json for simple-name receiver matching - Add KnownSerializerNamespaces set and IsKnownSerializerNamespace helper - Update conditional access test to include using System.Text.Json Co-authored-by: Dobby <dobby@microsoft.com>
- Changed return null to continue in ExtractTriggerMetadata for unparseable attributes - Allows finding valid attributes when multiple/malformed attributes exist Co-authored-by: Dobby <dobby@microsoft.com>
- Each method is visited once in the loop, so cache is redundant — removed Co-authored-by: Dobby <dobby@microsoft.com>
- Check full receiver expression text for System.Text.Json.JsonSerializer etc. - Allows diagnostics when using fully-qualified calls without a using directive Co-authored-by: Dobby <dobby@microsoft.com>
- Map each serializer type to its specific namespace (JsonSerializer->System.Text.Json) - Fix NormalizeQualifiedName doc: uses string prefix strip, not recursive walk - Acknowledge nested type '+' vs '.' and using static false positive as known limitations Co-authored-by: Dobby <dobby@microsoft.com>
- Use exact namespace match instead of StartsWith to prevent child namespace false positives - Strip global:: prefix from fully-qualified receiver text - Remove unused IsKnownSerializerNamespace and KnownSerializerNamespaces Co-authored-by: Dobby <dobby@microsoft.com>
- Check using static against KnownFullyQualifiedSerializerTypes instead of simple name - Add NormalizeUsingName to strip global:: from using name Co-authored-by: Dobby <dobby@microsoft.com>
…er detection Known serializer types (JsonSerializer, JsonConvert) are static classes, so conditional access (?.) cannot be used on them. Any conditional-access Deserialize call is on a user-defined type/variable — skip it entirely. Also suppress simple-name serializer matching when the compilation unit declares a type with the same name (e.g., class JsonSerializer), since the local declaration shadows the imported type.
daviburg
force-pushed
the
feature/trigger-payload-validation
branch
from
f89811f to
7481e5a
Compare
…heck Replace hardcoded string comparisons with the shared set so serializer identity is defined in one place. Adding a new serializer now only requires updating the static data declarations, not hunting for scattered string literals.
3 tasks
1. Test global::System.Text.Json using directive normalization — found and fixed a bug where importedNamespaces wasn't normalizing global:: prefix, so 'using global::System.Text.Json;' wouldn't recognize JsonSerializer. 2. Test nested SDK type with '+' separator — verifies BuildTypeNameLookup correctly extracts simple names from 'Outer+WrongTriggerPayload' and that CSDK200 fires for a mismatched nested type.
1. Alias using directives (e.g., 'using STJ = System.Text.Json;') do not bring types into scope by simple name — exclude them from importedNamespaces to prevent false serializer matches. 2. Support simple-name using static (e.g., 'using static JsonSerializer;') when the corresponding namespace is already imported and the name is not shadowed by a local type declaration. 3. Extracted IsKnownStaticSerializerImport helper to consolidate both fully-qualified and simple-name static import detection.
Member
Author
|
Closing to re-create with a fresh review context (86 resolved threads were likely overwhelming Copilot reviewer). Same branch, same code — new PR for a clean review pass. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements all 5 trigger payload type validation diagnostics from #42.
Diagnostics
Validation
Closes #42