Skip to content
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.

Sign up for GitHub

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

dont return null on exception in JsonPointerExtensions.Find #1412

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

dont return null on exception in JsonPointerExtensions.Find #1412

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b320ae
dont return null on exception in JsonPointerExtensions.Find
SimonCropp Oct 6, 2023
06059ab
- refactors node lookup to make it more resilient to looking up unexi…
baywet Oct 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 12 additions & 19 deletions src/Microsoft.OpenApi.Readers/ParseNodes/JsonPointerExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,33 +21,26 @@ public static YamlNode Find(this JsonPointer currentPointer, YamlNode baseYamlNo
return baseYamlNode;
}

try
var pointer = baseYamlNode;
foreach (var token in currentPointer.Tokens)
{
var pointer = baseYamlNode;
foreach (var token in currentPointer.Tokens)
if (pointer is YamlSequenceNode sequence)
{
if (pointer is YamlSequenceNode sequence)
{
pointer = sequence.Children[Convert.ToInt32(token)];
}
else
pointer = sequence.Children[Convert.ToInt32(token)];
}
else
{
if (pointer is YamlMappingNode map)
{
if (pointer is YamlMappingNode map)
if (!map.Children.TryGetValue(new YamlScalarNode(token), out pointer))
{
if (!map.Children.TryGetValue(new YamlScalarNode(token), out pointer))
{
return null;
}
return null;
}
}
}

return pointer;
}
catch (Exception)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this one I'm not sure about this change. Yes error swallowing is terrible, I believe this was done to add tolerance to poorly structured documents.
I think those are the only reasons why we might get an exception here:

  • sequence.Children is null: we can guard against that
  • the lookup in the children dictionary doesn't contain the value: we can use a TryGetValue instead
  • Convert.ToInt32 fails to parse the token: we can guard against that
  • the constructor for yamlScalarNode fails to parse the token, I'd have to check the API surface/implementation but we should be able to guard against that too.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sequence.Children

if it is valid we should check. if it isnt we should throw

the lookup in the children dictionary doesn't contain the value: we can use a TryGetValue instead

agreed

Convert.ToInt32 fails to parse the token: we can guard against that

doesnt that imply a corrupted/invalid doc and we should throw?

the constructor for yamlScalarNode fails to parse the token, I'd have to check the API surface/implementation but we should be able to guard against that too.

better to throw than return null

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@darrelmiller was also worried about that pattern way back when the PR was made
https://github.com/microsoft/OpenAPI.NET/pull/197/files#r168376724

That being said, this method is only used in RootNode.Find, which is only used to find the openapi/swagger nodes.
I think this tolerance has been added because it's the very beginning of reading the document, depending on the version of the spec, the parsing logic is different, and the lib will be looking for nodes that do not exist.

Returning null is better (faster) than throwing and catching an exception, and that part of the code is brittle by nature (node might or might not be there), hence why some fault tolerance is needed.

What's weird is why that extension method was made public initially, it should be internal instead. By I guess the ship has sailed.

So avoid derailing the logic or negatively impacting the performance, let's keep the current behaviour of returning null, and let's restructure the implementation so it cannot throw with the suggestions I've made. (I'm going to push a commit)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there, I just pushed the changes, let me know what you think.
No need to guard against the constructor, all it does is copying the string value and use it during the lookup to get the hashcode.

{
return null;
}

return pointer;
baywet marked this conversation as resolved.
Show resolved Hide resolved
}
}
}