rknell / alfred

A performant, expressjs like server framework with a few gadgets that make life even easier.
Other
526 stars 29 forks source link

Trailing comma in JSON breaks the whole thing #85

Closed tomassasovsky closed 2 years ago

tomassasovsky commented 2 years ago

the body I'm sending:

{
    "email": "to.sasovsky@gmail.com",
}

the error I'm getting:

image

rknell commented 2 years ago

Ok, so at the end of the day, its invalid JSON. Darts JSON decoder falls over:

Screen Shot 2022-03-29 at 10 20 29 am

and even the javascript JSON decoder will throw if you put a trailing comma:

Screen Shot 2022-03-29 at 10 23 31 am

So I don't think it should handle it gracefully. However - it should throw an error that its an invalid body, as opposed to header already sent. So I will do some work on throwing a better error but will leave the invalid JSON alone.

(Edit: the javascript JSON example wasn't valid)

tomassasovsky commented 2 years ago

I understand, the thing is that it takes the app down. I've tried adding the onInternalError function but still.

rknell commented 2 years ago

we definitely don't want to crash the server!

rknell commented 2 years ago

I just went to have a look at it and the most recent test is there to handle invalid JSON input and return a 400 (thereby not crashing the server) what version of Alfred are you using?

tomassasovsky commented 2 years ago

using alfred: ^0.1.5+3

rknell commented 2 years ago

That release was literally the one that added the test for dodgy JSON so it should be working.

It might be that the published version doesn't include the code - ie I made a mistake. Do you mind adding a dependency for the master branch and seeing if that resolves it? If it does I will release a new version.

alfred:
    git: https://github.com/rknell/alfred.git
tomassasovsky commented 2 years ago

tried that, I'm getting a pretty nasty error: image

tomassasovsky commented 2 years ago

Tried running it directly with dart, but still.. image

rknell commented 2 years ago

Ahhhh....

So I think its being caused by you using a try catch block and then trying to return something.

I can reproduce it with this:

  test('it handles invalid json input with a trailing comma', () async {
    app.post('/test', (req, res) async {
      try {
        await req.body;
      } catch (e) {
        throw AlfredException(500, {'test': 'response'});
      }
    });
    final response = await http.post(Uri.parse('http://localhost:$port/test'),
        body: '{ "email": "to.sasovsky@gmail.com",}',
        headers: {'Content-Type': 'application/json'});
    expect(response.statusCode, 400);
  });

I'll see if there is a way not to crash the server when you have already sent a header.

seceba commented 2 years ago

Thank you for your help, you have shared very useful information.

I would like to make one addition. If we add double quotes to the key name when submitting Form-Data and we use AlfredException to catch the error, alfred crashes.

Postman:

curl --location --request DELETE '127.0.0.1:3000/testnew' \ --form '"fooo"="123"'

Key name "foo" with quotes.

Endpoint Codes:

server.post("/testnew", (req, res) async { try { var testbody = await req.body; return testbody.toString(); } catch (e) { throw AlfredException(401, "Error"); } });

Crash Errors:

**Unhandled exception: Bad state: Header already sent

0 _HttpResponse.statusCode= (dart:_http/http_impl.dart:1190:35)

1 Alfred._incomingRequest

package:alfred/src/alfred.dart:364

#2 _QueuedFuture.execute package:queue/src/dart_queue_base.dart:26 **
rknell commented 2 years ago

should be resolved in v0.1.6+1