server: send validation failure reason to clients #109

Merged
merged 1 commit into from Nov 5, 2012

Projects

None yet

3 participants

@indutny
Contributor
indutny commented Oct 31, 2012

Make 400 a validation failure http code, and send JSON payload with error reason to clients.

I'm not sure if encoding body with JSON is the best option here, any comments would be welcome

@rauchg
Contributor
rauchg commented Oct 31, 2012

The 4 in 400 already indicates error, so I don't see the benefit of JSON with error: true and reason over just sending the reason as text/plain.

@indutny
Contributor
indutny commented Oct 31, 2012

yeah, agreed. it's just hard to check it in tests...

@rauchg
Contributor
rauchg commented Oct 31, 2012

400 >= res.statusCode ?

@indutny
Contributor
indutny commented Oct 31, 2012

https://github.com/LearnBoost/engine.io/pull/109/files#L1R53 <- see, I'm checking what error has happened

@3rd-Eden
Contributor

But the body of the request doesn't need to be JSON, it can just be plain text.

On Wednesday 31 October 2012 at 16:43, Fedor Indutny wrote:

https://github.com/LearnBoost/engine.io/pull/109/files#L1R53 <- see, I'm checking what error has happened


Reply to this email directly or view it on GitHub (#109 (comment)).

@rauchg
Contributor
rauchg commented Oct 31, 2012

If we were doing something like

{ code: 1, message: 'Error message' }

it would be more justified, since we're adding more information. But error: true is redundant, and the message can just be the body.

@rauchg
Contributor
rauchg commented Oct 31, 2012

I actually think we should go with codes, and document them in the spec.

UNKNOWN_TRANSPORT
UNKNOWN_SID
BAD_HANDSHAKE_METHOD

@rauchg
Contributor
rauchg commented Oct 31, 2012

And let's get rid of error: true

@rauchg
Contributor
rauchg commented Oct 31, 2012

The spec is in the engine.io-client repo btw (tree/master/SPEC.md)

@indutny
Contributor
indutny commented Oct 31, 2012

@guille force pushed, will open PR for engine.io-client soon

@indutny
Contributor
indutny commented Nov 5, 2012

Any update?

@rauchg rauchg merged commit ee1eacf into socketio:master Nov 5, 2012
@rauchg
Contributor
rauchg commented Nov 5, 2012

Beautiful, we need to update SPEC now

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