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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better error for JSON Pointer "overshooting" the actual JSON structure. #2154

Closed
ateska opened this issue Mar 22, 2024 · 5 comments 路 Fixed by #2178
Closed

Better error for JSON Pointer "overshooting" the actual JSON structure. #2154

ateska opened this issue Mar 22, 2024 · 5 comments 路 Fixed by #2178
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@ateska
Copy link

ateska commented Mar 22, 2024

Hello,

I post after a longer time 馃憢馃徎 - but we still very intensively use SIMDJSON (thru cysimdjson).

Is your feature request related to a problem? Please describe.

{
	"document": {
		"key1": 1,
		"key2": "2",
		"key3": "3",
		"key4": 40,
		"key5": "50"
	}
}

The call .at_pointer("/document/key4/sub") fails (correctly) but the reported error is INVALID_JSON_POINTER (Invalid JSON pointer syntax).

The JSON Pointer syntax is correct (at least in my understanding) - in a different JSON document, the JSON pointer will be perfectly fine.
When this error is reported to users, it is misleading what to fix.

Describe the solution you'd like

Return different error code for this situation such as NO_SUCH_FIELD, so that the problem has a structure that can be used for a correct problem reporting to users.

Describe alternatives you've considered

We consider to build a validator of JSON Pointer that will be applied before the JSON pointer is applied to the JSON.
So that we can capture real syntax errors, report them to the user and this one will be reported as a "missing field/incorrect JSON structure" for a given JSON Pointer

Additional context

N/A

Are you willing to contribute code or documentation toward this new feature?

No, I don't write C++ well enough to contribute, sorry.

@lemire
Copy link
Member

lemire commented Mar 22, 2024

It is a reasonable issue. The reason why we get this error is that we arrive at a number, and we are asked, at the number to resolve 'sub'. This falls through and reports INVALID_JSON_POINTER. It would be easy to do better, although it is not entirely clear right now what we should do instead. We almost surely need a different error code. See the code (it is quite simple):

simdjson_inline simdjson_result<value> value::at_pointer(std::string_view json_pointer) noexcept {
  json_type t;
  SIMDJSON_TRY(type().get(t)); // <==== the type is number, so not array or object
  switch (t)
  {
    case json_type::array:
      return (*this).get_array().at_pointer(json_pointer);
    case json_type::object:
      return (*this).get_object().at_pointer(json_pointer);
    default:
      return INVALID_JSON_POINTER;// <===== We get here
  }
}

We could throw a different error just by changing one line.

@lemire
Copy link
Member

lemire commented Mar 22, 2024

This is actually not a difficult issue. I am inviting contributions.

@lemire lemire added help wanted Extra attention is needed good first issue Good for newcomers labels Mar 22, 2024
@lemire
Copy link
Member

lemire commented Mar 22, 2024

Marking as a good first issue.

@yankov-pt
Copy link

Hi, would NO_SUCH_FIELD be the right error code for this?

@lemire
Copy link
Member

lemire commented Mar 26, 2024

@yankov-pt I suppose so, yes.

pnck added a commit to pnck/simdjson that referenced this issue May 8, 2024
pnck added a commit to pnck/simdjson that referenced this issue May 8, 2024
@pnck pnck mentioned this issue May 8, 2024
lemire pushed a commit that referenced this issue May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants