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

Requesthandler should support json array without object wrapper #1554

Open
mwitte opened this issue Apr 23, 2019 · 8 comments

Comments

Projects
None yet
4 participants
@mwitte
Copy link

commented Apr 23, 2019

Description

It should be possible to send a json array without object wrapper to a controller action with Content-Type json/application

Steps to Reproduce

curl --header "Content-Type: application/json" --request POST --data '[{"foo":"bar"},{"bar":"foo"}]' https://myhost/myroute

Expected behavior

There should be the possibility to define a name for this special case in the route.

Actual behavior

Neos\Flow\Mvc\Exception\InvalidArgumentNameException:
Invalid argument name (must be a non-empty string). 1210858767
Neos\Flow\Mvc\ActionRequest_Original::setArgument("0", array|1|)

Flow wants to map the element keys as request arguments which makes no sense here. The array should be mapped to an argument name which should be configurable in the route.

Affected Versions

All versions

@albe

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

This would indeed be helpful for building e.g. an API endpoint receiving multiple entities and avoiding the currently necessary wrapping of request body in an object (i.e. --data '{ "foos": [{"foo":"bar"}, {"bar":"foo"}] }')

The place to hook into would be here:
https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/Mvc/DispatchComponent.php#L102

Could look something like this:

$bodyArgumentName = $componentContext->getParameter(Routing\RoutingComponent::class, 'bodyArgumentName');
if ($bodyArgumentName !== null) {
    $arguments = Arrays::arrayMergeRecursiveOverrule($arguments, [$bodyArgumentName => $parsedBody]);
} else {
    $arguments = Arrays::arrayMergeRecursiveOverrule($arguments, $parsedBody);
}

Routes.yaml:

-
  name: 'my multi value receiving endpoint'
  uriPattern: '/foo/bar'
  defaults:
    '@controller': 'Foo'
    '@action':     'bar'
    '@format':     'json'
  httpMethods: ['POST']
  bodyArgumentName: 'foos'

wdyt @bwaidelich ?

@albe albe added the Feature label Apr 24, 2019

@bwaidelich

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

IMO what you call "body argument name" has got nothing to do with routing really, does it?
It does not affect which endoint is triggered etc.
Instead I could imagine that we just allow this (and don't map it to ActionRequest arguments) and one could retrieve the "body arguments" via the HTTP request (I think it's ServerRequestInterface:: getParsedBody()

@albe

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Yeah, my first feeling too was that it doesn't quite belong into routing, but then again, you might want to configure the "body argument name" per route effectively, because that's where you map the outside world to your internal world.

But it's correct that this probably only needs configuration per action, rather then per route. So where would you put that configuration? I'm reluctant to create a mapping of "Controller:Action" => "bodyArgumentName" somewhere in settings and the other alternative I could come up with would be a method Annotation like @Flow\ParsedBody(name="foo").

@bwaidelich

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

So where would you put that configuration?

Sorry if I wasn't clear: I suggested not to have this configuration at all but allow the action to get hold of all the parsed request body if it needs to (and IIRC that is already possible via $this->request->getHttpRequest()->getParsedBody())

@kitsunet

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I agree, either that or a way to configure that as part of the property mapping step.

@albe

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

but allow the action to get hold of all the parsed request body if it needs to

That's always possible ofc, but I think this issue is more about allowing to somehow map a "list of things" from the body in an MVC context like is done for any other request argument, gaining from the automatic PropertyMapping, instead of needing a special access path. i.e. "As a developer I just want to write my action like this: receiveManyAction(array $listOfThings)" and gladly receive properly converted entities, because the argument is properly annotated with an Element typehint.

I agree, either that or a way to configure that as part of the property mapping step.

PropertyMapping Configuration could be a good solution, indeed. I'll investigate.

@albe albe self-assigned this May 22, 2019

@albe

This comment has been minimized.

Copy link
Member

commented May 26, 2019

Ok, so after a bit of investigation, PropertyMappingConfiguration won't work, because it happens too late. What is needed to allow this, is an way to merge the parsed body differently into the request arguments.

currently:

$parsedBody = $this->parseRequestBody($httpRequest);
if ($parsedBody !== []) {
    $arguments = Arrays::arrayMergeRecursiveOverrule($arguments, $parsedBody);
    ...
}

with the PSR-7 change this is done inside the ActionRequest constructor:
https://github.com/neos/flow-development-collection/pull/1552/files#diff-769dd63c742a292e80f74b6644a5e2e1R26

After this merge, it would not be possible (or very hard) to extract the original parsed body and retroactively assign it to an action argument, and remove all subproperties from the merged arguments without losing possibly existing previous values.

e.g.

POST /acme/do?foo=notbar { "foo": "bar", "baz": "quux" }
parsedBody should be assigned to action parameter MyObject $body
$arguments = ['foo' => 'notbar'];
$parsedBody = [ 'foo' => 'bar', 'baz' => 'quux'];
...merge...
$arguments = ['foo' => 'bar', 'baz' => 'quux'];
...expected...
$arguments = ['foo' => 'notbar', 'body' => ['foo' => 'bar', 'baz' => 'quux']];

So what I see as possibilities:

  • do that in the MVC layer only, which means some way to signal from the action itself to assign the request body, e.g. via an Annotation. However, that means we need to resolve the Controller before creating the ActionRequest (doable, because that happens after routing https://github.com/neos/flow-development-collection/pull/1552/files#diff-e1f97733ccfb3c59b5ed49656f9835d2R38)
  • do that in the HTTP layer via an component (or inside the new PrepareMvcRequestComponent) via some static configuration that maps via RequestPatterns to requestBodyName (though I think this is a bit over-engineered tbh. - you probably don't need to map the body differently depending on Ip or Host)
@albe

This comment has been minimized.

Copy link
Member

commented May 30, 2019

I pushed a PoC which uses a new method annotation to specify the argument to map the request body into. I basically ignore the ActionRequest arguments and directly map the request->getHttpRequest()->getParsedBody() inside mapRequestArgumentsToControllerArguments() when this flag is set.
With the change as is, this could now also be done with the PropertyMappingConfiguration, but I find the annotation a bit more straightforward. Open for opinions there though.

The only drawback (or maybe feature?) is, that with this, the request body could be mapped doubly:

POST /api/v1/foo { "name": "foo1", ... }
/**
 * @param Foo $foo
 * @param string $name
 * @Flow\MapRequestBody("$foo")
 */
public function fooAction(Foo $foo, string $name)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.