Skip to content
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

[HttpClient] allow arbitrary JSON values in requests #34216

Merged

Conversation

@pschultz
Copy link
Contributor

pschultz commented Nov 1, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

Allow arbitrary values in the "json" request option. Previously values were
limitated to arrays and objects of type JsonSerializable. This doesn't account
for scalar values and classes with public properties (which don't need to
implement JsonSerializable), all of which are perfectly acceptable arguments to
json_encode
:

value
The value being encoded. Can be any type except a resource.

It seems silly to expect users to add classes implementing JsonSerializable to represent, say, scalar values or an empty object.

I'm not sure if you consider removed exceptional cases a BC break or not. I'm assuming "no" for now.

@pschultz pschultz force-pushed the classmarkets:http/arbitrary-json-values branch from 831cc35 to 4b75a5a Nov 1, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 1, 2019

I think allowing stdclass would make sense. I'm really unsure about scalars. That doesn't look like valid API design to me so I don't feel the need support them much. There is "body" if ppl really want them.

@pschultz

This comment has been minimized.

Copy link
Contributor Author

pschultz commented Nov 4, 2019

I think allowing stdclass would make sense.

That still ignores all other object types:

class Book
{
    public $title = '';
    public $author = '';
}

I'm really unsure about scalars. That doesn't look like valid API design to me

That's not for Symfony do decide, especially not for a client which has to work with existing servers. There are plenty of services that distingish between an empty object and null, for example.

If the json option doesn't allow all valid values, library authors have no choice but to ignore the json option and implement the encoding themselves.

What is the reason for the type restriction? I don't see any downside in removing it. Passing any value that json_encode can't work with will still throw an InvalidArgumentException.

@nicolas-grekas nicolas-grekas added this to the next milestone Nov 4, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 4, 2019

That still ignores all other object types:

To me, we don't want to allow arbitrary objects, or that's quite dangerous as one can very easily provide something that is not meant to be sent on the wire.
Yes, it's still possible to do so, but there is a small barrier at least.

In you example, (array) $book would to it.

There are plenty of services that distingish between and empty object and null, for example.

Can you provide an example please? E.g a link to a documentation?

If the json option doesn't allow all valid values, library authors have no choice but to ignore the json option and implement the encoding themselves.

That'd be fair to me - such libs would take responsibility for the type broadening - ensuring some guard (or not) on their own.

What is the reason for the type restriction? I don't see any downside in removing it.

On top of the small safety barrier, this is also consistent with toArray().
Let's say the client favors JSON APIs with the O enforced to array|object.
That doesn't restrict its capabilities in any way, since there is always body+getContent().

Passing any value that json_encode can't work with will still throw an InvalidArgumentException.

That's the issue: json_encode works with way too many objects.

At least that's the current reasoning.

@pschultz

This comment has been minimized.

Copy link
Contributor Author

pschultz commented Nov 4, 2019

In you example, (array) $book would to it.

As a library author I can't do that, because JsonSerialize may be implemented.

That's the issue: json_encode works with way too many objects.

I don't follow this reasoning. json_encode supports all those values that can be represented in JSON. And checking the "top-level" value doesn't change anything about that anyway. It seems silly to me to disallow class instances at the top level but allowing them as an array value (i.e. $book fails but ['data' => $book] is fine).

I was just surprised that the json option isn't just an equivalent for 'body' => json_encode($foo) (plus content-type header and exception handling). I assumed that was an oversight. If it wasn't then I'll just pretend the json option doesn't exist.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 6, 2019

/cc @symfony/mergers any opinion here?

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Nov 6, 2019

*
* @throws InvalidArgumentException When the value cannot be json-encoded
*/
private static function jsonEncode($value, int $flags = null, int $maxDepth = 512): string
{
$flags = $flags ?? (JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT | JSON_PRESERVE_ZERO_FRACTION);
if (!\is_array($value) && !$value instanceof \JsonSerializable) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 6, 2019

Member

Should null be supported too? if yes then the isset L71 should be replaced by array_key_exists()
This would prevent setting a default "json" option and being able to override it with setting a "body" one later one.
But setting a default json/body makes no sense to me so doesn't have to be supported that much.

This comment has been minimized.

Copy link
@pschultz

pschultz Nov 7, 2019

Author Contributor

Hmm, good catch, I haven't thought about that.

HttpClientInterface::OPTIONS_DEFAULTS is the only place where options are documented, isn't it? I don't like removing the json option there.

Also, it's somewhat dangerous to change the meaning of null from "no request body" to "request body containing null". Sure, we could point that out in the changelog, but you probably know as well as I do how good people are at reading changelogs, especially if the client package gets updated as a transitive dependency.

I'm leaning towards not supporting null.

'json' => null, // array|\JsonSerializable - when set, implementations MUST set the "body" option to
// the JSON-encoded value and set the "content-type" headers to a JSON-compatible
// value if they are not defined - typically "application/json"
'json' => null, // mixed - when set, implementations MUST set the "body" option to the JSON-encoded

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 6, 2019

Member

when defined

This comment has been minimized.

Copy link
@pschultz

pschultz Nov 7, 2019

Author Contributor

Changed it to "if set". We either have to remove the default option completely to support null, or null is inconsequential, making "set" the correct term.

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Nov 7, 2019
@nicolas-grekas nicolas-grekas changed the base branch from master to 4.4 Nov 7, 2019
@nicolas-grekas nicolas-grekas changed the base branch from 4.4 to master Nov 7, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 7, 2019

I'm not able to push on your repo to update the patch.
Can you please squash your commits and rebase+retarget for 4.4?

Allow arbitrary values in the "json" request option. Previously values were
limitated to arrays and objects of type JsonSerializable. This doesn't account
for scalar values and classes with public properties (which don't need to
implement JsonSerializable), all of which are perfectly acceptable arguments to
json_encode.
@pschultz pschultz force-pushed the classmarkets:http/arbitrary-json-values branch from 001a502 to e98731a Nov 7, 2019
@pschultz

This comment has been minimized.

Copy link
Contributor Author

pschultz commented Nov 7, 2019

Rebased on 4.4.

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.4 Nov 7, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 7, 2019

Thank you @pschultz.

nicolas-grekas added a commit that referenced this pull request Nov 7, 2019
…pschultz)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpClient] allow arbitrary JSON values in requests

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | n/a
| License       | MIT
| Doc PR        | n/a

Allow arbitrary values in the "json" request option. Previously values were
limitated to arrays and objects of type JsonSerializable. This doesn't account
for scalar values and classes with public properties (which don't need to
implement JsonSerializable), [all of which are perfectly acceptable arguments to
json_encode](https://www.php.net/manual/en/function.json-encode.php):

> value
> The value being encoded. Can be any type except a resource.

It seems silly to expect users to add classes implementing JsonSerializable to represent, say, scalar values or an empty object.

I'm not sure if you consider removed exceptional cases a BC break or not. I'm assuming "no" for now.

Commits
-------

e98731a [HttpClient] allow arbitrary JSON values in requests
@nicolas-grekas nicolas-grekas merged commit e98731a into symfony:4.4 Nov 7, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@pschultz pschultz deleted the classmarkets:http/arbitrary-json-values branch Nov 7, 2019
This was referenced Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.