Skip to content

Make "hydra:mapping" item's property nullable #3877

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

Merged
merged 5 commits into from
Dec 3, 2020
Merged

Make "hydra:mapping" item's property nullable #3877

merged 5 commits into from
Dec 3, 2020

Conversation

GaryPEGEOT
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no

Context

My entites have the following filter, to select properties. Parameter name have been overrode.
* @ApiFilter(PropertyFilter::class, arguments={"parameterName": "fields"})

And I got the following error in my tests, using assertMatchesResourceCollectionJsonSchema assertion

   │             ...
   │             3 => Array &9 (
   │                 '@type' => 'IriTemplateMapping'
   │                 'variable' => 'fields[]'
   │                 'property' => null
   │                 'required' => false
   │             )
   │             ...
   │     )
   │ ) matches the provided JSON Schema.
   │ hydra:search.hydra:mapping[3].property: NULL value found, but a string is required

@GaryPEGEOT
Copy link
Contributor Author

GaryPEGEOT commented Dec 3, 2020

Turns out justinrainbow/json-schema doesn't seems to support nullable properties: jsonrainbow/json-schema#551

@@ -121,7 +121,10 @@ public function buildSchema(string $className, string $format = 'jsonld', string
'properties' => [
'@type' => ['type' => 'string'],
'variable' => ['type' => 'string'],
'property' => ['type' => 'string'],
'property' => [
'nullable' => true,
Copy link
Member

Choose a reason for hiding this comment

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

'nullable' => true is only for OpenAPI < 3.1.
I don't think we should add it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. the change from string to 'type' => ['string', 'null'], broke Behat pipelien, I'll update the tests as well if you're OK with the change ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think this change is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that 'null' is valid type starting on Open-API 3.1 and the schema generated by the api:openapi:export command is at "3.0.2", so it wont pass swagger-cli validation (which is normal). I could check the version to add nullable or ['string', 'null'] combo, but I don't think you have control over the generated minor version. Any idea on how to solve this ?

Copy link
Member

@alanpoulain alanpoulain Dec 3, 2020

Choose a reason for hiding this comment

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

It's more or less what it has been done here:

private function addNullabilityToTypeDefinition(array $jsonSchema, Type $type, ?Schema $schema): array
{
if ($schema && Schema::VERSION_SWAGGER === $schema->getVersion()) {
return $jsonSchema;
}
if (!$type->isNullable()) {
return $jsonSchema;
}
if (\array_key_exists('$ref', $jsonSchema)) {
return [
'nullable' => true,
'anyOf' => [$jsonSchema],
];
}
if ($schema && Schema::VERSION_JSON_SCHEMA === $schema->getVersion()) {
return array_merge(
$jsonSchema,
[
'type' => \is_array($jsonSchema['type'])
? array_merge($jsonSchema['type'], ['null'])
: [$jsonSchema['type'], 'null'],
]
);
}
return array_merge($jsonSchema, ['nullable' => true]);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed ! I've kept nullable for openapi version, but if it's not necessary I'll move it to a ternary.

$mappingPropDefinition = ['type' => 'string', 'nullable' => true];
break;
default:
$mappingPropDefinition = ['type' => 'string'];
Copy link
Member

Choose a reason for hiding this comment

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

Could you initialize the property before the switch and rename it $nullableStringDefinition? In case we want to reuse it elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@alanpoulain alanpoulain merged commit d4420d2 into api-platform:2.5 Dec 3, 2020
@alanpoulain
Copy link
Member

Thank you @GaryPEGEOT.

@GaryPEGEOT GaryPEGEOT deleted the patch-2 branch December 3, 2020 18:00
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.

2 participants