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

fix(graphql): link relations requires the property #4931

Closed

Conversation

webda2l
Copy link
Contributor

@webda2l webda2l commented Sep 8, 2022

Q A
Branch? 2.7
Tickets #4613 (comment) & #4613 (comment)
License MIT
Doc PR api-platform/docs#...
  • Add tests

@develth
Copy link
Contributor

develth commented Nov 2, 2022

Nice work!
That also fixes an issue that lihnkHandler include relations that sould ne included.
Could we get this out of draft stage to get it released?

@develth
Copy link
Contributor

develth commented Nov 2, 2022

It just need a few adaptions to work for current state of code

@alanpoulain
Copy link
Member

@develth This PR needs Behat tests. I also need to understand what it does exactly. If you would you can create another PR with them. Also it should target 3.0, not 2.7.

@develth
Copy link
Contributor

develth commented Nov 2, 2022

I also need to understand what it does exactly

Same for me :)
Had issues that graphQL SubQuery added links for not relevant properties, tried to apply that and it worked. So i could not explain - sorry

@develth
Copy link
Contributor

develth commented Nov 2, 2022

Maybe @webda2l could add some context about why it fixes it.

@webda2l
Copy link
Contributor Author

webda2l commented Nov 2, 2022

Query like

{
  videos {
    collection {
      id
      materials {
          id
          slug
      }
      bodyParts {
          id
          slug
      }
    }
  }
}

With code like

#[APIP\ApiResource(
    normalizationContext: ['groups' => ['video:read', 'propKey:read', 'propValue:read', 'teacher:read', 'timestamp']],
    order: ['createdAt' => 'DESC'],
    paginationItemsPerPage: 12,
    operations: [new APIP\Get()],
    graphQlOperations: [
        new APIP\GraphQl\QueryCollection(
            name: 'collection_query',
            paginationType: 'page'
        ),
    ],
)]
class Video
{
...
    #[Groups(['video:read'])]
    #[ORM\ManyToMany(targetEntity: PropertyValue::class)]
    #[ORM\JoinTable(name: 'videos_materials')]
    #[ORM\JoinColumn(name: 'video_uuid', referencedColumnName: 'uuid')]
    #[ORM\InverseJoinColumn(name: 'videopropvalue_id', referencedColumnName: 'id')]
    private Collection $materials;

    #[Groups(['video:read'])]
    #[ORM\ManyToMany(targetEntity: PropertyValue::class)]
    #[ORM\JoinTable(name: 'videos_bodyparts')]
    #[ORM\JoinColumn(name: 'video_uuid', referencedColumnName: 'uuid')]
    #[ORM\InverseJoinColumn(name: 'videopropvalue_id', referencedColumnName: 'id')]
    private Collection $bodyParts;
...
}

#[APIP\ApiResource(
    normalizationContext: ['groups' => ['propValue:read']],
    paginationEnabled: false,
    operations: [new APIP\Get()],
    graphQlOperations: [
        new APIP\GraphQl\QueryCollection(
            name: 'collection_query',
            paginationType: 'page'
        ),
    ],
)]
...
class PropertyValue
{
...

is broken on 2.7/3.0. Due to the fact that links on a same entity (PropertyValue) but on a different property (materials & bodyParts) are not differentiated in the new codebase.

And so, depends on the order of the gql "materials then bodyParts" or "bodyParts then materials", the results of both of them will always be the latest.

{
  videos {
    collection {
      id
      materials {
          id
          slug
      }
      bodyParts {
          id
          slug
      }
    }
  }
}

--or--

{
  videos {
    collection {
      id
      bodyParts {
          id
          slug
      }
      materials {
          id
          slug
      }
    }
  }
}

This PR works but yes, a core dev could do a better fix with full understanding of the codebase I think.

@develth
Copy link
Contributor

develth commented Nov 7, 2022

@webda2l do you update this PR or should i create a new?

develth added a commit to develth/api-platform-core that referenced this pull request Nov 7, 2022
develth added a commit to develth/api-platform-core that referenced this pull request Nov 7, 2022
@develth
Copy link
Contributor

develth commented Nov 7, 2022

@alanpoulain got it reproduced and fix with this commit in my PR-draft (#5169)

It Happens as soon a Entity has an OneToMany Relation to an Item, that i s already realted on the current Entity:
https://github.com/api-platform/core/pull/5169/files#diff-08042a64a511694e2c545567a923e34fe7ca4ea7910432dc8fbc6d11851e33a7R109

GraphQL Query:
https://github.com/api-platform/core/pull/5169/files#diff-f54e74012b91b2ca0d153f08032a62a7d2aac6135901caa47fa4e05a87cf5127R453

By updated Dummy.php i had some struggles with other tests and currently do not want to add a lot of overhead

@alanpoulain
Copy link
Member

@develth Nice, thanks! You should use (or create) another test resource, not the Dummy one which is overused.

@develth
Copy link
Contributor

develth commented Nov 8, 2022

Will do it and mention you on the other PR

develth added a commit to develth/api-platform-core that referenced this pull request Nov 8, 2022
alanpoulain pushed a commit to develth/api-platform-core that referenced this pull request Dec 13, 2022
@alanpoulain
Copy link
Member

Superseded by #5169.

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.

3 participants