[Serializer] Add support for seld/jsonlint #51172
Conversation
|
Do we have any number regarding the performance penalty introduced by this check? As this class is often used to create web APId, we should be careful to not introduce a way to amplify DOS attacks. |
|
Don't we already have this problem when signing in, which is very expensive in Symfony because of password encoding? I wouldn't be too harsh on that point here. |
|
In prod, you'll likely set up some rate limiting for the login endpoint. Having a way to easily consume many resources on every endpoint is a different story. Also, this feature is only useful during development. In production, it should be disabled as clients should never send invalid JSON anyway, and if they do it's easy to debug using client-side tools (most API development tools, such as Hoppscotch or Postman, include a JSON linter for instance). I would at least make this feature opt-in through a context flag. |
|
Problem I got inspired by is precisely Postman showing no issues with payload and PHP's json_decode refusing it (because of some invisible leading characters). And frontend dev was connecting to Prod API there. But I guess I could have reported issue to postman instead. Let's close here, I guess it's not too hard to decorate this class anyways. |
|
To clarify I'm not against merging this feature as long as it's opt-in! |
743cee9 to
0f3087d
Compare
0f3087d to
476325d
Compare
|
Addressed |
|
Just a thought: Can/should we have FrameworkBundle enable this feature if the jsonlint packe is available and debug mode is on? Alternatively, we could also do this in a recipe. |
f45b0a8 to
820b543
Compare
@ostrolucky Do you want to implement that in that PR or in a future one? |
Famous last words 😉 In my experience with consumer facing APIs you get quite often amateur level devs integrating things, sometimes writing json by hand, and for them having an API that returns helpful errors can save a bunch of time. |
820b543 to
c61a43d
Compare
Let's do it in next PR. Do you prefer recipe, or frameworkbundle doing this automatically on kernel.debug + when lib is installed? |
I think I would prefer a framework bundle integration. |
|
Thank you @ostrolucky. |
… in dev by default (ostrolucky) This PR was merged into the 6.4 branch. Discussion ---------- [FrameworkBundle] Enable `json_decode_detailed_errors` in dev by default | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #51172 (comment) | License | MIT | Doc PR | Follows * #51172 Commits ------- b595e90 [FrameworkBundle] Enable `json_decode_detailed_errors` in dev by default
… in dev by default (ostrolucky) This PR was merged into the 6.4 branch. Discussion ---------- [FrameworkBundle] Enable `json_decode_detailed_errors` in dev by default | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix symfony/symfony#51172 (comment) | License | MIT | Doc PR | Follows * symfony/symfony#51172 Commits ------- b595e900b2 [FrameworkBundle] Enable `json_decode_detailed_errors` in dev by default
|
I'm sorry for being so late to this party but... would it be a bad idea to trigger seld/jsonlint whenever we actually encounter a syntax error? We don't need to lint everything, just (some of) the failures. All the other suggested changes can still stick (opt-in, when package is available etc). We can use it without taking a (severe) performance hit (we can have our cake and eat it too). |
|
I've implemented my PR originally like that. It would trigger no matter the dev/prod environment. But I've changed it after the feedback provided, please check the conversation that we had here. |
|
Yeah, I'm sorry, I've read the conversation, but I didn't read the code. Thanks for pointing it out and for the very useful feature! |
This is kinda RFC proposal for improving JSON parsing errors in API context. At the moment, pretty much any time there is something wrong with input payload, framework will return something like so
{ "title": "Deserialization Failed", "detail": "Syntax error" }This provides very minimal info to go on. Also, "Syntax error" is quite confusing for majority of PHP devs I know (and especially frontend developers, to whom these errors display), as they don't realize this is how PHP's json_decode reports these issues. Wouldn't it be better if we got something like
{ "title": "Deserialization Failed", "detail": "Parse error on line 1:\n{'foo': 'bar'}\n^\nInvalid string, it appears you used single quotes instead of double quotes" }or
{ "title": "Deserialization Failed", "detail": "Parse error on line 1:\nkaboom!\n^\nExpected one of: 'STRING', 'NUMBER', 'NULL', 'TRUE', 'FALSE', '{', '['" }^ scenarios above are from
symfony/src/Symfony/Component/Serializer/Tests/Encoder/JsonDecodeTest.php
Lines 70 to 71 in fe30946
? This is what my patch will do. All you need to do is install @Seldaek's seld/jsonlint.
Now, initially I've just implemented support for this only in case jsonlint is installed, because I know how much Symfony tries to avoid adding dependencies. But if you are up for it, we could also make this required dependency, or embed its parser in Symfony. Or not do anything about it, saying PHP itself should improve this, but not sure how likely is that going to happen any time soon.