-
Notifications
You must be signed in to change notification settings - Fork 536
[Do not review] Experimental sourcenode parser #5013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/Microsoft.Health.Fhir.SourceNodeSerialization/SourceNodes/JsonNodeSourceNode.cs
Fixed
Show fixed
Hide fixed
src/Microsoft.Health.Fhir.SourceNodeSerialization/SourceNodes/MetaSourceNode.cs
Fixed
Show fixed
Hide fixed
{ | ||
if (resource is ResourceJsonNode resourceJsonNode) | ||
{ | ||
Name = name ?? resourceJsonNode.ResourceType; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the issue, we should avoid accessing the virtual property Name
in the constructor. Instead, we can introduce a private field to store the value of Name
during construction and use this field to implement the Name
property. This ensures that the constructor does not rely on virtual calls, eliminating the risk of unexpected behavior.
-
Copy modified line R26 -
Copy modified line R32 -
Copy modified lines R110-R111
@@ -25,3 +25,3 @@ | ||
{ | ||
Name = name ?? resourceJsonNode.ResourceType; | ||
_name = name ?? resourceJsonNode.ResourceType; | ||
ResourceType = resourceJsonNode.ResourceType; | ||
@@ -31,3 +31,3 @@ | ||
{ | ||
Name = name; | ||
_name = name; | ||
Location = location; | ||
@@ -109,3 +109,4 @@ | ||
|
||
public override string Name { get; } | ||
private readonly string _name; | ||
public override string Name => _name; | ||
|
if (resource is ResourceJsonNode resourceJsonNode) | ||
{ | ||
Name = name ?? resourceJsonNode.ResourceType; | ||
ResourceType = resourceJsonNode.ResourceType; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the issue, we should avoid making a virtual call in the constructor. Instead of directly accessing the ResourceType
property, we can use a private field to store the value during construction. This ensures that the constructor does not rely on virtual calls, eliminating the risk of unexpected behavior.
Steps to fix:
- Introduce a private readonly field
_resourceType
to store the value ofResourceType
during construction. - Modify the
ResourceType
property to return the value of_resourceType
. - Update the constructor to assign the value to
_resourceType
instead of directly accessing theResourceType
property.
-
Copy modified line R27 -
Copy modified lines R116-R117
@@ -26,3 +26,3 @@ | ||
Name = name ?? resourceJsonNode.ResourceType; | ||
ResourceType = resourceJsonNode.ResourceType; | ||
_resourceType = resourceJsonNode.ResourceType; | ||
Location = location ?? resourceJsonNode.ResourceType; | ||
@@ -115,3 +115,4 @@ | ||
|
||
public override string ResourceType { get; } | ||
private readonly string _resourceType; | ||
public override string ResourceType => _resourceType; | ||
|
{ | ||
Name = name ?? resourceJsonNode.ResourceType; | ||
ResourceType = resourceJsonNode.ResourceType; | ||
Location = location ?? resourceJsonNode.ResourceType; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the issue, we should avoid directly assigning the virtual property Location
in the constructor. Instead, we can use a private field to store the value and expose it through the virtual property. This ensures that the virtual property is not accessed during the base class constructor execution, preventing any unintended behavior.
Steps to implement the fix:
- Introduce a private field
_location
to store the value ofLocation
. - Modify the
Location
property to return the value of_location
. - Update the constructor to assign the value to
_location
instead of directly assigning toLocation
.
-
Copy modified line R24 -
Copy modified line R29 -
Copy modified line R115
@@ -23,2 +23,3 @@ | ||
{ | ||
private readonly string _location; | ||
if (resource is ResourceJsonNode resourceJsonNode) | ||
@@ -27,3 +28,3 @@ | ||
ResourceType = resourceJsonNode.ResourceType; | ||
Location = location ?? resourceJsonNode.ResourceType; | ||
_location = location ?? resourceJsonNode.ResourceType; | ||
} | ||
@@ -113,3 +114,3 @@ | ||
|
||
public override string Location { get; } | ||
public override string Location => _location; | ||
|
} | ||
else | ||
{ | ||
Name = name; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the issue, we should avoid assigning a value to the virtual property Name
directly in the constructor. Instead, we can introduce a private backing field for Name
and use it internally within the constructor. The Name
property can then return the value of this backing field. This ensures that no virtual calls are made during the constructor's execution.
-
Copy modified line R26 -
Copy modified line R32 -
Copy modified lines R110-R111
@@ -25,3 +25,3 @@ | ||
{ | ||
Name = name ?? resourceJsonNode.ResourceType; | ||
_name = name ?? resourceJsonNode.ResourceType; | ||
ResourceType = resourceJsonNode.ResourceType; | ||
@@ -31,3 +31,3 @@ | ||
{ | ||
Name = name; | ||
_name = name; | ||
Location = location; | ||
@@ -109,3 +109,4 @@ | ||
|
||
public override string Name { get; } | ||
private readonly string _name; | ||
public override string Name => _name; | ||
|
else | ||
{ | ||
Name = name; | ||
Location = location; |
Check warning
Code scanning / CodeQL
Virtual call in constructor or destructor Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the issue, avoid accessing the virtual property Location
directly in the constructor. Instead, use a private or protected field to store the value during construction, and then implement the Location
property to return this field. This ensures that the constructor does not invoke any overridden implementations of the property.
Steps to fix:
- Introduce a private or protected field
_location
to store the value ofLocation
. - Update the constructor to assign the value to
_location
instead of directly toLocation
. - Modify the
Location
property to return the value of_location
.
-
Copy modified line R19 -
Copy modified line R29 -
Copy modified line R34 -
Copy modified line R115
@@ -18,2 +18,3 @@ | ||
{ | ||
private readonly string _location; | ||
private readonly Lazy<List<(string Name, Lazy<IEnumerable<ISourceNode>> Node)>> _propertySourceNodes; | ||
@@ -27,3 +28,3 @@ | ||
ResourceType = resourceJsonNode.ResourceType; | ||
Location = location ?? resourceJsonNode.ResourceType; | ||
_location = location ?? resourceJsonNode.ResourceType; | ||
} | ||
@@ -32,3 +33,3 @@ | ||
Name = name; | ||
Location = location; | ||
_location = location; | ||
} | ||
@@ -113,3 +114,3 @@ | ||
|
||
public override string Location { get; } | ||
public override string Location => _location; | ||
|
510f4ef
to
0aa243f
Compare
resource.Meta.LastUpdated = new DateTimeOffset(lastUpdated.DateTime.TruncateToMillisecond(), lastUpdated.Offset); | ||
if (!lastUpdatedIsNull && resource.Meta.LastUpdated.Value > Clock.UtcNow.AddSeconds(10)) // 5 sec is the max for the computers in the domain | ||
var lastUpdated = lastUpdatedIsNull ? Clock.UtcNow : resource.Meta.LastUpdated; | ||
var updatedDateTime = new DateTimeOffset(lastUpdated.Value.DateTime.TruncateToMillisecond(), lastUpdated.Value.Offset); |
Check warning
Code scanning / CodeQL
Dereferenced variable may be null Warning
lastUpdated
Variable
lastUpdated
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the issue, we need to ensure that lastUpdated
is never dereferenced when it is null
. This can be achieved by adding a null check before accessing lastUpdated.Value
. If lastUpdated
is null
, an appropriate fallback value (e.g., Clock.UtcNow
) should be used.
The fix involves:
- Adding a null check for
lastUpdated
before accessinglastUpdated.Value
. - Ensuring that the fallback value is used if
lastUpdated
isnull
.
-
Copy modified lines R54-R57
@@ -53,2 +53,6 @@ | ||
var lastUpdated = lastUpdatedIsNull ? Clock.UtcNow : resource.Meta.LastUpdated; | ||
if (lastUpdated == null) | ||
{ | ||
lastUpdated = Clock.UtcNow; | ||
} | ||
var updatedDateTime = new DateTimeOffset(lastUpdated.Value.DateTime.TruncateToMillisecond(), lastUpdated.Value.Offset); |
|
||
if (!(_contentElement == null || | ||
_contentElement.Value.ValueKind != JsonValueKind.Object | ||
|| _contentElement.Value.EnumerateObject().Any() == false)) |
Check notice
Code scanning / CodeQL
Unnecessarily complex Boolean expression Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fix the issue, we will simplify the Boolean expression _contentElement.Value.EnumerateObject().Any() == false
to !_contentElement.Value.EnumerateObject().Any()
. This change reduces unnecessary complexity while preserving the original logic. The fix will be applied directly to line 91 of the file.
-
Copy modified line R91
@@ -90,3 +90,3 @@ | ||
_contentElement.Value.ValueKind != JsonValueKind.Object | ||
|| _contentElement.Value.EnumerateObject().Any() == false)) | ||
|| !_contentElement.Value.EnumerateObject().Any())) | ||
{ |
|
||
if (prop.ValueKind == JsonValueKind.Array) | ||
{ | ||
return (prop.EnumerateArray().Select(x => x).ToArray(), true); |
Check warning
Code scanning / CodeQL
Redundant Select Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fix the issue, we will remove the redundant Select
call from the LINQ query on line 210. The prop.EnumerateArray()
method already returns an IEnumerable<JsonElement>
that can be directly converted to an array using the ToArray()
method. By removing the Select(x => x)
call, we simplify the code without altering its behavior.
-
Copy modified line R210
@@ -209,3 +209,3 @@ | ||
{ | ||
return (prop.EnumerateArray().Select(x => x).ToArray(), true); | ||
return (prop.EnumerateArray().ToArray(), true); | ||
} |
7a07f66
to
d4cca2e
Compare
d4cca2e
to
c55846e
Compare
ITypedElement node = sourceNode.ToTypedElement(ModelInfoProvider.StructureDefinitionSummaryProvider); | ||
ITypedElement familyType = node.Select("Patient.name.family").Single(); | ||
|
||
IReadOnlyCollection<IElementDefinitionSummary> definitions = familyType.ChildDefinitions(ModelInfoProvider.StructureDefinitionSummaryProvider); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
definitions
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the issue, the assignment to the variable definitions
on line 170 should be removed entirely, as it is not used anywhere in the method. This will clean up the code and eliminate the unnecessary assignment. No additional changes are required, as the removal does not affect the functionality of the method.
-
Copy modified line R170
@@ -169,3 +169,3 @@ | ||
|
||
IReadOnlyCollection<IElementDefinitionSummary> definitions = familyType.ChildDefinitions(ModelInfoProvider.StructureDefinitionSummaryProvider); | ||
familyType.ChildDefinitions(ModelInfoProvider.StructureDefinitionSummaryProvider); | ||
} |
75e9def
to
cbbdcd6
Compare
cbbdcd6
to
b0bdc96
Compare
Description
Very experimental fhir parser.
Related issues
Addresses [issue #].
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)