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

Use default value of PHP properties to Swagger doc #2386

Merged
merged 8 commits into from Nov 29, 2019

Conversation

aaa2000
Copy link
Contributor

@aaa2000 aaa2000 commented Dec 16, 2018

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #2363
License MIT
Doc PR

Add default and example information to swagger context if php properties
have default values so the exemple documentation section use these values.

With a class

<?php

namespace App\Entity;

use ApiPlatform\Core\Annotation\ApiProperty;
use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\ORM\Mapping as ORM;
use Ramsey\Uuid\UuidInterface;
use Symfony\Component\Serializer\Annotation\Groups;

/**
 * @ORM\Entity
 * @ApiResource(
 *     normalizationContext={"groups": {"person:read"}}
 * )
 * @ApiResource
 */
class Person
{
    /**
     * @var UuidInterface
     *
     * @ApiProperty(identifier=true)
     * @Groups({"person:read"})
     *
     * @ORM\Id
     * @ORM\Column(type="uuid", unique=true)
     * @ORM\GeneratedValue(strategy="CUSTOM")
     * @ORM\CustomIdGenerator(class="Ramsey\Uuid\Doctrine\UuidGenerator")
     */
    public $code;

    /**
     * @var bool
     *
     * @Groups({"person:read", "person:write"})
     *
     * @ORM\Column(type="boolean", options={"default" : false})
     */
    public $isActive = true;

    /**
     * @var integer
     *
     * @Groups({"person:read", "person:write"})
     *
     * @ORM\Column(type="integer", nullable=true)
     */
    public $int = 100;

    /**
     * @var array
     *
     * @Groups({"person:read", "person:write"})
     *
     * @ORM\Column(type="json_array", nullable=true)
     */
    public $array = ['aa', 'bb'];

    /**
     * @var string
     *
     * @ApiProperty(
     *     attributes={
     *         "swagger_context"={
     *             "example"="example string"
     *         }
     *     }
     * )
     *
     * @Groups({"person:read", "person:write"})
     *
     * @ORM\Column(nullable=true)
     */
    public $string = 'aaaaaa';
}

The swagger documentation displays

api_platform_default_value

@aaa2000 aaa2000 force-pushed the swagger-default-property-value branch 3 times, most recently from d30616f to 9fa9635 Compare December 17, 2018 20:38
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement! I made some design comments but I like the idea!

@soullivaneuh
Copy link
Contributor

@dunglas Won't 2.3 be a better branch target for this?

@dunglas
Copy link
Member

dunglas commented Dec 20, 2018

@soullivaneuh no because it's a new feature.

@aaa2000 aaa2000 force-pushed the swagger-default-property-value branch 4 times, most recently from e2e37b3 to c2a32e7 Compare December 22, 2018 17:21
@aaa2000
Copy link
Contributor Author

aaa2000 commented Dec 22, 2018

@dunglas PR updated to introduce the default and example properties in ApiProperty

@@ -74,6 +74,10 @@
<argument type="service" id="api_platform.metadata.property.metadata_factory.cached.inner" />
</service>

<service id="api_platform.metadata.property.metadata_factory.default_property" decorates="api_platform.metadata.property.metadata_factory" decoration-priority="30" class="ApiPlatform\Core\Metadata\Property\Factory\DefaultPropertyMetadataFactory" public="false">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decoration-priority="30" is correct ?

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/Annotation/ApiProperty.php Outdated Show resolved Hide resolved
src/Annotation/ApiProperty.php Outdated Show resolved Hide resolved
@aaa2000
Copy link
Contributor Author

aaa2000 commented Jan 7, 2019

@meyerbaptiste done

@aaa2000 aaa2000 force-pushed the swagger-default-property-value branch from 4f541a1 to 5ff77d5 Compare January 19, 2019 12:17
@aaa2000 aaa2000 force-pushed the swagger-default-property-value branch from 5ff77d5 to 78e5a0a Compare October 5, 2019 14:46
@soyuka
Copy link
Member

soyuka commented Oct 6, 2019

@aaa2000 aaa2000 force-pushed the swagger-default-property-value branch 5 times, most recently from fc50d2d to 5395ec6 Compare October 20, 2019 15:36
@aaa2000
Copy link
Contributor Author

aaa2000 commented Oct 20, 2019

I also added the Doctrine default value but I'm not sure it's a good idea because with postgres, Doctrine insert a null value for the column, and there is the error
ERROR: null value in column "string" violates not-null constraint

I can use Doctrine default value as example value but a doctrine annotation @ORM\Column(name="created_at", type="datetime", options={"default": "CURRENT_TIMESTAMP"}) will display CURRENT_TIMESTAMP as example. Finally, I do not think that this is desirable to use the Doctrine default value

@soyuka
Copy link
Member

soyuka commented Oct 22, 2019

I also added the Doctrine default value but I'm not sure it's a good idea because with postgres, Doctrine insert a null value for the column, and there is the error
ERROR: null value in column "string" violates not-null constraint

Not sure I got that, what is behind this insert?

About the second point, thoughts @api-platform/core-team ?

@aaa2000
Copy link
Contributor Author

aaa2000 commented Oct 27, 2019

Given an entity with Doctrine default value

<?php

namespace App\Entity;

use ApiPlatform\Core\Annotation\ApiProperty;
use ApiPlatform\Core\Annotation\ApiResource;
use Doctrine\ORM\Mapping as ORM;
use Ramsey\Uuid\UuidInterface;

/**
 * @ORM\Entity
 * @ApiResource
 */
class Bug
{
    /**
     * @var UuidInterface
     *
     * @ApiProperty(identifier=true)
     *
     * @ORM\Id
     * @ORM\Column(type="uuid", unique=true)
     * @ORM\GeneratedValue(strategy="CUSTOM")
     * @ORM\CustomIdGenerator(class="Ramsey\Uuid\Doctrine\UuidGenerator")
     */
    public $code;


    /**
     * @var string
     *
     * @ORM\Column(options={"default": "example name"})
     */
    public $name;
}

When I send the request curl -X POST "http://localhost:8080/bugs" -H "accept: application/ld+json" -H "Content-Type: application/ld+json" -d "{}"
Then I have the error

{
  "@context": "/contexts/Error",
  "@type": "hydra:Error",
  "hydra:title": "An error occurred",
  "hydra:description": "An exception occurred while executing 'INSERT INTO bugs (code, name) VALUES (?, ?)' with params [\"4387fe5d-ce66-4783-a310-33256cd9c294\", null]:\n\nSQLSTATE[23502]: Not null violation: 7 ERROR:  null value in column \"name\" violates not-null constraint\nDETAIL:  Failing row contains (4387fe5d-ce66-4783-a310-33256cd9c294, null).",

To have the default value inserted with postgres, the query should be INSERT INTO bugs (code, name) VALUES ('4387fe5d-ce66-4783-a310-33256cd9c294', default );

So, it is maybe better to use the Doctrine default value as example instead of default property metadata and change https://github.com/api-platform/core/pull/2386/files#diff-fbaaae4f33f38c0ec0e318994cb386faR82 $propertyMetadata = $propertyMetadata->withDefault($fieldMapping['options']['default']); by $propertyMetadata = $propertyMetadata->withExample($fieldMapping['options']['default']);

@soyuka
Copy link
Member

soyuka commented Nov 1, 2019

👍 if you can fix this and add a test case for it

@flug
Copy link
Contributor

flug commented Nov 14, 2019

@aaa2000 you can fix et add test please for merge this pull request ?

@aaa2000
Copy link
Contributor Author

aaa2000 commented Nov 24, 2019

I can't fix, the fix should be done in doctrine. The doctrine ORM doesn't take into account the options={"default": "example name"} when it create the insertion. The options seems to be used only for creating the database schema

@soyuka
Copy link
Member

soyuka commented Nov 25, 2019

Okay nice, we'll open an issue to track this. I the meantime I'm 👍 to merge this.

@soyuka soyuka force-pushed the swagger-default-property-value branch from 5395ec6 to a83c118 Compare November 25, 2019 10:51
@soyuka soyuka merged commit b5fae3a into api-platform:master Nov 29, 2019
@soyuka
Copy link
Member

soyuka commented Nov 29, 2019

Thanks for the work @aaa2000

soyuka added a commit that referenced this pull request Nov 29, 2019
@teohhanhui
Copy link
Contributor

I can't fix, the fix should be done in doctrine. The doctrine ORM doesn't take into account the options={"default": "example name"} when it create the insertion. The options seems to be used only for creating the database schema

It's not a bug. That's exactly what it's intended for, the default value at the database level. It'd be a bug if Doctrine ORM actually tries to use it. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants