Add basic ID classes and parsing#1
Open
javiers451 wants to merge 24 commits intoGlobalTypeSystem:mainfrom
Open
Add basic ID classes and parsing#1javiers451 wants to merge 24 commits intoGlobalTypeSystem:mainfrom
javiers451 wants to merge 24 commits intoGlobalTypeSystem:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces a complete Global Type System (GTS) .NET library with multi-project structure, including core ID parsing and pattern matching, JSON entity extraction with schema support, in-memory registry implementations, and comprehensive test coverage across parsing, extraction, and ID operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parsers as Parsers<br/>(Pidgin)
participant GtsId as GtsId
participant Segment as GtsIdSegment
User->>GtsId: Parse("vendor.pkg.ns.type~")
GtsId->>Parsers: GtsTypeId.ParseOrThrow()
Parsers->>Parsers: Identifier → VersionFull → Pattern
Parsers-->>GtsId: IdentifierInfo {Kind, Segments[]}
GtsId->>GtsId: MapSegment (convert SegmentInfo → GtsIdSegment)
loop For each SegmentInfo
GtsId->>Segment: new(vendor, pkg, ns, type, major, minor)
Segment-->>GtsId: GtsIdSegment
end
GtsId->>GtsId: Set IsType=true, IsInstance=false
GtsId-->>User: GtsId {Id, Segments[], IsType}
sequenceDiagram
participant User
participant GtsJsonEntity as GtsJsonEntity
participant Extractor as Extraction<br/>Logic
participant JsonWalker as Reference<br/>Walker
participant References as GtsRef[]
User->>GtsJsonEntity: ExtractEntity(JsonObject)
GtsJsonEntity->>Extractor: IsJsonSchema()
Extractor-->>GtsJsonEntity: bool
GtsJsonEntity->>Extractor: ExtractId (with config priorities)
Extractor->>Extractor: FirstNonEmptyField (try EntityIdPropertyNames)
Extractor-->>GtsJsonEntity: ExtractResult {Id, SchemaId, Field}
GtsJsonEntity->>JsonWalker: ExtractReferences(JsonNode)
JsonWalker->>JsonWalker: Walk JSON tree recursively
loop For each $id, $ref, or schema field found
JsonWalker->>References: Track {Id, SourcePath}
end
JsonWalker-->>GtsJsonEntity: GtsReference[]
GtsJsonEntity-->>User: GtsJsonEntity {GtsId, SchemaId, Content, GtsRefs, Label}
sequenceDiagram
participant Client
participant Registry as GtsRegistry
participant Store as IGtsStore
participant Dict as Dictionary/<br/>ConcurrentDict
Client->>Registry: InMemory(config) / InMemoryThreadSafe(config)
Registry->>Registry: Factory method selects store type
alt Simple
Registry->>Store: new InMemoryGtsStore<Dictionary>()
else Thread-Safe
Registry->>Store: new InMemoryGtsStore<ConcurrentDict>()
end
Store->>Dict: Initialize with new()
Registry-->>Client: InMemoryGtsRegistry instance
Client->>Registry: SaveAsync(entity)
Registry->>Store: SaveAsync(entity)
Store->>Dict: Store/Update [id] = entity
Store-->>Registry: ValueTask
Registry-->>Client: ValueTask
Client->>Registry: GetAsync(id)
Registry->>Store: GetAsync(id)
Store->>Dict: Lookup [id]
Dict-->>Store: GtsJsonEntity?
Store-->>Registry: ValueTask<GtsJsonEntity?>
Registry-->>Client: ValueTask<GtsJsonEntity?>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 10
🧹 Nitpick comments (6)
Gts/Properties/AssemblyInfo.cs (1)
3-3: Strong-name signing is not currently enabled; the current code is correct.
Gtsis not strong-named (noSignAssembly,PublicSign, or key files present). The currentInternalsVisibleTodeclaration is correct for this configuration. If strong-name signing is introduced in the future, update Line 3 to include the full public key:[assembly: InternalsVisibleTo("Gts.Tests, PublicKey=...")].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Properties/AssemblyInfo.cs` at line 3, The Assembly attribute InternalsVisibleTo("Gts.Tests") in AssemblyInfo.cs is correct for an unsigned assembly; leave it unchanged now. If you later enable strong-name signing (e.g., add SignAssembly/PublicSign or a .snk key), update the assembly attribute to include the test assembly's full public key (change InternalsVisibleTo("Gts.Tests") to InternalsVisibleTo("Gts.Tests, PublicKey=...")) so the signed friend assembly matches.Gts/Parsing/ParseResult.cs (1)
3-11: Consider making the constructor private or clarifying failure semantics.The class has an implicit public constructor, allowing
new ParseResult()which won't equalSuccessand will convert tofalse. This creates ambiguity:
- If
Successshould be the only valid instance, make the constructor private to enforce the singleton pattern.- If failures need to carry details, consider adding explicit failure representation.
♻️ Option 1: Enforce singleton pattern
public sealed class ParseResult { public static readonly ParseResult Success = new(); + + private ParseResult() { } public static implicit operator bool(ParseResult result) { return result == Success; } }♻️ Option 2: Add explicit failure support
public sealed class ParseResult { public static readonly ParseResult Success = new(); + + public string? ErrorMessage { get; } + public bool IsSuccess => this == Success; + + private ParseResult() { } + + private ParseResult(string errorMessage) + { + ErrorMessage = errorMessage; + } + + public static ParseResult Failure(string message) => new(message); public static implicit operator bool(ParseResult result) { return result == Success; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Parsing/ParseResult.cs` around lines 3 - 11, Make the public constructor of ParseResult private to enforce the singleton pattern so the only default-success instance is ParseResult.Success; then, if you need to represent failures, add an explicit failure representation (e.g., a static factory like ParseResult.CreateFailure(...) that returns instances with properties such as IsSuccess and an optional ErrorMessage) and update the implicit operator bool to return result.IsSuccess instead of comparing to Success; this ensures no one can create ambiguous new ParseResult() instances and provides a clear way to carry failure details when needed.Gts/Parsing/ParseException.cs (1)
3-11: Consider providing a meaningful exception message.The exception doesn't pass a message to the base
Exceptionconstructor, soMessagewill be empty/default when caught. Consider adding a descriptive message:♻️ Proposed improvement
public class ParseException : Exception { public ParseResult ParseResult { get; } public ParseException(ParseResult parseResult) + : base("Failed to parse GTS identifier.") { ParseResult = parseResult; } + + public ParseException(string message, ParseResult parseResult) + : base(message) + { + ParseResult = parseResult; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Parsing/ParseException.cs` around lines 3 - 11, The ParseException constructor currently sets the ParseResult but does not pass a descriptive message to the base Exception, leaving Exception.Message empty; update the ParseException(ParseResult parseResult) constructor to call base(...) with a meaningful message (e.g., including parseResult.Status or a brief description) and still store the ParseResult property so callers can both read Message and access ParseResult for details.Gts.Store/IGtsStore.cs (1)
15-15: Remove commented-out interface members.Line 15 should be tracked in an issue instead of keeping a dead signature in the interface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts.Store/IGtsStore.cs` at line 15, Remove the dead commented-out interface member "ValueTask<int> ValidateAsync();" from IGtsStore (the commented line in the IGtsStore interface) and track this work as an issue/ticket instead; update the interface file by deleting that commented signature so the interface contains only active members and add a corresponding issue in your tracker describing the intended ValidateAsync proposal for future implementation.Gts/GtsIdSegment.cs (1)
11-11: Remove commented-out members.These placeholders are stale noise; move history to git and keep the class surface clean.
Also applies to: 21-21, 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/GtsIdSegment.cs` at line 11, Remove the stale commented-out member declarations in the GtsIdSegment class (e.g., the commented "string? segment," line and the other commented members present in the same class) so the class surface is clean; use git history if you need to recover the removed placeholders and ensure only active fields/properties/members (actual declarations used by GtsIdSegment) remain.Gts/Extraction/GtsExtract.cs (1)
242-255: Remove retained commented-out implementation block.The large commented section at Line 242–Line 255 is stale and makes this helper harder to maintain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Extraction/GtsExtract.cs` around lines 242 - 255, Remove the stale commented-out implementation block that iterates propertyNames and calls GetFieldValue/IsValidGtsId; specifically delete the entire commented section containing the two foreach blocks referencing propertyNames, GetFieldValue, and IsValidGtsId so the helper no longer retains dead commented code and the method (GtsExtract helper) remains clean and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Gts.Store/GtsRegistry.cs`:
- Around line 18-22: GtsRegistry.SaveAsync currently skips validation (TODO)
causing ValidateGtsReferences to be a no-op and allowing InMemoryGtsRegistry to
bypass reference checks; update SaveAsync to call the validation routine when
the config flag is enabled (e.g., call ValidateGtsReferences(entity) or the
configured validator before delegating to _store.SaveAsync) and make the
validator throw or return a clear error if references are invalid so SaveAsync
fails fast instead of silently saving; ensure the same behavior applies for
subclasses like InMemoryGtsRegistry by keeping the call in the base SaveAsync.
- Around line 12-16: The GtsRegistry constructor lacks null guards for its
dependencies; add defensive checks at the start of the protected
GtsRegistry(IGtsStore store, GtsRegistryConfig config) constructor to throw
ArgumentNullException for null store and null config before assigning to _store
and Config so failures occur fast (use the parameter names "store" and "config"
in the exceptions to aid diagnostics).
In `@Gts.Store/InMemory/InMemoryGtsStore.cs`:
- Around line 29-32: GetAllAsync currently returns the live enumerable
_entities.Values which can throw InvalidOperationException if the underlying
dictionary is modified; change GetAllAsync (the method named GetAllAsync in
InMemoryGtsStore) to materialize a snapshot (e.g., call ToList() or ToArray() on
_entities.Values) and return that concrete collection wrapped in
ValueTask.FromResult so callers receive a stable snapshot rather than a live
enumerable.
In `@Gts/Extraction/GtsExtract.cs`:
- Around line 220-240: FirstNonEmptyField currently iterates the JSON object's
keys, which makes selection depend on input serialization order; instead iterate
the provided propertyNames in their configured priority order and for each name
check if json contains that key, call GetFieldValue(json, name) and apply the
two-pass logic (first pass: non-empty and IsValidGtsId, second pass: any
non-empty) so the priority in the propertyNames list (GtsExtractOptions) is
honored; update the two foreach loops to iterate propertyNames
(IReadOnlyList<string> propertyNames) and use the key variable name exactly as
in the diff when calling GetFieldValue and IsValidGtsId.
- Around line 201-207: GetFieldValue currently calls node?.GetValue<string>()
which will throw for non-string JSON primitives; change this to use
TryGetValue<string>(out var s) (or equivalent safe extraction) on the
JsonNode/JsonValue so you only proceed when the value is actually a string and
non-empty, returning null otherwise; update both GetFieldValue and the similar
extraction sites used by ExtractEntity / ExtractReferences to perform the
TryGetValue check, handle null/whitespace consistently, and avoid assuming node
is string-typed to prevent InvalidOperationException at runtime.
In `@Gts/GtsId.cs`:
- Around line 80-85: TryParseInternal and the TryParse/TryParsePattern callers
currently call id.EndsWith("~") and pass pattern directly into
Parsers.GtsTypeId.Parse / Parsers.GtsInstanceId.Parse which will throw on null;
update TryParseInternal, TryParse and TryParsePattern to treat null/empty input
as a parse failure by first checking string.IsNullOrEmpty(id) or
string.IsNullOrEmpty(pattern) and returning a failed ParseResult (with result =
null) instead of calling EndsWith or the Parse methods, and only call
Parsers.GtsTypeId.Parse / Parsers.GtsInstanceId.Parse after confirming the input
is non-null; reference TryParseInternal, TryParse, TryParsePattern,
Parsers.GtsTypeId.Parse and Parsers.GtsInstanceId.Parse to locate the changes.
- Around line 143-149: The code currently allows non-wildcard patterns to match
prefixes because it calls MatchSegments(pattern.Segments, Segments) without
requiring equal segment counts; change the logic so that when pattern.Id
contains no '*' you first check pattern.Segments.Count == Segments.Count and
only then call MatchSegments; similarly, in the other branch (the
wildcard-handling block around MatchSegments invoked for patterns with a single
trailing '*') ensure you only allow prefix matching for patterns with a trailing
'*' and for non-wildcard cases enforce exact segment count—update the checks
around pattern.Id, pattern.Segments, Segments and calls to MatchSegments to
reflect these exact-vs-prefix rules.
- Around line 209-212: The Equals override in class GtsId incorrectly compares
the string property Id to the incoming obj; change GtsId.Equals(object? obj) to
detect if obj is a GtsId (and optionally a string) and compare the underlying Id
strings (e.g., return obj is GtsId other && Id == other.Id; or handle obj is
string). Also ensure GetHashCode for GtsId returns Id.GetHashCode() so
dictionary lookups are consistent with Equals.
In `@Gts/Parsing/Parsers.cs`:
- Line 33: The parser labels are incorrect: change the .Labelled usages so
VersionFull is labelled as nameof(VersionFull) instead of nameof(VersionMajor),
and change the .Labelled for IdentifierOrWildcard to use
nameof(IdentifierOrWildcard) instead of nameof(Identifier); locate the .Labelled
calls in Parsers.cs that reference VersionMajor and Identifier and update them
to the correct symbol names to make parse error messages accurate.
- Around line 108-121: The top-level parsers (GtsTypeId, GtsInstanceId,
GtsPattern) currently accept input with trailing garbage because they don't
require reaching End; update each parser expression by appending .Before(End) to
the final composed parser (e.g. the result of GtsPrefix.Then(Dot).Then(...)) so
that parsing fails on any unconsumed input; modify the definitions of GtsTypeId,
GtsInstanceId, and GtsPattern to end with .Before(End) while keeping the
existing use of Segment, Pattern, IdentifierInfo and IdentifierKind.
---
Nitpick comments:
In `@Gts.Store/IGtsStore.cs`:
- Line 15: Remove the dead commented-out interface member "ValueTask<int>
ValidateAsync();" from IGtsStore (the commented line in the IGtsStore interface)
and track this work as an issue/ticket instead; update the interface file by
deleting that commented signature so the interface contains only active members
and add a corresponding issue in your tracker describing the intended
ValidateAsync proposal for future implementation.
In `@Gts/Extraction/GtsExtract.cs`:
- Around line 242-255: Remove the stale commented-out implementation block that
iterates propertyNames and calls GetFieldValue/IsValidGtsId; specifically delete
the entire commented section containing the two foreach blocks referencing
propertyNames, GetFieldValue, and IsValidGtsId so the helper no longer retains
dead commented code and the method (GtsExtract helper) remains clean and
maintainable.
In `@Gts/GtsIdSegment.cs`:
- Line 11: Remove the stale commented-out member declarations in the
GtsIdSegment class (e.g., the commented "string? segment," line and the other
commented members present in the same class) so the class surface is clean; use
git history if you need to recover the removed placeholders and ensure only
active fields/properties/members (actual declarations used by GtsIdSegment)
remain.
In `@Gts/Parsing/ParseException.cs`:
- Around line 3-11: The ParseException constructor currently sets the
ParseResult but does not pass a descriptive message to the base Exception,
leaving Exception.Message empty; update the ParseException(ParseResult
parseResult) constructor to call base(...) with a meaningful message (e.g.,
including parseResult.Status or a brief description) and still store the
ParseResult property so callers can both read Message and access ParseResult for
details.
In `@Gts/Parsing/ParseResult.cs`:
- Around line 3-11: Make the public constructor of ParseResult private to
enforce the singleton pattern so the only default-success instance is
ParseResult.Success; then, if you need to represent failures, add an explicit
failure representation (e.g., a static factory like
ParseResult.CreateFailure(...) that returns instances with properties such as
IsSuccess and an optional ErrorMessage) and update the implicit operator bool to
return result.IsSuccess instead of comparing to Success; this ensures no one can
create ambiguous new ParseResult() instances and provides a clear way to carry
failure details when needed.
In `@Gts/Properties/AssemblyInfo.cs`:
- Line 3: The Assembly attribute InternalsVisibleTo("Gts.Tests") in
AssemblyInfo.cs is correct for an unsigned assembly; leave it unchanged now. If
you later enable strong-name signing (e.g., add SignAssembly/PublicSign or a
.snk key), update the assembly attribute to include the test assembly's full
public key (change InternalsVisibleTo("Gts.Tests") to
InternalsVisibleTo("Gts.Tests, PublicKey=...")) so the signed friend assembly
matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4b6ebc8-f18b-490c-91f7-3f280c25e2a4
📒 Files selected for processing (38)
.gitmodulesGts.Store/Gts.Store.csprojGts.Store/GtsRegistry.csGts.Store/GtsRegistryConfig.csGts.Store/IGtsStore.csGts.Store/InMemory/InMemoryGtsRegistry.csGts.Store/InMemory/InMemoryGtsStore.csGts.Tests/Extraction/ExtractBasicTests.csGts.Tests/Extraction/ExtractEntityTests.csGts.Tests/Extraction/ExtractSchemaTests.csGts.Tests/Gts.Tests.csprojGts.Tests/GtsIdSegmentTests.csGts.Tests/GtsIdTests.csGts.Tests/Matching/ExactMatchingTests.csGts.Tests/Matching/PatternMatchingTests.csGts.Tests/Parsing/IdentifierParserTests.csGts.Tests/Parsing/InstanceParserTests.csGts.Tests/Parsing/PatternParserTests.csGts.Tests/Parsing/SectionParserTests.csGts.Tests/Parsing/SegmentParserTests.csGts.Tests/Parsing/TypeParserTests.csGts.Tests/Parsing/VersionParserTests.csGts/Extraction/ExtractResult.csGts/Extraction/GtsExtract.csGts/Extraction/GtsExtractOptions.csGts/Extraction/GtsJsonEntity.csGts/Extraction/GtsReference.csGts/Gts.csprojGts/GtsId.csGts/GtsIdSegment.csGts/Parsing/ParseException.csGts/Parsing/ParseResult.csGts/Parsing/Parsers.csGts/Properties/AssemblyInfo.csGts/Utils/GuidUtils.csREADME.mdglobal.jsongts-dotnet.sln
Comment on lines
+18
to
+22
| public ValueTask SaveAsync(GtsJsonEntity entity) | ||
| { | ||
| // TODO: validation logic | ||
| return _store.SaveAsync(entity); | ||
| } |
There was a problem hiding this comment.
ValidateGtsReferences is currently a no-op.
Line 20 leaves validation unimplemented, but the API exposes a config flag for it. Since InMemoryGtsRegistry (Gts.Store/InMemory/InMemoryGtsRegistry.cs, Lines 6-23) does not override SaveAsync, validation is silently skipped for all current registry variants.
🔧 Minimal fail-fast patch to avoid silent bypass
public ValueTask SaveAsync(GtsJsonEntity entity)
{
- // TODO: validation logic
+ ArgumentNullException.ThrowIfNull(entity);
+ if (Config.ValidateGtsReferences)
+ {
+ throw new NotSupportedException(
+ "ValidateGtsReferences is enabled, but reference validation is not implemented yet.");
+ }
return _store.SaveAsync(entity);
}If you want, I can draft a concrete reference-validation implementation next (and you can track it as a follow-up issue).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public ValueTask SaveAsync(GtsJsonEntity entity) | |
| { | |
| // TODO: validation logic | |
| return _store.SaveAsync(entity); | |
| } | |
| public ValueTask SaveAsync(GtsJsonEntity entity) | |
| { | |
| ArgumentNullException.ThrowIfNull(entity); | |
| if (Config.ValidateGtsReferences) | |
| { | |
| throw new NotSupportedException( | |
| "ValidateGtsReferences is enabled, but reference validation is not implemented yet."); | |
| } | |
| return _store.SaveAsync(entity); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gts.Store/GtsRegistry.cs` around lines 18 - 22, GtsRegistry.SaveAsync
currently skips validation (TODO) causing ValidateGtsReferences to be a no-op
and allowing InMemoryGtsRegistry to bypass reference checks; update SaveAsync to
call the validation routine when the config flag is enabled (e.g., call
ValidateGtsReferences(entity) or the configured validator before delegating to
_store.SaveAsync) and make the validator throw or return a clear error if
references are invalid so SaveAsync fails fast instead of silently saving;
ensure the same behavior applies for subclasses like InMemoryGtsRegistry by
keeping the call in the base SaveAsync.
Comment on lines
+201
to
+207
| private static string? GetFieldValue(JsonObject json, string propertyName) | ||
| { | ||
| if (!json.TryGetPropertyValue(propertyName, out var node)) | ||
| return null; | ||
|
|
||
| var s = node?.GetValue<string>(); | ||
| if (string.IsNullOrWhiteSpace(s)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the code at the specified lines and check for unguarded string extraction
# First, let's see the current state of the file around the mentioned lines
echo "=== Code around lines 201-207 ==="
sed -n '200,210p' Gts/Extraction/GtsExtract.cs
echo ""
echo "=== Code around lines 269-274 ==="
sed -n '268,280p' Gts/Extraction/GtsExtract.cs
echo ""
echo "=== All GetValue<string>() calls ==="
rg -n 'GetValue<\s*string\s*>\(\)' Gts/Extraction/GtsExtract.cs -C2
echo ""
echo "=== All TryGetValue<string>() calls ==="
rg -n 'TryGetValue<\s*string\s*>\(' Gts/Extraction/GtsExtract.cs -C2Repository: GlobalTypeSystem/gts-dotnet
Length of output: 1357
🏁 Script executed:
#!/bin/bash
# Get broader context of both methods
echo "=== GetFieldValue method (lines 201-215) ==="
sed -n '201,215p' Gts/Extraction/GtsExtract.cs
echo ""
echo "=== WalkAndCollectRefs method containing line 271 (lines 265-285) ==="
sed -n '265,285p' Gts/Extraction/GtsExtract.cs
echo ""
echo "=== Check if there's a JsonValue import and what it refers to ==="
head -30 Gts/Extraction/GtsExtract.cs | grep -E 'using|JsonValue'
echo ""
echo "=== Check full imports ==="
grep -n 'using' Gts/Extraction/GtsExtract.csRepository: GlobalTypeSystem/gts-dotnet
Length of output: 1806
🌐 Web query:
System.Text.Json JsonValue.GetValue<T> throws exception non-string type
💡 Result:
JsonValue.GetValue<T>() does not “parse” or do broad type conversions. It will only succeed when the underlying node already represents a compatible JSON primitive kind (number/bool/string/null) for T. If the node is a JSON string (e.g., "123") and you ask for GetValue<int>(), it throws InvalidOperationException (“unable to convert … String to Int32”). The same happens if you accidentally built a node from a string containing JSON—it stays a JSON string, not an object/number/etc. (source.dot.net)
Fix options
- If the value is actually a JSON string and you want a number/bool: get the string then parse yourself:
var s = node.GetValue<string>(); var i = int.Parse(s, CultureInfo.InvariantCulture);
- If the string contains JSON text: parse it first:
var parsed = JsonNode.Parse(node.GetValue<string>());
- Prefer non-throwing access + inspect kind:
jsonValue.TryGetValue(out int i)/jsonNode.GetValueKind()to branch correctly. (dev.to)
- When creating nodes, avoid implicit “string -> JsonNode” if you meant JSON content; use
JsonNode.Parse(jsonText)or create typed values (e.g.,JsonValue.Create(123)/JsonSerializer.SerializeToNode(value)). (blog.json-everything.net)
Citations:
- 1: https://source.dot.net/System.Text.Json/R/a927ec755eb0e0d0.html?utm_source=openai
- 2: https://dev.to/vparab/working-with-jsonobject-jsonnode-jsonvalue-and-jsonarray-systemtextjson-api-5b8l?utm_source=openai
- 3: https://blog.json-everything.net/posts/a-common-problem/?utm_source=openai
Guard string extraction from JsonValue to prevent runtime failures.
Lines 206 and 271 use unguarded GetValue<string>() calls that will throw InvalidOperationException when the JSON contains non-string primitives (numbers, booleans, null). This breaks ExtractEntity and ExtractReferences on valid JSON payloads.
Replace with TryGetValue<string>() for safe type checking:
Fix
private static string? GetFieldValue(JsonObject json, string propertyName)
{
if (!json.TryGetPropertyValue(propertyName, out var node))
return null;
-
- var s = node?.GetValue<string>();
- if (string.IsNullOrWhiteSpace(s))
+
+ if (node is not JsonValue value || !value.TryGetValue<string>(out var s) || string.IsNullOrWhiteSpace(s))
return null;
var trimmed = s!.Trim();
if (trimmed.Length == 0)
return null;
@@
private static void WalkAndCollectRefs(JsonNode? node, string path, List<GtsReference> refs, HashSet<string> seen)
{
@@
if (node is JsonValue value)
{
- var str = value.GetValue<string>();
-
- if (!string.IsNullOrEmpty(str) && IsValidGtsId(str))
+ if (value.TryGetValue<string>(out var str) && !string.IsNullOrEmpty(str) && IsValidGtsId(str))
{
var sourcePath = string.IsNullOrEmpty(path) ? "root" : path;
var key = str + "|" + sourcePath;
if (seen.Add(key))
refs.Add(new GtsReference(str, sourcePath));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gts/Extraction/GtsExtract.cs` around lines 201 - 207, GetFieldValue currently
calls node?.GetValue<string>() which will throw for non-string JSON primitives;
change this to use TryGetValue<string>(out var s) (or equivalent safe
extraction) on the JsonNode/JsonValue so you only proceed when the value is
actually a string and non-empty, returning null otherwise; update both
GetFieldValue and the similar extraction sites used by ExtractEntity /
ExtractReferences to perform the TryGetValue check, handle null/whitespace
consistently, and avoid assuming node is string-typed to prevent
InvalidOperationException at runtime.
Contributor
Claude Opus 4.6 Code Review SummaryPR: Add basic ID classes and parsing Critical Issues (must fix before merge)1.
|
| Severity | Count | Blocking? |
|---|---|---|
| Critical | 5 | Yes |
| Major | 1 | Recommended |
| Minor | 5 | No |
Recommended fix order:
GtsId.Equals— blocks all store functionalityTryParsenull guard — standard API safety contractGetValue<string>()— runtime crash on real-world JSON- Parser
.Before(End)— silently accepts invalid input MatchSegmentsexact matching — incorrect resultsFirstNonEmptyFielditeration order — silent priority violation
Contributor
|
I didn’t see the CLI tool, web server, and the gts-spec submodule in the code. These are required for testing the implementation via pytest + REST API. This is done consistently across all implementations (gts-rust, gts-go, gts-ts, gts-python). See here: You can checkout, say, https://github.com/GlobalTypeSystem/gts-rust and ask LLM to explain the Makefile, see: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Gts/GtsId.cs (2)
13-13:⚠️ Potential issue | 🟡 MinorEnforce
MaxLengthbefore invoking the parser.
GtsId.MaxLengthis public, but neither parse path checks it, so oversized inputs still go through the full parser/allocation path. If this limit is part of the contract, rejectid.Length > MaxLengthandpattern.Length > MaxLengthup front.Suggested fix
private static ParseResult TryParseInternal(string? id, out GtsId? result) { if (id is null) { result = null; return ParseResult.ArgumentIsNull; } + if (id.Length > MaxLength) + { + result = null; + return new ParseResult(); + } var (parseResult, isType) = id.EndsWith("~") ? (Parsers.GtsTypeId.Parse(id), true) : (Parsers.GtsInstanceId.Parse(id), false); ... private static ParseResult TryParsePatternInternal(string? pattern, out GtsId? result) { if (pattern is null) { result = null; return ParseResult.ArgumentIsNull; } + if (pattern.Length > MaxLength) + { + result = null; + return new ParseResult(); + } var parseResult = Parsers.GtsPattern.Parse(pattern);Also applies to: 80-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/GtsId.cs` at line 13, Reject inputs longer than GtsId.MaxLength before invoking the heavy parse/allocation path: add upfront length checks in the public parse entry points (e.g., GtsId.Parse, GtsId.TryParse and any factory/constructor wrappers that accept id or pattern strings) to return failure or throw when id.Length > GtsId.MaxLength or pattern.Length > GtsId.MaxLength; do this at the start of those methods so oversized inputs never reach the internal parser logic or allocation-heavy routines.
150-161:⚠️ Potential issue | 🔴 CriticalRequire exact segment counts for non-wildcard matches.
A concrete pattern like
a~bstill matchesa~b~cbecause the non-wildcard branch delegates straight toMatchSegments, andMatchSegmentsonly rejects when the pattern is longer than the candidate. Non-wildcard patterns need an equal-count check first.Suggested fix
public bool Matches(GtsId pattern) { // TODO: this logic is probably flawed, check if (pattern is null) return false; if (!pattern.Id.Contains('*')) - return MatchSegments(pattern.Segments, Segments); + return pattern.Segments.Count == Segments.Count + && MatchSegments(pattern.Segments, Segments); if (pattern.Id.Count(c => c == '*') > 1 || !pattern.Id.EndsWith('*')) return false; return MatchSegments(pattern.Segments, Segments); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/GtsId.cs` around lines 150 - 161, The Matches method in GtsId lets a concrete pattern like a~b match a~b~c because when pattern.Id contains no '*' it calls MatchSegments without enforcing equal segment counts; update Matches so that when pattern.Id contains no wildcard you first check pattern.Segments.Length == Segments.Length and return false if not equal, then call MatchSegments; keep existing wildcard handling (including the single trailing '*' restriction and the same MatchSegments call) intact so only non-wildcard patterns require exact segment count.Gts/Parsing/Parsers.cs (1)
52-54:⚠️ Potential issue | 🟡 MinorFix the
IdentifierOrWildcardparser label.This still reports failures as
Identifier, which makes parse diagnostics misleading when the wildcard branch is involved.Suggested fix
internal static readonly Parser<char, string> IdentifierOrWildcard = Wildcard.Or(Identifier) - .Labelled(nameof(Identifier)); + .Labelled(nameof(IdentifierOrWildcard));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Parsing/Parsers.cs` around lines 52 - 54, The parser IdentifierOrWildcard is labelled as nameof(Identifier) causing failures from the Wildcard branch to be reported as "Identifier"; change the label call on the combined parser (Wildcard.Or(Identifier).Labelled(...)) to use nameof(IdentifierOrWildcard) (i.e., label the combined parser itself) so parse diagnostics correctly report the IdentifierOrWildcard alternative.Gts.Store/GtsRegistry.cs (1)
21-24:⚠️ Potential issue | 🟠 MajorDon’t silently ignore
ValidateGtsReferences.When
Config.ValidateGtsReferencesis enabled,SaveAsyncstill writes straight to the store. That means every current registry implementation can accept invalid references even though the flag is on. Call validation here, or fail fast until it exists.Minimal fail-fast patch
public ValueTask SaveAsync(GtsJsonEntity entity) { - // TODO: validation logic + ArgumentNullException.ThrowIfNull(entity); + if (Config.ValidateGtsReferences) + { + throw new NotSupportedException( + "ValidateGtsReferences is enabled, but reference validation is not implemented."); + } return _store.SaveAsync(entity); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts.Store/GtsRegistry.cs` around lines 21 - 24, GtsRegistry.SaveAsync currently writes directly to _store and ignores Config.ValidateGtsReferences; update SaveAsync in class GtsRegistry to check Config.ValidateGtsReferences and call ValidateGtsReferences(entity) before delegating to _store.SaveAsync, and if validation fails throw/return a failed task so the save fails fast; if ValidateGtsReferences is not yet implemented, make SaveAsync return an immediate failure when the flag is enabled (e.g., throw InvalidOperationException or return a faulted ValueTask) rather than persisting invalid data.
🧹 Nitpick comments (3)
Gts/Parsing/ParseResult.cs (1)
3-11: Hide directParseResultconstruction.
implicit boolonly treats the exactSuccesssingleton as truthy, but this public class still has an implicit public parameterless constructor. That makes it easy to create opaque result instances with no defined state. Making construction private/internal and exposing named singleton/factory members would keep the contract explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Parsing/ParseResult.cs` around lines 3 - 11, The ParseResult class currently exposes a public parameterless constructor allowing arbitrary instances; make the constructor non-public (private or internal) to prevent external construction and restrict instances to the named singletons/factories (e.g., keep/retain the static readonly ParseResult Success and ParseResult ArgumentIsNull and any future factory methods); update/confirm the implicit operator bool(ParseResult) continues to compare to ParseResult.Success and that any code creating new results uses provided singletons or new explicit factory members instead of direct construction.Gts.Store/IGtsStore.cs (1)
11-11: Prefer a read-only return type forGetAllAsync.The current implementations return snapshots, so
IList<GtsJsonEntity>suggests mutability that the store does not actually honor.IReadOnlyList<GtsJsonEntity>orIReadOnlyCollection<GtsJsonEntity>would make the contract clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts.Store/IGtsStore.cs` at line 11, The interface method GetAllAsync currently returns a mutable IList<GtsJsonEntity>; change its return type to a read-only collection such as IReadOnlyList<GtsJsonEntity> (or IReadOnlyCollection<GtsJsonEntity>) in IGtsStore's GetAllAsync signature so the contract reflects snapshots are immutable; update any implementing classes/methods that implement GetAllAsync to match the new return type (preserve returning snapshots but cast/convert to the chosen IReadOnly... type).Gts/GtsIdSegment.cs (1)
8-52: Consider makingGtsIdSegmentimmutable.This sits inside a value-object graph, but the internal parameterless ctor plus internal setters still allow post-parse mutation from anywhere in the assembly. A constructor-only/
init-only shape would makeGtsIdsafer to cache and compare.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/GtsIdSegment.cs` around lines 8 - 52, Make GtsIdSegment immutable by removing the internal parameterless ctor and changing the mutable properties (Vendor, Package, Namespace, Type, VersionMajor, VersionMinor, IsType, IsWildcard) to be constructor-assigned only (use init-only properties or get-only properties set from the existing internal GtsIdSegment(...) ctor). Keep the internal multi-parameter constructor as the sole way to create instances, update any internal callers to supply values there, and remove/comment out the parameterless ctor and internal setters so instances cannot be mutated post-construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Gts.Store/GtsRegistry.cs`:
- Around line 21-24: GtsRegistry.SaveAsync currently writes directly to _store
and ignores Config.ValidateGtsReferences; update SaveAsync in class GtsRegistry
to check Config.ValidateGtsReferences and call ValidateGtsReferences(entity)
before delegating to _store.SaveAsync, and if validation fails throw/return a
failed task so the save fails fast; if ValidateGtsReferences is not yet
implemented, make SaveAsync return an immediate failure when the flag is enabled
(e.g., throw InvalidOperationException or return a faulted ValueTask) rather
than persisting invalid data.
In `@Gts/GtsId.cs`:
- Line 13: Reject inputs longer than GtsId.MaxLength before invoking the heavy
parse/allocation path: add upfront length checks in the public parse entry
points (e.g., GtsId.Parse, GtsId.TryParse and any factory/constructor wrappers
that accept id or pattern strings) to return failure or throw when id.Length >
GtsId.MaxLength or pattern.Length > GtsId.MaxLength; do this at the start of
those methods so oversized inputs never reach the internal parser logic or
allocation-heavy routines.
- Around line 150-161: The Matches method in GtsId lets a concrete pattern like
a~b match a~b~c because when pattern.Id contains no '*' it calls MatchSegments
without enforcing equal segment counts; update Matches so that when pattern.Id
contains no wildcard you first check pattern.Segments.Length == Segments.Length
and return false if not equal, then call MatchSegments; keep existing wildcard
handling (including the single trailing '*' restriction and the same
MatchSegments call) intact so only non-wildcard patterns require exact segment
count.
In `@Gts/Parsing/Parsers.cs`:
- Around line 52-54: The parser IdentifierOrWildcard is labelled as
nameof(Identifier) causing failures from the Wildcard branch to be reported as
"Identifier"; change the label call on the combined parser
(Wildcard.Or(Identifier).Labelled(...)) to use nameof(IdentifierOrWildcard)
(i.e., label the combined parser itself) so parse diagnostics correctly report
the IdentifierOrWildcard alternative.
---
Nitpick comments:
In `@Gts.Store/IGtsStore.cs`:
- Line 11: The interface method GetAllAsync currently returns a mutable
IList<GtsJsonEntity>; change its return type to a read-only collection such as
IReadOnlyList<GtsJsonEntity> (or IReadOnlyCollection<GtsJsonEntity>) in
IGtsStore's GetAllAsync signature so the contract reflects snapshots are
immutable; update any implementing classes/methods that implement GetAllAsync to
match the new return type (preserve returning snapshots but cast/convert to the
chosen IReadOnly... type).
In `@Gts/GtsIdSegment.cs`:
- Around line 8-52: Make GtsIdSegment immutable by removing the internal
parameterless ctor and changing the mutable properties (Vendor, Package,
Namespace, Type, VersionMajor, VersionMinor, IsType, IsWildcard) to be
constructor-assigned only (use init-only properties or get-only properties set
from the existing internal GtsIdSegment(...) ctor). Keep the internal
multi-parameter constructor as the sole way to create instances, update any
internal callers to supply values there, and remove/comment out the
parameterless ctor and internal setters so instances cannot be mutated
post-construction.
In `@Gts/Parsing/ParseResult.cs`:
- Around line 3-11: The ParseResult class currently exposes a public
parameterless constructor allowing arbitrary instances; make the constructor
non-public (private or internal) to prevent external construction and restrict
instances to the named singletons/factories (e.g., keep/retain the static
readonly ParseResult Success and ParseResult ArgumentIsNull and any future
factory methods); update/confirm the implicit operator bool(ParseResult)
continues to compare to ParseResult.Success and that any code creating new
results uses provided singletons or new explicit factory members instead of
direct construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a27769e-e159-4fbe-826d-e06e803c9b78
📒 Files selected for processing (8)
Gts.Store/GtsRegistry.csGts.Store/IGtsStore.csGts.Store/InMemory/InMemoryGtsStore.csGts.Tests/GtsIdTests.csGts/GtsId.csGts/GtsIdSegment.csGts/Parsing/ParseResult.csGts/Parsing/Parsers.cs
There was a problem hiding this comment.
♻️ Duplicate comments (5)
Gts/Extraction/GtsExtract.cs (3)
231-268:⚠️ Potential issue | 🟠 MajorIterate
propertyNamesin priority order, not JSON keys.Lines 233-251 iterate
jsonkeys and check membership inpropertyNames, making field selection depend on JSON serialization order rather than the configured priority inGtsExtractOptions. The commented-out code at lines 254-266 shows the correct approach.🔧 Proposed fix — iterate propertyNames in priority order
private static (string? field, string? value) FirstNonEmptyField(JsonObject json, IReadOnlyList<string> propertyNames) { - foreach (var (key, _) in json) - { - if (propertyNames.Contains(key)) - { - var val = GetFieldValue(json, key); - if (!string.IsNullOrEmpty(val) && IsValidGtsId(val)) - return (key, val); - } - } - - foreach (var (key, _) in json) - { - if (propertyNames.Contains(key)) - { - var val = GetFieldValue(json, key); - if (!string.IsNullOrEmpty(val)) - return (key, val); - } - } + foreach (var name in propertyNames) + { + var val = GetFieldValue(json, name); + if (!string.IsNullOrEmpty(val) && IsValidGtsId(val)) + return (name, val); + } + + foreach (var name in propertyNames) + { + var val = GetFieldValue(json, name); + if (!string.IsNullOrEmpty(val)) + return (name, val); + } return (null, null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Extraction/GtsExtract.cs` around lines 231 - 268, FirstNonEmptyField currently iterates the JsonObject keys which makes selection depend on JSON order; change it to iterate the provided propertyNames in priority order instead: for each name in propertyNames call GetFieldValue(name) and on the first non-empty value that passes IsValidGtsId return (name, value), and if none match in that first pass do a second pass over propertyNames returning the first non-empty value; remove the two loops that iterate json keys and use propertyNames loops so priority from GtsExtractOptions is respected.
276-293:⚠️ Potential issue | 🔴 CriticalGuard
GetValue<string>()inWalkAndCollectRefs.Line 282 calls
value.GetValue<string>()without type-checking, which throws for non-string JSON primitives encountered during tree traversal.🔧 Proposed fix
if (node is JsonValue value) { - var str = value.GetValue<string>(); - - if (!string.IsNullOrEmpty(str) && IsValidGtsId(str)) + if (value.TryGetValue<string>(out var str) && !string.IsNullOrEmpty(str) && IsValidGtsId(str)) { var sourcePath = string.IsNullOrEmpty(path) ? "root" : path; var key = str + "|" + sourcePath; if (seen.Add(key)) refs.Add(new GtsReference(str, sourcePath)); } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Extraction/GtsExtract.cs` around lines 276 - 293, WalkAndCollectRefs calls JsonValue.GetValue<string>() unguarded which throws for non-string primitives; change the logic in WalkAndCollectRefs to use JsonValue.TryGetValue<string>(out var str) (or otherwise inspect value's ValueKind) and only treat it as a candidate when TryGetValue returns true and str is not null/empty, then continue to use IsValidGtsId(str) and the existing seen/ref addition logic; ensure you reference the JsonValue instance named value and keep the sourcePath/seen key behavior unchanged.
212-228:⚠️ Potential issue | 🔴 CriticalGuard
GetValue<string>()to prevent runtime exceptions on non-string primitives.Line 217 calls
node?.GetValue<string>()which throwsInvalidOperationExceptionwhen the JSON value is a number, boolean, or null. This breaks extraction on valid JSON payloads.🔧 Proposed fix
private static string? GetFieldValue(JsonObject json, string propertyName) { if (!json.TryGetPropertyValue(propertyName, out var node)) return null; - - var s = node?.GetValue<string>(); - if (string.IsNullOrWhiteSpace(s)) + + if (node is not JsonValue value || !value.TryGetValue<string>(out var s) || string.IsNullOrWhiteSpace(s)) return null; - var trimmed = s!.Trim(); + var trimmed = s.Trim(); if (trimmed.Length == 0) return null; if (propertyName == "$id" && trimmed.StartsWith(GtsUriPrefix, StringComparison.Ordinal)) trimmed = trimmed.Substring(GtsUriPrefix.Length); return trimmed; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Extraction/GtsExtract.cs` around lines 212 - 228, GetFieldValue currently calls node?.GetValue<string>() which throws InvalidOperationException for non-string JSON primitives; update GetFieldValue to guard that call by first verifying the JsonNode is a JsonValue whose underlying value is a string (or use TryGetValue<string>) before calling GetValue<string>(), and for non-string primitives either return null (to preserve current behavior) or convert safely via ToString(); ensure you modify the logic around the node?.GetValue<string() call inside GetFieldValue so it never calls GetValue<string>() on number/boolean/null nodes.Gts/GtsId.cs (1)
150-161:⚠️ Potential issue | 🔴 CriticalNon-wildcard patterns allow prefix matches instead of requiring exact segment count.
When
pattern.Idcontains no*, Line 156 callsMatchSegmentswithout verifying segment counts are equal. Combined with Line 179 (patternSegs.Count > candidateSegs.Count), a shorter non-wildcard pattern incorrectly matches a longer candidate.🔧 Proposed fix
public bool Matches(GtsId pattern) { // TODO: this logic is probably flawed, check if (pattern is null) return false; if (!pattern.Id.Contains('*')) + { + if (pattern.Segments.Count != Segments.Count) return false; return MatchSegments(pattern.Segments, Segments); + } if (pattern.Id.Count(c => c == '*') > 1 || !pattern.Id.EndsWith('*')) return false; return MatchSegments(pattern.Segments, Segments); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/GtsId.cs` around lines 150 - 161, The Matches method currently allows non-wildcard patterns to match candidates with differing segment counts; update Matches (in GtsId) so that when pattern.Id contains no '*' you first check pattern.Segments.Count == Segments.Count and return false if they differ, then call MatchSegments; keep the existing wildcard validation (no more than one '*' and must end with '*') and existing call to MatchSegments for wildcard cases; reference the Matches method, pattern.Id, MatchSegments, Segments, and the patternSegs/candidateSegs count logic to locate where to add the equality check.Gts/Parsing/Parsers.cs (1)
53-55:⚠️ Potential issue | 🟡 MinorParser label still incorrect.
Line 55 labels
IdentifierOrWildcardasnameof(Identifier), making parse error messages misleading. This was flagged in a previous review and marked as fixed, but the label remains incorrect.🔧 Proposed fix
internal static readonly Parser<char, string> IdentifierOrWildcard = Wildcard.Or(Identifier) - .Labelled(nameof(Identifier)); + .Labelled(nameof(IdentifierOrWildcard));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Parsing/Parsers.cs` around lines 53 - 55, The parser label is incorrect: change the Labelled call on IdentifierOrWildcard (currently using nameof(Identifier)) to use the correct symbol name (e.g., nameof(IdentifierOrWildcard) or the literal "IdentifierOrWildcard") so that the composed parser Wildcard.Or(Identifier).Labelled(...) reports accurate labels in error messages; update the Labelled argument where IdentifierOrWildcard is defined to reference IdentifierOrWildcard instead of Identifier.
🧹 Nitpick comments (2)
Gts/Parsing/ParseResult.cs (1)
7-9: Add aFailuresentinel for parse failures.The class defines
SuccessandArgumentIsNullsingletons, but callers inGtsId.TryParseInternal(Line 96) andTryParsePatternInternal(Line 126) constructnew ParseResult()for parse failures. While these instances correctly evaluate tofalse, this violates the singleton pattern and prevents future error-message enrichment.♻️ Proposed fix
/// <summary>Indicates successful parse.</summary> public static readonly ParseResult Success = new(); /// <summary>Indicates null input was passed.</summary> public static readonly ParseResult ArgumentIsNull = new(); // TODO: actual error message + /// <summary>Indicates a parse failure (syntax or structure error).</summary> + public static readonly ParseResult Failure = new(); // TODO: actual error messageThen update
GtsId.csto useParseResult.Failureinstead ofnew ParseResult().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/Parsing/ParseResult.cs` around lines 7 - 9, Add a new ParseResult singleton named Failure (similar to Success and ArgumentIsNull) and replace any places that construct new ParseResult() with that sentinel; specifically add public static readonly ParseResult Failure = new(); to the ParseResult class and update callers in GtsId.TryParseInternal and TryParsePatternInternal to return ParseResult.Failure instead of new ParseResult() so failures use the shared sentinel and can later carry enriched error messages.Gts/GtsId.cs (1)
219-225: Consider implementingIEquatable<GtsId>for dictionary performance.
GtsIdis used as a key inDictionary<GtsId, GtsJsonEntity>andConcurrentDictionary<GtsId, GtsJsonEntity>(seeInMemoryGtsStoreandInMemoryGtsRegistry). WithoutIEquatable<GtsId>, the default equality comparer falls back toEquals(object?), causing boxing on every lookup.♻️ Proposed enhancement
-public sealed class GtsId +public sealed class GtsId : IEquatable<GtsId> { // ... existing members ... + /// <summary>Determines whether this GtsId equals another.</summary> + public bool Equals(GtsId? other) + => other is not null && string.Equals(Id, other.Id, StringComparison.Ordinal); + /// <inheritdoc/> public override bool Equals(object? obj) - => obj is GtsId id && string.Equals(Id, id.Id, StringComparison.Ordinal); + => Equals(obj as GtsId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/GtsId.cs` around lines 219 - 225, GtsId is used as dictionary keys but doesn't implement IEquatable<GtsId>, causing boxing; implement IEquatable<GtsId> on the GtsId type and add a strongly-typed Equals(GtsId? other) method that compares Id with StringComparison.Ordinal, then have the existing Equals(object? obj) delegate to that typed Equals; keep the current GetHashCode logic (using StringComparer.Ordinal.GetHashCode(Id)) and ensure null-safety for Id in both Equals(GtsId) and GetHashCode to avoid NREs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Gts/Extraction/GtsExtract.cs`:
- Around line 231-268: FirstNonEmptyField currently iterates the JsonObject keys
which makes selection depend on JSON order; change it to iterate the provided
propertyNames in priority order instead: for each name in propertyNames call
GetFieldValue(name) and on the first non-empty value that passes IsValidGtsId
return (name, value), and if none match in that first pass do a second pass over
propertyNames returning the first non-empty value; remove the two loops that
iterate json keys and use propertyNames loops so priority from GtsExtractOptions
is respected.
- Around line 276-293: WalkAndCollectRefs calls JsonValue.GetValue<string>()
unguarded which throws for non-string primitives; change the logic in
WalkAndCollectRefs to use JsonValue.TryGetValue<string>(out var str) (or
otherwise inspect value's ValueKind) and only treat it as a candidate when
TryGetValue returns true and str is not null/empty, then continue to use
IsValidGtsId(str) and the existing seen/ref addition logic; ensure you reference
the JsonValue instance named value and keep the sourcePath/seen key behavior
unchanged.
- Around line 212-228: GetFieldValue currently calls node?.GetValue<string>()
which throws InvalidOperationException for non-string JSON primitives; update
GetFieldValue to guard that call by first verifying the JsonNode is a JsonValue
whose underlying value is a string (or use TryGetValue<string>) before calling
GetValue<string>(), and for non-string primitives either return null (to
preserve current behavior) or convert safely via ToString(); ensure you modify
the logic around the node?.GetValue<string() call inside GetFieldValue so it
never calls GetValue<string>() on number/boolean/null nodes.
In `@Gts/GtsId.cs`:
- Around line 150-161: The Matches method currently allows non-wildcard patterns
to match candidates with differing segment counts; update Matches (in GtsId) so
that when pattern.Id contains no '*' you first check pattern.Segments.Count ==
Segments.Count and return false if they differ, then call MatchSegments; keep
the existing wildcard validation (no more than one '*' and must end with '*')
and existing call to MatchSegments for wildcard cases; reference the Matches
method, pattern.Id, MatchSegments, Segments, and the patternSegs/candidateSegs
count logic to locate where to add the equality check.
In `@Gts/Parsing/Parsers.cs`:
- Around line 53-55: The parser label is incorrect: change the Labelled call on
IdentifierOrWildcard (currently using nameof(Identifier)) to use the correct
symbol name (e.g., nameof(IdentifierOrWildcard) or the literal
"IdentifierOrWildcard") so that the composed parser
Wildcard.Or(Identifier).Labelled(...) reports accurate labels in error messages;
update the Labelled argument where IdentifierOrWildcard is defined to reference
IdentifierOrWildcard instead of Identifier.
---
Nitpick comments:
In `@Gts/GtsId.cs`:
- Around line 219-225: GtsId is used as dictionary keys but doesn't implement
IEquatable<GtsId>, causing boxing; implement IEquatable<GtsId> on the GtsId type
and add a strongly-typed Equals(GtsId? other) method that compares Id with
StringComparison.Ordinal, then have the existing Equals(object? obj) delegate to
that typed Equals; keep the current GetHashCode logic (using
StringComparer.Ordinal.GetHashCode(Id)) and ensure null-safety for Id in both
Equals(GtsId) and GetHashCode to avoid NREs.
In `@Gts/Parsing/ParseResult.cs`:
- Around line 7-9: Add a new ParseResult singleton named Failure (similar to
Success and ArgumentIsNull) and replace any places that construct new
ParseResult() with that sentinel; specifically add public static readonly
ParseResult Failure = new(); to the ParseResult class and update callers in
GtsId.TryParseInternal and TryParsePatternInternal to return ParseResult.Failure
instead of new ParseResult() so failures use the shared sentinel and can later
carry enriched error messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60cd8a8a-2cd6-4e85-a4cd-eba7cd89899b
📒 Files selected for processing (13)
Gts.Store/GtsRegistry.csGts.Store/GtsRegistryConfig.csGts.Store/IGtsStore.csGts.Store/InMemory/InMemoryGtsRegistry.csGts.Store/InMemory/InMemoryGtsStore.csGts/Extraction/GtsExtract.csGts/Extraction/GtsJsonEntity.csGts/GtsId.csGts/GtsIdSegment.csGts/Parsing/ParseException.csGts/Parsing/ParseResult.csGts/Parsing/Parsers.csGts/Utils/GuidUtils.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Gts/Parsing/ParseException.cs
- Gts.Store/IGtsStore.cs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Gts/GtsId.cs (1)
33-39: BackSegmentswith an actual read-only collection.The property exposes
IReadOnlyCollection<GtsIdSegment>but stores the mutableList<T>passed from the parse paths. Consumers can downcast the runtime type toList<T>and mutateSegments, which undermines the value-object invariant used byMatches().Wrap the incoming collection in
ReadOnlyCollection<T>to prevent downcasting:♻️ Proposed refactor
- public IReadOnlyCollection<GtsIdSegment> Segments { get; } + public IReadOnlyList<GtsIdSegment> Segments { get; } internal GtsId(string id, IReadOnlyCollection<GtsIdSegment> segments) { Id = id; - Segments = segments; + Segments = segments.ToList().AsReadOnly(); }Also applies to: 102–107, 132–135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Gts/GtsId.cs` around lines 33 - 39, The Segments property is exposed as IReadOnlyCollection<GtsIdSegment> but currently stores the mutable collection passed in, allowing callers to downcast and mutate it; in each GtsId constructor (e.g. internal GtsId(string id, IReadOnlyCollection<GtsIdSegment> segments) and the other constructors referenced) wrap the incoming collection in a ReadOnlyCollection<GtsIdSegment> and assign that to Segments (preserving the IReadOnlyCollection<GtsIdSegment> type) so callers cannot downcast to List<T> and mutate the segments used by Matches() and other value-object logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Gts/Extraction/GtsJsonEntity.cs`:
- Around line 276-313: FirstNonEmptyField currently iterates JSON keys which
breaks the configured priority order; change it to iterate the provided
propertyNames in order instead: for each name in propertyNames call
GetFieldValue(json, name) and if not null/empty and IsValidGtsId return (name,
val); after that loop repeat over propertyNames returning the first non-empty
value; update the function body (referencing FirstNonEmptyField, GetFieldValue,
IsValidGtsId, and the property name lists like
EntityIdPropertyNames/SchemaIdPropertyNames) to use propertyNames-ordered
iteration rather than looping over json keys so extraction respects
GtsExtractOptions priority.
- Around line 257-273: GetFieldValue currently calls node?.GetValue<string>()
which throws for non-string JSON leaves; change it to safely probe the JsonNode
kind (or check for JsonValue) and extract a string representation only when
appropriate: for string nodes use GetString()/GetValue<string>() safely, for
number/boolean/null use their textual representation (e.g. call TryGetValue(out
object?) / ToString() or use JsonElement.GetRawText()) and treat null/whitespace
the same as before, then apply Trim() and the $id GtsUriPrefix logic. Apply the
same safe-extraction approach in WalkAndCollectRefs (and the other affected
blocks referenced around lines 325-337) so ExtractId, ExtractEntity, and
ExtractReferences no longer crash on non-string leaves.
---
Nitpick comments:
In `@Gts/GtsId.cs`:
- Around line 33-39: The Segments property is exposed as
IReadOnlyCollection<GtsIdSegment> but currently stores the mutable collection
passed in, allowing callers to downcast and mutate it; in each GtsId constructor
(e.g. internal GtsId(string id, IReadOnlyCollection<GtsIdSegment> segments) and
the other constructors referenced) wrap the incoming collection in a
ReadOnlyCollection<GtsIdSegment> and assign that to Segments (preserving the
IReadOnlyCollection<GtsIdSegment> type) so callers cannot downcast to List<T>
and mutate the segments used by Matches() and other value-object logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f88b009-d53b-4a3f-b4d6-2dced805143a
📒 Files selected for processing (6)
Gts.Tests/Extraction/ExtractBasicTests.csGts.Tests/Extraction/ExtractEntityTests.csGts.Tests/Extraction/ExtractSchemaTests.csGts.Tests/Matching/PatternMatchingTests.csGts/Extraction/GtsJsonEntity.csGts/GtsId.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- Gts.Tests/Extraction/ExtractEntityTests.cs
- Gts.Tests/Extraction/ExtractSchemaTests.cs
- Gts.Tests/Extraction/ExtractBasicTests.cs
- Gts.Tests/Matching/PatternMatchingTests.cs
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
2 participants
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 by CodeRabbit
New Features
Tests