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.

Sign up for GitHub

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

Jump to bottom

fix(graphql): Embedded nullable relations graphql #6100

Merged
merged 1 commit into from
Feb 20, 2024
Merged

fix(graphql): Embedded nullable relations graphql #6100

merged 1 commit into from
Feb 20, 2024

Conversation

Koenstell
Copy link
Contributor

@Koenstell Koenstell commented Jan 10, 2024 •
edited
Loading

Q A
Branch? main
Tickets api-platform/api-platform#2562
License MIT

We have a number of cases where we support creating new entities through a relationship with a 'parent' entity. This works fine, but the prop is always marked as 'required' in the GraphQL types that are being generated. This PR addresses this issue and will look for #[ApiProperty(required: false)] or #[ORM\JoinColumn(nullable: true)] to mark the input as non-required in the GraphQL type.

@Koenstell Koenstell force-pushed the fix/embedded-nullable-relations-graphql branch from c3779ea to 384b648 Compare January 10, 2024 14:09
@Koenstell Koenstell mentioned this pull request Jan 10, 2024
Open
soyuka
soyuka reviewed Jan 11, 2024
View reviewed changes
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

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

Nice fix, would it be possible to check for a change in the behat features (add a test that checks the schema where a property isn't required)?

src/GraphQl/Type/TypeBuilder.php Outdated Show resolved Hide resolved
src/GraphQl/Type/TypeBuilder.php Outdated Show resolved Hide resolved
src/GraphQl/Type/TypeBuilder.php Outdated Show resolved Hide resolved
@Koenstell Koenstell force-pushed the fix/embedded-nullable-relations-graphql branch from 384b648 to 00f8d50 Compare January 16, 2024 15:41
@Koenstell
Copy link
Contributor Author

I've added a behat test for this, and while doing this I found an edge-case that wasn't taken care of. Namely, if you have 2 references to the same type, it will 'cache' the type definition (including the required/not required attribute). I had to throw a few things around in TypeBuilder::getResourceObjectType to accommodate for that.

Next up I will refactor the required argument and double-check cs-fixer/phpstan.

@Koenstell Koenstell force-pushed the fix/embedded-nullable-relations-graphql branch 3 times, most recently from f2e3b95 to d15faef Compare January 17, 2024 10:33
@Koenstell
Copy link
Contributor Author

@soyuka I've addressed everything I believe. Not sure why the Behat (PHP8.3) (Symfony lowest) test is failing, other Behat tests are working and the error doesnt make sense to me?

Please review the changes whenever you can.

soyuka
soyuka approved these changes Jan 17, 2024
View reviewed changes
src/GraphQl/Type/TypeBuilder.php Show resolved Hide resolved
src/GraphQl/Type/TypeBuilder.php Outdated Show resolved Hide resolved
src/GraphQl/Type/TypeBuilderEnumInterface.php Outdated Show resolved Hide resolved
@Koenstell Koenstell force-pushed the fix/embedded-nullable-relations-graphql branch 3 times, most recently from e70e4f6 to 0d95307 Compare January 23, 2024 09:54
@Koenstell
Copy link
Contributor Author

@soyuka Thanks for the review, i've addressed the deprecating of the TypeBuilderEnumInterface please check if it looks good. Some of the tests seem to fail but it feels like it's because of other changes in the 3.2 branch where i branched of from, if not, please let me know.

@Koenstell Koenstell requested a review from soyuka January 24, 2024 08:12
@Koenstell Koenstell force-pushed the fix/embedded-nullable-relations-graphql branch from 0d95307 to 3055b7a Compare January 24, 2024 13:58
@Koenstell
Copy link
Contributor Author

@soyuka Any chance you can re-review this?

@soyuka
Copy link
Member

soyuka commented Feb 16, 2024

Lowest is green on 3.2 so I think this is related. You need to reproduce the steps of the CI, basically --prefer-lowest it looks like the schema changes. Also a rebase of 3.2 should fix cs

soyuka
soyuka reviewed Feb 16, 2024
View reviewed changes
src/GraphQl/Type/FieldsBuilder.php
{
if ($typeBuilder instanceof TypeBuilderInterface) {
@trigger_error(sprintf('$typeBuilder argument of FieldsBuilder implementing "%s" is deprecated since API Platform 3.1. It has to implement "%s" instead.', TypeBuilderInterface::class, TypeBuilderEnumInterface::class), \E_USER_DEPRECATED);
}
if ($typeBuilder instanceof TypeBuilderEnumInterface) {
@trigger_error(sprintf('$typeBuilder argument of TypeConverter implementing "%s" is deprecated since API Platform 3.3. It has to implement "%s" instead.', TypeBuilderEnumInterface::class, ContextAwareTypeBuilderInterface::class), \E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

Perfect, but this means that we need to target main we wouldn't want to introduce deprecations in a patch release (3.3 will come out soon).

soyuka
soyuka approved these changes Feb 16, 2024
View reviewed changes
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

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

Quite nice, few leftovers, I know the lowest test that breaks isn't fun to fix I'm not sure why the schema changed only on the lowest branch.

@Koenstell Koenstell force-pushed the fix/embedded-nullable-relations-graphql branch from 3055b7a to c7d1769 Compare February 19, 2024 14:23
@Koenstell Koenstell changed the base branch from 3.2 to main February 19, 2024 14:24
@Koenstell Koenstell force-pushed the fix/embedded-nullable-relations-graphql branch from c7d1769 to 469e300 Compare February 19, 2024 14:27
@Koenstell Koenstell force-pushed the fix/embedded-nullable-relations-graphql branch from 469e300 to 7675366 Compare February 19, 2024 14:50
@Koenstell
Copy link
Contributor Author

@soyuka Thanks again. I've rebased&r my branch on main and pointed the PR to main and somehow the lowest test now succeeds, but the only test failing now is PHPUnit (PHP 8.3) (Symfony dev). I tested locally and it seems to be failing on main as well for the same test so it feels unrelated to my changes.

Please check if all looks good now :) And again, your help is much appreciated.

@soyuka soyuka merged commit 89c9229 into api-platform:main Feb 20, 2024
68 of 72 checks passed
soyuka added a commit to soyuka/core that referenced this pull request Feb 20, 2024
@soyuka
test(graphql): odm fixture api-platform#6100
b6680af
@aegypius aegypius mentioned this pull request Feb 20, 2024
Closed
soyuka added a commit that referenced this pull request Feb 20, 2024
@soyuka
test(graphql): odm fixture #6100 (#6168)
7694219
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soyuka soyuka soyuka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Koenstell @soyuka