Fix handling of empty request content by allowing to disable listeners per operation #2757

teohhanhui · 2019-04-23T18:26:12Z

Q	A
Bug fix?	yes?
New feature?	kinda?
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1282, #2731
License	MIT
Doc PR	TODO

The toggleable listeners are:

ReadListener ("read")
DeserializeListener ("deserialize")
ValidateListener ("validate")
WriteListener ("write")
SerializeListener ("serialize")

Basically, we fix the handling of empty request content by removing explicit handling of empty request content (which was wrong, resulting in weird bugs such as #2731), but allowing the user to be in control instead.

TODO:

Fix tests
Add tests
GraphQL support

soyuka · 2019-04-24T07:52:25Z

Thanks to this you'd have the ability to remove some listeners (@ApiResource(attributes={"read"=false})) ?
What are the use cases? Why isn't it possible to send an empty body anymore?

I also don't get the link with #1282

teohhanhui · 2019-04-24T07:55:15Z

Thanks to this you'd have the ability to remove some listeners (@ApiResource(attributes={"read"=false})) ?

Yes.

Why isn't it possible to send an empty body anymore?

I don't get what you mean. It's possible to send an empty body if you set "deserialize"=false (and possibly "validate"=false, depending on the use case).

teohhanhui · 2019-04-24T08:05:20Z

Well, we fix #1282 by doing the right thing out of the box, and allowing the user to disable specific listeners so that they're able to send empty body, as necessary.

Or in other words, no weird attempts by us to try to read the user's mind. 🙈

teohhanhui · 2019-04-24T08:08:34Z

But I'll add tests to show that.

soyuka · 2019-04-24T08:09:23Z

I don't get what you mean. It's possible to send an empty body if you set "deserialize"=false (and possibly "validate"=false, depending on the use case).

You removed a functional test, in a way a promise that this use case works :p. I'm not fond of these new attributes though, they're scarry lol, the wdyt @dunglas ?

Or in other words, no weird attempts by us to try to read the user's mind. see_no_evil

Couldn't we fix this by implementing the following table instead?

content-type	body content	result	notes
not set	empty	process the request
not set	with data	406	throw not acceptable: content-type is mandatory
set	empty	may throw 400 if deserializer fails	an empty string is valid text but invalid json
set	with data	process the request

teohhanhui · 2019-04-24T08:16:07Z

You removed a functional test, in a way a promise that this use case works :p.

Because it's wrong. I'll add new ones.

Couldn't we fix this by implementing the following table instead?

No. The interpretation depends on the content-type. We don't have enough information to make that decision. It's better to leave it up to the user. But we do the right thing by default, by leaving that up to the Serializer.

dunglas

I like the idea, but these new attributes must be honored in GraphQL resolvers too.

features/main/crud.feature

src/Bridge/Symfony/Bundle/Resources/config/api.xml

src/Metadata/Resource/ToggleableOperationAttributeTrait.php

dunglas · 2019-04-24T09:57:58Z

Also, couldn't this replace the _api_receive, _api_respond and _api_persist request attributes?

teohhanhui · 2019-04-24T12:40:02Z

Also, couldn't this replace the _api_receive, _api_respond and _api_persist request attributes?

Yes, but we'd need to add the same for:

AddFormatListener ("negotiate")
RespondListener ("send")

We could rename the listeners too (and deprecate the old names).

So the whole chain would be:

negotiate -> read -> deserialize -> validate -> write -> serialize -> send

Any of the steps can be disabled individually.

WDYT @dunglas?

teohhanhui · 2019-04-24T12:40:42Z

The semantics would be of interest to the built-in events implementation. /cc @alanpoulain

teohhanhui · 2019-04-25T18:04:08Z

Also, couldn't this replace the _api_receive, _api_respond and _api_persist request attributes?

@dunglas Another difficulty is that _api_respond is also used in SwaggerUiListener and ExceptionListener. I'm not sure what we should do about that.

Resolved

teohhanhui · 2019-04-26T18:20:11Z

Could someone help implement the GraphQL support?

@alanpoulain? 😄

teohhanhui · 2019-04-29T17:04:29Z

@dunglas Could GraphQL support be added in a separate PR (in master)? My primary aim here is to fix a bug. It's only as a result of that that we end up introducing a new feature. 😆

features/main/crud.feature

…s per operation The toggleable listeners are: - ReadListener ("read") - DeserializeListener ("deserialize") - ValidateListener ("validate") - WriteListener ("write") - SerializeListener ("serialize")

soyuka · 2019-05-07T14:48:55Z

This needs to be documented asap

maks-rafalko · 2019-05-21T11:05:52Z

This needs to be documented asap

Here I'm crying again. Sorry - we are trying to be up-to-date with new stable releases in quite a big project.

Our tests failing for 2.4.3, and I spent quite a bit of time looking to the changelog, commits between 2.4.2 and 2.4.3 and searching in 3 repos something similar to this issue to find that this feature is not documented (I'm not even saying that this is a BC break in patch version).

Could you please revise the way you guys deploy new features? If it's released, please make sure it's documented. Please make sure there is a note in the changelog.

dunglas · 2019-05-21T11:23:13Z

@borNfreee I agree, I missed it wasn't targeting master. Maybe should we revert this patch and merge it only in master. Also, it introduces an inconsistency with GraphQL. WYDT @teohhanhui? Can I revert this patch safely?

maks-rafalko · 2019-05-21T11:25:47Z

If you guys decide to revert it, please tag a 2.4.4 if that is possible ASAP, since now everyone who wants to update a patch version will have a problem

teohhanhui · 2019-05-21T11:32:40Z

It's a bug fix. I don't see why it should be reverted.

But we should (I should) add the documentation.

dunglas · 2019-05-21T11:34:09Z

It breaks existing applications that are just upgrading to a new patch version, it’s not acceptable.

teohhanhui · 2019-05-21T11:40:34Z

It's only for custom operations which used an empty request body. Our previous hack introduced other bugs, such as throwing an unexpected and cryptic exception when the request body is empty.

There's no way to maintain compatibility with buggy behavior. So by definition a bug fix must break what depended on the buggy behavior.

#2757 (comment)

teohhanhui · 2019-05-21T12:43:43Z

I know this is overused, but...

On a more serious note, fixing bugs inevitably breaks apps that were depending on the buggy behavior. We could try to minimize that impact, but it should not dictate how we should / should not fix a bug.

dunglas · 2019-05-21T13:16:57Z

We could at least agree to not merge bug fixes introducing BC breaks in patch releases. They should at least be in minor releases. This one is a true BC break, not just an edge case... because the old behavior was covered by a test.

It's too late to revert now, but could you add a note in the CHANGELOG explaining how to fix this BC break please?

teohhanhui · 2019-05-21T14:30:48Z

All bug fixes are BC breaks. We have many test cases that are wrong. It's an unavoidable problem.

could you add a note in the CHANGELOG

I'm on it.

teohhanhui · 2019-05-21T15:01:20Z

See #2815

teohhanhui requested review from dunglas, soyuka and antograssiot April 23, 2019 18:26

This was referenced Apr 23, 2019

Allow a POST request to have an empty body #1786

Merged

Empty body on custom operations #1282

Closed

teohhanhui force-pushed the fix/handling-empty-request-content branch from 148776e to a37e4f6 Compare April 23, 2019 18:30

teohhanhui mentioned this pull request Apr 23, 2019

Cannot validate values of type "NULL" automatically #2731

Closed

teohhanhui force-pushed the fix/handling-empty-request-content branch 2 times, most recently from d104428 to caba84f Compare April 23, 2019 19:58

dunglas previously requested changes Apr 24, 2019

View reviewed changes

features/main/crud.feature Outdated Show resolved Hide resolved

src/Bridge/Symfony/Bundle/Resources/config/api.xml Outdated Show resolved Hide resolved

src/Metadata/Resource/ToggleableOperationAttributeTrait.php Show resolved Hide resolved

teohhanhui force-pushed the fix/handling-empty-request-content branch from caba84f to 7fb57fa Compare April 25, 2019 18:24

teohhanhui force-pushed the fix/handling-empty-request-content branch 2 times, most recently from 5db69a4 to a35bf0d Compare April 26, 2019 18:14

teohhanhui requested a review from dunglas April 26, 2019 18:16

soyuka approved these changes May 3, 2019

View reviewed changes

soyuka mentioned this pull request May 3, 2019

Impossible to use messenger Input class with empty DTO (no attributes) #2773

Closed

dunglas approved these changes May 6, 2019

View reviewed changes

features/main/crud.feature Outdated Show resolved Hide resolved

features/main/crud.feature Outdated Show resolved Hide resolved

Fix handling of empty request content by allowing to disable listener…

b1590b8

…s per operation The toggleable listeners are: - ReadListener ("read") - DeserializeListener ("deserialize") - ValidateListener ("validate") - WriteListener ("write") - SerializeListener ("serialize")

teohhanhui force-pushed the fix/handling-empty-request-content branch from a35bf0d to b1590b8 Compare May 6, 2019 16:20

teohhanhui merged commit d806c72 into api-platform:2.4 May 6, 2019

teohhanhui deleted the fix/handling-empty-request-content branch May 6, 2019 16:39

soyuka mentioned this pull request May 7, 2019

document toggleable listeners api-platform/docs#804

Closed

teohhanhui mentioned this pull request May 21, 2019

Add a note in the CHANGELOG about empty request content bugfix #2815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of empty request content by allowing to disable listeners per operation #2757

Fix handling of empty request content by allowing to disable listeners per operation #2757

teohhanhui commented Apr 23, 2019 •

edited

Loading

soyuka commented Apr 24, 2019 •

edited

Loading

teohhanhui commented Apr 24, 2019 •

edited

Loading

teohhanhui commented Apr 24, 2019 •

edited

Loading

teohhanhui commented Apr 24, 2019

soyuka commented Apr 24, 2019 •

edited

Loading

teohhanhui commented Apr 24, 2019 •

edited

Loading

dunglas left a comment

dunglas commented Apr 24, 2019

teohhanhui commented Apr 24, 2019 •

edited

Loading

teohhanhui commented Apr 24, 2019

teohhanhui commented Apr 25, 2019

teohhanhui commented Apr 26, 2019

teohhanhui commented Apr 29, 2019

soyuka commented May 7, 2019

maks-rafalko commented May 21, 2019

dunglas commented May 21, 2019

maks-rafalko commented May 21, 2019

teohhanhui commented May 21, 2019

dunglas commented May 21, 2019

teohhanhui commented May 21, 2019 •

edited

Loading

teohhanhui commented May 21, 2019

dunglas commented May 21, 2019

teohhanhui commented May 21, 2019

teohhanhui commented May 21, 2019

Fix handling of empty request content by allowing to disable listeners per operation #2757

Fix handling of empty request content by allowing to disable listeners per operation #2757

Conversation

teohhanhui commented Apr 23, 2019 • edited Loading

soyuka commented Apr 24, 2019 • edited Loading

teohhanhui commented Apr 24, 2019 • edited Loading

teohhanhui commented Apr 24, 2019 • edited Loading

teohhanhui commented Apr 24, 2019

soyuka commented Apr 24, 2019 • edited Loading

teohhanhui commented Apr 24, 2019 • edited Loading

dunglas left a comment

Choose a reason for hiding this comment

dunglas commented Apr 24, 2019

teohhanhui commented Apr 24, 2019 • edited Loading

teohhanhui commented Apr 24, 2019

teohhanhui commented Apr 25, 2019

teohhanhui commented Apr 26, 2019

teohhanhui commented Apr 29, 2019

soyuka commented May 7, 2019

maks-rafalko commented May 21, 2019

dunglas commented May 21, 2019

maks-rafalko commented May 21, 2019

teohhanhui commented May 21, 2019

dunglas commented May 21, 2019

teohhanhui commented May 21, 2019 • edited Loading

teohhanhui commented May 21, 2019

dunglas commented May 21, 2019

teohhanhui commented May 21, 2019

teohhanhui commented May 21, 2019

teohhanhui commented Apr 23, 2019 •

edited

Loading

soyuka commented Apr 24, 2019 •

edited

Loading

teohhanhui commented Apr 24, 2019 •

edited

Loading

teohhanhui commented Apr 24, 2019 •

edited

Loading

soyuka commented Apr 24, 2019 •

edited

Loading

teohhanhui commented Apr 24, 2019 •

edited

Loading

teohhanhui commented Apr 24, 2019 •

edited

Loading

teohhanhui commented May 21, 2019 •

edited

Loading