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

Json.Decode.decodeValue runtime error #890

Open
pauldijou opened this Issue Aug 4, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@pauldijou

pauldijou commented Aug 4, 2017

If you have a recursive JavaScript object and you try to apply a simple decoder on it like that:

myString = Json.Decode.decodeValue Json.Decode.string myRecursiveObject

This will crash at runtime because it tries to stringify the recursive value when generating the error message which will fail with TypeError: Uncaught error: Converting circular structure to JSON

@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Aug 4, 2017

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented Aug 4, 2017

Thanks for the issue! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@pietro909

This comment has been minimized.

Show comment
Hide comment
@pietro909

pietro909 Aug 8, 2017

I also incurred in that problem once. But do you think is Elm's error? The decoder is meant to decode JSON data, and the JSON spec doesn't mention references.

I think you shouldn't try to parse a generic JavaScript Object unless it is JSON compliant.

pietro909 commented Aug 8, 2017

I also incurred in that problem once. But do you think is Elm's error? The decoder is meant to decode JSON data, and the JSON spec doesn't mention references.

I think you shouldn't try to parse a generic JavaScript Object unless it is JSON compliant.

@pauldijou

This comment has been minimized.

Show comment
Hide comment
@pauldijou

pauldijou Aug 8, 2017

Yeah, I've been asking myself the same question. But JSON is actually a String which follow a specific syntax. According to that, Json.Encode functions should all return strings, Json.Encode.Value should probably not exist, and Json.Decode.decodeValue would make no sense. That's one way to solve the problem.

But, if keeping Json.Encode.Value, it's dangerous because you can pass anything as a Json.Encode.Value inside a port, even if it is a JavaScript function or a recursive object. There is no check at all.

So, if the goal is to prevent compiled code to crash at runtime, I still think there is a problem somewhere. Not sure if the best solution is to prevent Json.Encode.Value at all, or just checking them inside ports, or adding a try/catch inside toString and decoders.

pauldijou commented Aug 8, 2017

Yeah, I've been asking myself the same question. But JSON is actually a String which follow a specific syntax. According to that, Json.Encode functions should all return strings, Json.Encode.Value should probably not exist, and Json.Decode.decodeValue would make no sense. That's one way to solve the problem.

But, if keeping Json.Encode.Value, it's dangerous because you can pass anything as a Json.Encode.Value inside a port, even if it is a JavaScript function or a recursive object. There is no check at all.

So, if the goal is to prevent compiled code to crash at runtime, I still think there is a problem somewhere. Not sure if the best solution is to prevent Json.Encode.Value at all, or just checking them inside ports, or adding a try/catch inside toString and decoders.

@ChristophP

This comment has been minimized.

Show comment
Hide comment
@ChristophP

ChristophP Nov 22, 2017

I just had the issue as well. I was

  • decoding a circular JS object
  • with decodeValue
  • and no stringifications

Everything worked when the decoding succeeds but when the decoder failed, JSON.stringify() is called to construct a proper error msg.
Would be cool if this line line were wrapped in a try catch https://github.com/elm-lang/core/blob/5.1.1/src/Native/Json.js#L235

ChristophP commented Nov 22, 2017

I just had the issue as well. I was

  • decoding a circular JS object
  • with decodeValue
  • and no stringifications

Everything worked when the decoding succeeds but when the decoder failed, JSON.stringify() is called to construct a proper error msg.
Would be cool if this line line were wrapped in a try catch https://github.com/elm-lang/core/blob/5.1.1/src/Native/Json.js#L235

@rupertlssmith

This comment has been minimized.

Show comment
Hide comment
@rupertlssmith

rupertlssmith Dec 1, 2017

Did anyone capture the runtime exception that is thrown at: https://github.com/elm-lang/core/blob/5.1.1/src/Native/Json.js#L235 ?

I want to know if it is an out of stack space exception from infinite looping, or if it is some other exception indicating that javascript spotted the infinite loop.

If its an out of stack space exception, then wrapping it in a try/catch is not a good idea.

Also, the code has changed a lot since the 5.1.1 release, so this might already be fixed. Will take a look.

rupertlssmith commented Dec 1, 2017

Did anyone capture the runtime exception that is thrown at: https://github.com/elm-lang/core/blob/5.1.1/src/Native/Json.js#L235 ?

I want to know if it is an out of stack space exception from infinite looping, or if it is some other exception indicating that javascript spotted the infinite loop.

If its an out of stack space exception, then wrapping it in a try/catch is not a good idea.

Also, the code has changed a lot since the 5.1.1 release, so this might already be fixed. Will take a look.

@rupertlssmith

This comment has been minimized.

Show comment
Hide comment
@rupertlssmith

rupertlssmith Dec 1, 2017

Seems to be fixed here:
https://github.com/elm-lang/core/blob/master/src/Elm/Kernel/Json.js#L327

On failure the JSON is returned as a Value.

rupertlssmith commented Dec 1, 2017

Seems to be fixed here:
https://github.com/elm-lang/core/blob/master/src/Elm/Kernel/Json.js#L327

On failure the JSON is returned as a Value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment