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

OneToMany relation when using custom identifier in GraphQL returns null/wrong data #1542

Open
itsmitul opened this issue May 30, 2020 · 6 comments

Comments

@itsmitul
Copy link

itsmitul commented May 30, 2020

API Platform version(s) affected: 2.5.6

Description
When using custom identifier that is not a primary key, OneToMany result set returns null or wrong data. This issue is caused because when fetching data from OneToMany table, the custom identifier value is used instead of primary key.

How to reproduce

Table Blog has a custom identifier slug and primary key id. This table have a OneToMany relationship with Table Image

These are the queries made while fetching data:

SELECT b0_.id AS id_0, b0_.content AS content_1,  b0_.slug AS slug_3 FROM blog b0_ WHERE b0_.slug = "highest peak"

SELECT i0_.id AS id_0, i0_.src AS src_1 FROM image i0_ WHERE i0_.blog_id = "highest peak" ORDER BY i0_.id ASC LIMIT 30

While fetching from image , Blog ID should be sent in parameters but slug is used.

However, things are working fine in REST.

Update: Works fine in 2.5.4

@mxmp210
Copy link

mxmp210 commented Jun 4, 2020

Indeed there is something wrong with IRI converter, referencing Custom identifier and subresources, not just docs issue but it works when you pass real ID in url.
Steps to reproduce:

  1. Create Organization and News Entity, having OneToMany relation. so each org can have multiple news. Also News & Organization are api sub-resources.
  2. Create Organization 1 with org1 as it's code. News 1 as First news, News 2 as Second News.
  3. Add org1 reference to both news entities.

Test Cases:

Test Case Path Entity REST GraphQL
GET /api/news/1 News Identifier Returned org1 Identifier Returned org1
GET /api/organization/org1 Organization Identifier Acceptedorg1 Identifier Accepted org1
GET /api/news/1/organization Subresource Organization Identifier Accepted1 Organization Returned with proper iri org1 Identifier Accepted1 Organization Returned with proper iri org1
GET /api/organization/org1/news Subresource News Identifier Acceptedorg1 null returned for news Identifier Acceptedorg1 null returned for news
GET /api/organization/1/news Subresource News Identifier Accepted 1 collection returned for news Identifier Rejected 1 404

So there's bug in DataProvider which do not convert IRI;s to proper internal ID's and query with code directly. Shouldn't it be checking and getting item first and then passing right identifier to doctrine in the first place?

Here is relevant query from data provider which clearly passes code as id which is not valid as id's are integer values and code is alphanumeric value stored in different column.

SELECT n0_.id AS id_0, n0_.title AS title_1, n0_.description AS description_2, n0_.image AS image_3, n0_.created_at AS created_at_4, n0_.updated_at AS updated_at_5, n0_.organization_id AS organization_id_6 FROM news n0_ WHERE n0_.organization_id = 'org1' ORDER BY n0_.id ASC LIMIT 30;

@stephanvierkant
Copy link

Related? api-platform/core#3585

@mxmp210
Copy link

mxmp210 commented Jun 5, 2020

Changing $association['inversedBy'] at line #213 in subresource data provider seems to fix the problem, can anyone confirm there are no side effects? I'm still pretty new to api-platform so experienced opinion is required here. Should be reviewed by someone having already written this Provider, because condition is not behaving as it was intended, it's same as removing whole optimization part for now.

Update:
It seems there is problem with IDENTITY function itself, in ideal cases doctrine is supposed to have single indexed column and it will return as expected. If we have other identifier then we need to join table, given identifier is indeed unique in database with UNIQUE constraint. Here is the code that optimizes it further and resolves to respective Query. If anyone has better suggestions, feel free to give inputs.

<?php 
if ($isLeaf && $oneToManyBidirectional) {
    $optimized_query = null;
    //Check if passed key is identifier
    if($key === $classMetadata->getSingleIdentifierFieldName()) {
        //Key is ID & we can go with IDENTITY Function to skip joins
        return $previousQueryBuilder->andWhere("IDENTITY($previousAlias) = :$placeholder");
    }     
    else {
        //API identifier is different, we need join to identify internal object Traditional Way
        $joinAlias = $queryNameGenerator->generateJoinAlias($association['mappedBy']);
        return $previousQueryBuilder->innerJoin("$previousAlias", $joinAlias)->andWhere("$joinAlias.$key = :$placeholder");
    }          
}

@soyuka
Copy link
Member

soyuka commented Jun 5, 2020

Thanks for your investigation @mxmp210 ! Could you open a PR for this?

@mxmp210
Copy link

mxmp210 commented Jun 6, 2020

Thanks for your investigation @mxmp210 ! Could you open a PR for this?

@soyuka Sure I'd love to, First it needs to be tested for nested subresources as well, in theory it should work but just to make sure it won't break in the future. Are there any built-in tests that which can be performed out-of-box for the usecase? Sorry this would be my first PR, little help required here.

@mxmp210
Copy link

mxmp210 commented Dec 28, 2023

@itsmitul This issue was resolved in later versions, seems it's safe to close.

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

No branches or pull requests

4 participants