api-platform / api-platform

🕸️ Create REST and GraphQL APIs, scaffold Jamstack webapps, stream changes in real-time.
https://api-platform.com
MIT License
8.69k stars 962 forks source link

GraphQL: @ApiProperty(required=false) does not work #872

Open ByScripts opened 6 years ago

ByScripts commented 6 years ago

In my Entity, I have a property:

/**
 * @var int The account balance
 *
 * @ORM\Column(type="integer")
 * @ApiProperty(required=false)
 * @Assert\Type(type="integer")
 */
private $balance = 0;

I want this property to be optional in my GraphQL query, but not in database.

So I added the required=false attributes, but it doesn't work.

ByScripts commented 6 years ago

In fact, it seems that no properties are taken in account. When settings field with:

@ApiPropery(readable=false, writable=false)

I still can read/write the property.

lukasluecke commented 5 years ago

@dunglas Is this something that should be changed? I tried implementing this in a basic version in SchemaBuilder::getResourceObjectTypeFields, but noticed that it would interfere with the default behaviour of required / non-null fields (i.e. it would force you to manually specify required for every property).

I think it would be nice being able to just override these assumptions when needed, but right now $propertyMetadata->isRequired() returns either true or false - we would need null or similar as well, to distinguish the case where the required flag was not set at all.

I think another way would be with the new input_class you could create a class where the property was nullable? Not sure if that would work though.

lukasluecke commented 5 years ago

@alanpoulain @dunglas I believe this should be moved to the core repository, if it's supposed to be pursued further? If you think it's a good idea, then I'd be willing to work on this.

alanpoulain commented 5 years ago

I don't understand why we should change how the PropertyMetadata works. The factory should be able to determine if a field is required or not based on the mapping configuration and the API Platform one.

lukasluecke commented 5 years ago

The issue is that the @ApiProperty annotation exists, but does not affect the GraphQL schema. Either we could make it more explicit that it's REST-specific, or make it work with GraphQL?

It's also not only required, but also writable (and maybe others as well). Using input / output might solve the same problem, but then there should be clearer guidance on which solution is preferred / working.

alanpoulain commented 5 years ago

I think we should make it work for GraphQL too, like ApiResource does.

lukasluecke commented 5 years ago

Do you have any ideas how that could best be achieved? I proposed one possibility (at least for required flag), but if there is a better way I'm open to suggestions.

oleg-brizy commented 4 years ago

It becomes optional when I set @ORM\Column(nullable=true) but my column can't be null in db.

https://github.com/api-platform/api-platform/issues/872#issuecomment-455198847 I think another way would be with the new input_class you could create a class where the property was nullable? Not sure if that would work though.

Too much trouble for one property.

pmzandbergen commented 4 years ago

+1

while @ORM\Column(nullable=true) sets the default for a field being nullable (or not) it should be configurable as well.

xterr commented 3 years ago

+1

brettins commented 3 years ago

I am using a boolean with a default, which also should be a case where the GraphQL input is not required.

    /**
     * @ORM\Column(type="boolean",  options={"default": false})
     */
ausi commented 3 years ago

I was able to reproduce the issue with required: false:

#[ORM\Column(type: 'string')]
#[ApiProperty(required: false)]
private string $foo;

This gives me the error Field createBarInput.foo of required type String! was not provided.

However using writable: false works in my case:

#[ORM\Column(type: 'string')]
#[ApiProperty(writable: false)]
private string $foo;

So it doesn’t seem like ApiProperty is ignored completely for GraphQL. But the required setting gets ignored still.

jensstalder commented 2 years ago

So this is still an issue? Is there at least a workaround? For example I am trying to add a property to an entity that does not need to persist to the db at all. But it's still a required field for graphql. At least when creating. when updating this does not seam to be a problem (since the values already exist and just get rehydrated into the entity already).

develth commented 1 year ago

Thats really need to be solved. What are the steps for that? What the blockers? Anyone already analyzed?

bitsbauer commented 1 year ago

In the docs at https://api-platform.com/docs/core/graphql/#securing-properties-including-associations a note says:

"Note: adding the ApiProperty security expression to a GraphQL property will automatically make the GraphQL property type nullable (if it wasn't already). This is because null is returned as the property value if access is denied via the security expression."

So a workaround may be to add some dummy security via ApiProperty-Annotation to the property which would make it optional in GraphQL:

#[ApiProperty(security: "is_granted('ROLE_USER')")]

I had a look and it seems this forces the nullable field via $forceNullable param at https://github.com/api-platform/core/blob/v3.0.9/src/GraphQl/Type/FieldsBuilder.php#L250

private function getResourceFieldConfiguration(?string $property, ?string $fieldDescription, ?string $deprecationReason, Type $type, string $rootResource, bool $input, Operation $rootOperation, int $depth = 0, bool $forceNullable = false): ?array { ... } 

This method is called a little bit up at https://github.com/api-platform/core/blob/v3.0.9/src/GraphQl/Type/FieldsBuilder.php#L216

if ($fieldConfiguration = $this->getResourceFieldConfiguration($property, $propertyMetadata->getDescription(), $propertyMetadata->getDeprecationReason(), $propertyType, $resourceClass, $input, $operation, $depth, null !== $propertyMetadata->getSecurity())) {
   $fields['id' === $property ? '_id' : $this->normalizePropertyName($property, $resourceClass)] = $fieldConfiguration;
}

So maybe just passing null !== $propertyMetadata->getSecurity() || null !== $propertyMetadata->getDefault() instead of just null !== $propertyMetadata->getSecurity() would already be enough but needs to be further investigated ...