dont return null on exception in JsonPointerExtensions.Find

[SimonCropp]

IMO catching an unknown exception type and return null will hide real errors

[baywet]

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?

[SimonCropp]

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

[baywet]

@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)

[baywet]

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.