Skip to content

Adjust null handling in exported JSON Schema / OpenAPI - #3518

Closed
Korbeil wants to merge 2 commits into
api-platform:mainfrom
Korbeil:feature/add-nullable-to-property
Closed

Adjust null handling in exported JSON Schema / OpenAPI#3518
Korbeil wants to merge 2 commits into
api-platform:mainfrom
Korbeil:feature/add-nullable-to-property

Conversation

@Korbeil

@KorbeilKorbeil commented Apr 23, 2020

Copy link
Copy Markdown
Contributor
QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
TicketsN/A
LicenseMIT
Doc PRN/A

#3402 was a really good start to have a better null handling in exported schema.

But actually we should use required attribute as a condition for nullability.
A non-required property should be nullable.

@KorbeilKorbeil changed the title Better nullable fields declaration in exported SwaggerBetter nullable properties in exported OpenAPIApr 23, 2020
@Korbeil
Korbeilforce-pushed the feature/add-nullable-to-property branch 5 times, most recently from ba308b4 to 2fc14d6CompareApril 23, 2020 17:44
@KorbeilKorbeil changed the title Better nullable properties in exported OpenAPI[WIP] Better nullable properties in exported OpenAPIApr 23, 2020
@Korbeil
Korbeilforce-pushed the feature/add-nullable-to-property branch from 2fc14d6 to bf4b2b0CompareApril 23, 2020 21:21
@KorbeilKorbeil changed the title [WIP] Better nullable properties in exported OpenAPI[WIP] Better nullable properties in exported JSON Schema / OpenAPIApr 23, 2020
@KorbeilKorbeil changed the title [WIP] Better nullable properties in exported JSON Schema / OpenAPIBetter nullable properties in exported JSON Schema / OpenAPIApr 23, 2020
}

$valueSchema = $this->typeFactory->getType(new Type($builtinType, $type->isNullable(), $className, $isCollection), $format, $propertyMetadata->isReadableLink(), $serializerContext, $schema);
$valueSchema = $this->typeFactory->getType(new Type($builtinType, !$propertyMetadata->isRequired() || $type->isNullable(), $className, $isCollection), $format, $propertyMetadata->isReadableLink(), $serializerContext, $schema);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Being nullable is not enought here, we also need to check if the property is required.
If it's not required, it means this property should be nullable.

@KorbeilKorbeil changed the title Better nullable properties in exported JSON Schema / OpenAPIBetter null handling in exported JSON Schema / OpenAPIApr 23, 2020
@Korbeil
Korbeilforce-pushed the feature/add-nullable-to-property branch 3 times, most recently from 82ba090 to a2cfac2CompareApril 23, 2020 21:59
@KorbeilKorbeil changed the title Better null handling in exported JSON Schema / OpenAPIAdjust null handling in exported JSON Schema / OpenAPIApr 24, 2020
@soyuka
soyuka requested a review from teohhanhuiApril 28, 2020 08:47
Base automatically changed from master to mainJanuary 23, 2021 21:59
@alanpoulain
alanpoulainforce-pushed the feature/add-nullable-to-property branch from a2cfac2 to ede900fCompareMarch 4, 2021 16:15
@alanpoulain
alanpoulainforce-pushed the feature/add-nullable-to-property branch from ede900f to 84e09daCompareMarch 9, 2021 10:05
@alanpoulain

Copy link
Copy Markdown
Member

Should it be really the case though?
If a property is not required but has a default value, it shouldn't be nullable, isn't it?

@stale

staleBot commented Nov 4, 2022

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stalestaleBot added the wontfix label Nov 4, 2022
@stale

staleBot commented Jan 3, 2023

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stalestaleBot added the stale label Jan 3, 2023
@stalestaleBot closed this Jan 10, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Korbeil@alanpoulain@soyuka