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

GraphQL: Inline Fragment on Array Fields #5908

Merged
merged 6 commits into from Aug 14, 2019

Conversation

@Moumouls
Copy link
Member

commented Aug 12, 2019

#5894
#5863

Allow to dive into pointers contained in an array field

  • Spec
  • Implementation
  • Optimization
@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

@davimacedo Do you have any feedback on the Spec?

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

It looks really good to me! It's for sure the best solution for the issue. Let's move forward with this spec.

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

So a bad news is that GraphQL don't currently support the union with Scalar.
I will create a new type Element with a field value that contain the value of the array element.

graphql/graphql-spec#215

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

So what do you think about the schema below?

type ArrayItem {
  value: Any
  pointer: Class # we already have an interface called Class
}

type SomeClass {
  arrayField: [ArrayItem!]
}
@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

The new spec look like this:

query GetCustomer($objectId: ID!) {
  objects {
    getCustomer(objectId: $objectId) {
      objectId
      manyRelations {
        ... on CustomerClass {
          objectId
          someCustomerField
          arrayField {
            ... on Element {
              value
            }
          }
        }
        ... on SomeClassClass {
          objectId
          someClassField
        }
      }
      createdAt
      updatedAt
    }
  }
}
@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

With my suggestion it would be:

query GetCustomer($objectId: ID!) {
  objects {
    getCustomer(objectId: $objectId) {
      objectId
      manyRelations {
        pointer {
          ... on CustomerClass {
            objectId
            someCustomerField
            arrayField {
              value
            }
          }        
          ... on SomeClassClass {
            objectId
            someClassField
          }
        }
      }
      createdAt
      updatedAt
    }
  }
}

It is little bit harder for returning the pointers (but still intuitive) and a lot easier for returning no pointer values (most common scenario).

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

You can also return objectId, createdAt, and updatedAt without using the inline fragment (a lot easier):

query GetCustomer($objectId: ID!) {
  objects {
    getCustomer(objectId: $objectId) {
      objectId
      manyRelations {
        pointer {
          objectId
          createdAt
          updatedAt
        }
      }
      createdAt
      updatedAt
    }
  }
}
@codecov

This comment has been minimized.

Copy link

commented Aug 13, 2019

Codecov Report

Merging #5908 into master will decrease coverage by 10.73%.
The diff coverage is 98.14%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #5908       +/-   ##
===========================================
- Coverage   93.69%   82.95%   -10.74%     
===========================================
  Files         153      153               
  Lines       10769    10801       +32     
===========================================
- Hits        10090     8960     -1130     
- Misses        679     1841     +1162
Impacted Files Coverage Δ
src/GraphQL/ParseGraphQLSchema.js 97.39% <100%> (+0.02%) ⬆️
src/GraphQL/loaders/parseClassQueries.js 97.36% <100%> (+0.14%) ⬆️
src/GraphQL/parseGraphQLUtils.js 92.3% <100%> (+17.3%) ⬆️
src/GraphQL/loaders/parseClassMutations.js 98.83% <100%> (ø) ⬆️
src/GraphQL/loaders/parseClassTypes.js 85.97% <100%> (-0.67%) ⬇️
src/GraphQL/loaders/defaultGraphQLTypes.js 97.17% <92.85%> (-0.26%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 3% <0%> (-93.68%) ⬇️
src/Adapters/Storage/Postgres/PostgresClient.js 78.57% <0%> (-7.15%) ⬇️
src/Controllers/UserController.js 93.45% <0%> (-0.94%) ⬇️
src/Routers/UsersRouter.js 93.54% <0%> (-0.65%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9031961...ecd8237. Read the comment docs.

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

I think inline fragment will do the job to dive into arrays, i don't really see real benefits of data architecture like:

type ArrayItem {
  value: Any
  pointer: Class # we already have an interface called Class
}

because it's the native behavior of Inline Fragment.
Using only Inline Fragment we allow developers to .map() easly on the array too.

It's now fully functional and tested, i invite you to give it a try on localhost 🚀

@Moumouls Moumouls marked this pull request as ready for review Aug 13, 2019

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

I currently don't understand why Postgres fail, do you have an idea @davimacedo ?

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

I am not sure but I guess it is something related to the selectedFields. Because of the inline fragment, it is probably returning more keys than what you expected, you are sending these keys to Postgres and it is failing because the field does not exist in the class. Try to debug the selectedValues that you are passing to the rest api. If you prefer I can also take a look on this later today.

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@davimacedo so the postgres error is due to keys (alias select) passed to the REST API. I think it's a Parse Server Postgres Error, it fail to query on an array with multi Pointers types and a deep select constraint. To fix this bug i'm currently using the root field on the select.

Effects: More data returned to Parse Server GraphQL on field Objects and Postgres compatible.

@davimacedo
Copy link
Member

left a comment

Great job! I added few questions.

src/GraphQL/parseGraphQLUtils.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/defaultGraphQLTypes.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/defaultGraphQLTypes.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/defaultGraphQLTypes.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/parseClassQueries.js Outdated Show resolved Hide resolved
@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Everything else seems good to me!

@davimacedo
Copy link
Member

left a comment

Great job! LGTM!

@davimacedo davimacedo merged commit 4bffdce into parse-community:master Aug 14, 2019

4 checks passed

Danger All good
Details
codecov/patch 98.14% of diff hit (target 93.69%)
Details
codecov/project Absolute coverage decreased by -10.73% but relative coverage increased by +4.45% compared to 9031961
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.