kleijnweb / swagger-bundle

Makes your OpenAPI (FKA Swagger) definition into a Symfony routing and validation spec
GNU Lesser General Public License v3.0
24 stars 13 forks source link

Attributes not being mapped to controller #115

Open nesl247 opened 6 years ago

nesl247 commented 6 years ago

I'm having an issue with the mapping of the request body to the controller action. Currently it's telling me my first parameter is null and thus it can't set it.

For example:

public function __invoke(string $slug, string $type, string $group, bool $private, array $label): Response
parameters:
        - in: body
          name: body
          schema:
            type: object
            properties:
              uuid:
                type: string
                format: uuid
              slug:
                type: string
              private:
                type: boolean
                default: false
              group:
                type: string
              type:
                $ref: '#/definitions/attribute-types'
              label:
                type: object
                description: Label for the attribute visible to the end user.
                required:
                  - global
                properties:
                  global:
                    type: object
                  store:
                    type: object
              validationRules:
                type: array
                items: {}
            required:
              - slug
              - group
              - type
              - label
            example:
              uuid: c02721b3-64d9-400d-b6ae-beb25eab8653
              slug: memory
              private: true
              group: technical
              type: string
              label:
                global:
                  en-US: Memory
                  es-US: Memoria
                store:
                  mx:
                    es-MX: Memoria (RAM)
app.CRITICAL: Internal Server Error [logref 5aa800200ffd0]: RuntimeException: Controller "CreateAttribute" requires that you provide a value for the "$slug" argument. Either the argument is nullable and no null value has been provided, no default value has been provided or because there is a non optional argument after this one.

Am I doing something wrong, or is there a bug?

kleijnweb commented 6 years ago

The problem here is that you defined an object parameter with the name body. The controller then needs to have an object parameter with the name body.

Probably what you want is to add an argument passing the whole object to the controller. You can either turn off hydration and expect a stdClass object, or use a $ref in the param definition and create a DTO in PHP.

Option 1:

public function __invoke(\stdClass $body): \stdClass // Don't return Response objects, SwaggerBundle will create them
{
  // Do stuff
}

Option 2:

paths:
  /some-path
     parameters:
        - in: body
          name: myObject
          schema: { $ref: '#/definitions/MyDtoType' }

definitions:
  MyDtoType:
    type: object
    properties:
        slug:
          type: string
   # Etc.
class MyDtoType
{
    private $slug;
    // Etc..
}
public function __invoke(MyDtoType $myObject): MaybeSomeOtherType // Don't return Response objects, SwaggerBundle will create them
{
  // Do stuff
}

See:

nesl247 commented 6 years ago

Unless I'm misunderstanding something, I think swagger-bundle may be behaving differently than the swagger spec.

From https://swagger.io/docs/specification/2-0/describing-request-body/: Note: The name of the body parameter is ignored. It is used for documentation purposes only.

I don't think that body name should be used at all. The value of body is actually coming from stoplight.io which is what we use to build our documentation.

kleijnweb commented 6 years ago

Well, you are free to name it anything you want (including body). But in order to map it to a controller argument, the names need to match. This is desired behavior, otherwise Symfony won't be able to determine how the arguments should be ordered when invoking the method. You can still have path and query parameters, after all.

The parameter -> controller method argument mapping internals is not actually SwaggerBundle behavior, but Symfony's FrameworkBundle doing. SwaggerBundle just sets the appropriate attributes on the request object.

nesl247 commented 6 years ago

My point is that the body parameter should not be referenced by swagger-bundle at all. Instead, the attributes that would be created would be top level. For example slug as I had it would be an attribute symfony is aware of, not body.

Symfony matches based on naming, not based on order. Therefore simply adding them all as attributes $request->attributes->set($name, $value).

kleijnweb commented 6 years ago

I see what you are saying but I feel like you would be better off just naming the object body in your case. The name is required anyway. Creating a separate attribute/argument for every property in the body increases the chance of collisions.

Consider this example where the path contains the original slug, and the body contains a new value for slug:

parameters:
  - in: path
    name: slug
    type: string
  - in: body
    name: body
    type: object
       properties:
         name: { type: string }
         slug: { type: string }

Both attributes (and thus arguments) would be named slug and the second would overwrite the first (probably causing a 404 by application code).

nesl247 commented 6 years ago

Oh I agree, which is why I completely dislike using the request attributes except when using object hydration.

I think that the json should be decoded and added to $request->request just like normal, and leave the attributes for hydrated values.

This let's us keep in line with Symfony 100%. It also means controllers are no longer dependent upon swagger-bundle.

kleijnweb commented 6 years ago

You can already have the request passed if you really want:

public function __invoke(Request $request)
{
  // Do stuff with $request->attributes->get('body'); and whatever else
}

And you could also return Response objects, but the event listeners that SwaggerBundle uses for dehydration, response validation, and serialization won't work, due a limitation of the HttpKernel:

// call controller
 $response = call_user_func_array($controller, $arguments);

// view
if (!$response instanceof Response) {
   // Dispatch more events
}

You can, if you want use SwaggerBundles ResponseFactory directly or indirectly via some more generic component in your controllers to produce the Reponse object, instead of having the ViewListener do it.

nesl247 commented 6 years ago

I know I can use the Request object already, but it is not being used in a standard way due to the request ParameterBag not being populated and instead the attributes ParameterBag is.

I'm actually not planning on using the dehydration, response validation or serialization functionality. I see that as more RAD functionality and we are not looking to do that.

Is there any reason though to not populate the request ParameterBag and leave attributes specifically for hydrated values and query parameters as is standard in Symfony?

kleijnweb commented 6 years ago

The routing takes care of populating the "standard" request parameter bags. These are still accessible as they would be with any other RouteLoader. The request parameter bag in Symfony is populated using the $_POST superglobal, and will only be populated when using form encoded POST requests, not when posting JSON (See Symfony's Request::createFromGlobals()).

So it makes no sense putting deserialized JSON in there. In the standard way, decoding a JSON post in your controller would have to look like this:

$json = json_decode($request->getContents());

You can still do that if you want, but why would you if you can just use the body parameter?

Even with hydration turned off, SwaggerBundle will do parameter coercion, to make for example "0" into FALSE, if dictated by the schema. The result of the coercion is added as an attribute to keep the original value available in the Request object. There is a possible collision with path parameters, but only if you declare a type other than string which is unlikely.